fix(foreach): Remove shell invocation from foreach #136
Conversation
annettejanewilson
force-pushed
the
simpler-foreach
branch
from
eb3cac4
to
910e907
Compare
|
Let's think about this, as it does look nicer - FYI @sledigabel I need to backtrack and check why we used a shell in the first place. From rough memory I vaguely recall frustration with there being enough real-world situations where a shell was needed, but that could be mistaken. I'll check slack history, particularly back on the internal pre-go (bash!) prototype of turbolift. |
|
@sledigabel, @Dan7-7-7 and I had our regular scheduled catchup yesterday; we're kinda on the fence about this, partly because it would be a breaking change and requires ordinary users (who are not necessarily POSIX power users) to learn the We also discussed, and looked up in Slack, the scenarios where shell has been desirable in the past; these are roughly in two categories:
Running some quick local tests with this branch I think we're safe-ish on the first but it would be a breaking change for the second: ^ Evidently the ASDF shims are still used ^ ... and actually apply ^ using shell syntax behaves as might be expected. Not shown: wrapping a scriptlet in single quotes used to work, but now doesn't work in our release version. ^ workaround necessary to execute multiple commands, if one does not want to put them in an actual script file. |
|
One other thing we wondered: given now mandatory use of |
|
FYI I fully implemented this with a My feeling is that it's best to provide a simple and predictable building block that handles 90% of cases as-is and which doesn't get in the way when you want to do the complex ones: you can easily invoke a shell yourself because you can be confident that your arguments get through to it untouched. This will need release notes, but I also know people are likely to be using this through brew and not seeing release notes. Do you think there's anything needed for the change in behaviour? I see 2.4.1 was released yesterday. I was hoping we could resolve this before releasing that. Here's a handy summary:
|
We could check that Note that it's not mandatory, just strongly recommended to avoid confusion. |
I think in the current state of the PR it is mandatory in order to avoid the command options from being swallowed by Cobra since we're not disabling the parsing any longer. So it's either using Cobra is too clever for this so checking the arg length sounds like a good strategy. |
|
Yes, it can be avoided but quickly becomes needed if any flags are used for the command. I was going to say we could emit a prominent warning, but most people when faced with a warning and an error only see the error. Perhaps we should just make it mandatory, and bomb out with our own very clear message if it's not been used? |
|
I've pushed a change to make
The problem here is that the brief usage instructions shown when the arguments are incorrect do not include either the "Long" or the "Short" description of the command. See cobra #1084. In all cases – brief or full – the usage instructions show: It's just that only in the full usage instructions is there text to reinforce the need for |
|
Oh, lol, now I type that out I see that |
|
Oh, I can't! It's put there by Cobra. Huh. Why does it put it at the end? Who puts flags at the end? |
As discussed in [issue 135], turbolift applies shell escaping inconsistently on the arguments in foreach. Upon consideration, I think there is no good reason to apply shell escaping at all. It makes turbolift more fragile, more confusing and ultimately less flexible. Instead, do not use a shell at all and all these problems disappear. None of the examples in the README use this functionality. If users do need to invoke a shell, for example to do redirection, it is much more explicit and understandable for them if they do it themselves. This PR: * Removes the use of a shell, instead directly executing a subprocess based on the arguments provided. * Removes custom handling of arguments. Users are directed to the standard GNU-style `--` to indicate that everything else on the command-line is an argument, not an option. This makes the code substantially simpler. [issue 135]: Skyscanner#135
Shell escape the arguments so you can tell when arguments have spaces in them.
Since we are deliberately removing custom handling of arguments, users will find that if they don't include a double hypen (`--`) separator before their foreach command, any options they pass to their command will be interpreted by turbolift and either result in errors, or worse, being quietly omitted from their command (e.g. `-v`), which could be surprising and frustrating! To head this off, refuse immediately if `--` either doesn't appear on the command-line, or appears after one or more arguments.
Make it clear that the double-hyphen separator `--` is mandatory and not just a good idea.
Turns out Cobra doesn't wrap usage messages, so they look pretty ugly if we don't pick a pretty conservative width and wrap by hand.
Turns out Cobra searches your usage line for the specific text "[flags]" and if it doesn't find it, shoves it on the end. Gross. For that reason, let's remove explicit callout of any flags and move it back to *before* the double-hyphen.
annettejanewilson
force-pushed
the
simpler-foreach
branch
from
91dfe89
to
52aecd5
Compare
|
Okay, I'm happy with this again now. I've rebased again |
annettejanewilson
changed the title
|
Please let me know if you would prefer the invocation formatting in a separate PR. I think maybe that would make sense as it's not a necessary part of this fix, more a feature in itself, and it should probably be applied more broadly than just to foreach. I note that right now it seems like all the invocations get reported twice by different parts of the code - once in foreach and again in executor. Again, that might be better dealt with in its own PR. I already did the work to separate it into its own commit, so I think it could easily be dropped from this PR. |
|
I think it looks good @annettejanewilson. The formatting can stay in the same PR as far as I'm concerned, it's a nice addition to this refactor. We can prob live with the error message triggered if the user omits the |
Wrap braces around the logged command to delimit it clearly. Add unit tests to cover argument formatting.
Done. |
Ah, sorry, I missed this longer reply earlier as we had multiple messages cross over - I only saw the smaller one following it. @rnorth Did I convince you with the table above? I would still argue this is a more coherent approach, but if we don't do this I think the status quo in 2.4.0 was more useful than where 2.4.1 ends up. |
|
Suggested text for release notes:
|
There was a problem hiding this comment.
LGTM, the change might cause minor disruption for users but I think ultimately it's more consistent / less confusing this way.
Re-approving after my comment but will let @rnorth and @sledigabel have the final say.
|
Let's merge - I agree with the approach. Thanks for this @annettejanewilson ! |
As discussed in issue 135, turbolift applies shell escaping inconsistently on the arguments in foreach. Upon consideration, I think there is no good reason to apply shell escaping at all. It makes turbolift more fragile, more confusing and ultimately less flexible.
Instead, do not use a shell at all and all these problems disappear. None of the examples in the README use this functionality. If users do need to invoke a shell, for example to do redirection, it is much more explicit and understandable for them if they do it themselves.
This PR:
--to indicate that everything else on the command-line is an argument, not an option. This makes the code substantially simpler.