bug: cmdline args need jsonification; tty support needs adjustment#27
bug: cmdline args need jsonification; tty support needs adjustment#27kvaps merged 1 commit intokvaps:masterfrom
Conversation
joesuf4 commented May 29, 2021
joesuf4 commented - switch shell to bash so we can use arrays and bulk element transforms
- use [ -t 0 ] to baseline whether we need terminal support for the invocation
joesuf4 force-pushed the jsonify-entrypoint branch 2 times, most recently from 3f4b412 to 8fca66b Compare - switch shell to bash so we can use arrays and bulk element transforms - use [ -t 0 ] to baseline whether we need terminal support for the invocation - send human-interest commentary to stderr instead of stdout
joesuf4 force-pushed the jsonify-entrypoint branch from 8fca66b to 9df9374 Compare 
| The motivation for this patch is that I want to use node-shell in CLI pipelines, so I am not interested in the default tty login shell API. I don't want "stuff" printed to stdout unless I'm expecting it from my CLI invocation, so I redirect that output to stderr. Also I have embedded newlines and double-quote characters in some of my CLI tooling that wraps node-shell, so I need better CLI->json-string translation than before I needed now. |
| Hi thank you for your PR, I'll review it in a while 👍 |
There was a problem hiding this comment.
Hi @joesuf4, sorry for late reply.
Your PR adds two new dependencies bash and sed.
I agree that using bash arrays makes it more convenient, but the original goal for this plugin was to keep it simplest and compatible with all possible architectures and any shell.
I believe that there is no reasons to upgrade shell version. Please take a look on these changes, I think I was able to make your changes compatible with standard bourne shell
joesuf4 left a comment
joesuf4 left a comment There was a problem hiding this comment.
Are we agreed on this MR?
| Ok. |
| Fighting with GitHub MR ui. Bear with me. |
| Merged in |
2 participants
