Skyscanner/turbolift

foreach sed fails with 'unknown shorthand flag' error

danielt998 opened this issue · 7 comments

danielt998 commented

I get the following error when running foreach sed with turbolift 2:

`$ turbolift foreach sed -i '' 's/foo/bar/g' README.md

Error: unknown shorthand flag: 'i' in -i
Usage:
turbolift foreach -- SHELL_COMMAND [flags]
Flags:
-h, --help help for foreach
Global Flags:
-v, --verbose verbose output
2021/05/21 11:49:08 unknown shorthand flag: 'i' in -i`

A temporary workaround is to encapsulate the sed command in double quotes, but I think this is a bug in the way parameters are handled. (also some of the README examples do not work either)

sledigabel commented

Good catch, I reckon we need to add DisableFlagParsing: True to the command here:
https://github.com/Skyscanner/turbolift/blob/main/cmd/foreach/foreach.go#L31-L36

As per the doc:
https://pkg.go.dev/github.com/spf13/cobra#Command

	// DisableFlagParsing disables the flag parsing.
	// If this is true all flags will be passed to the command as arguments.
	DisableFlagParsing bool
sledigabel commented

I'll prep up a PR and give this a go

rnorth commented

Ah, that's annoying. Does putting -- before the command make any difference? Eg. turbolift foreach -- sed ....

I'd need to check, and not sure if cobra handles that automatically to stop parsing flags. In theory, this might be the way, though.

sledigabel commented

@rnorth checkout #51
I think this should fix it

rnorth commented

Sure, but what I mean is that -- seems like the standard unix way to achieve the goal.

E.g. see https://unix.stackexchange.com/questions/11376/what-does-double-dash-mean

The downside of #51, I think, is that it will make it harder to implement actual flags for the foreach command, should they be needed (eg --parallel).

I think turbolift-bash was erroneous in that it didn't require -- - but maybe it should have done...

sledigabel commented

I agree -- is the preferred POSIX compatible solution, but I'd also argue it gets in the way of the user experience. We have another case like that with #53. Similarly, adding quotes around the command works since it then considers the command string as a single parameter, it makes sense from a shell perspective, but it's not particularly elegant.

My PR was due to the absence of any flag at the moment, and the idea was to look at it when the time comes but I see your point.

The docker cli has managed to fix it somehow.
image
They also use the TraverseChildren thing:
https://github.com/docker/cli/blob/master/cmd/docker/docker.go#L41

They are somehow parsing the arguments differently for the run, the codebase is a bit hairy i need to look deeper into it.