promu icon indicating copy to clipboard operation
promu copied to clipboard

Parameter / flag parsing from YAML does not remove quotes around quoted arguments

Open juliusv opened this issue 4 years ago • 7 comments

The parameter parsing code at https://github.com/prometheus/promu/blob/04550494378ed57e36ba704721cbefd26f4253ee/util/sh/sh.go#L46-L50 allows extracting quoted arguments (to allow such things as -tags 'foo bar'), but it does not remove the quotes around quoted arguments before passing this to the execution layer. Thus, quoted arguments aren't currently working correctly.

I wonder overall whether we should try to parse this as one line or go the "normal" way of just having a YAML list of command-line arguments with no postprocessing needed after YAML parsing.

juliusv avatar Oct 02 '19 12:10 juliusv

Hmm do you have an example showing what isn't working? In any case, I agree that it would be better to have a list.

simonpasquier avatar Oct 03 '19 10:10 simonpasquier

Yeah, I tried changing

https://github.com/prometheus/prometheus/blob/53ea6d63901b96a13bb79ff598c96af4a979adce/.promu.yml#L15

...to:

    flags: -mod=vendor -a -tags 'netgo release'

...and it didn't set the release tag (I guess it also didn't set the netgo tag then, but nowadays it appears that it results in a static binary even without that tag).

juliusv avatar Oct 03 '19 10:10 juliusv

Hmm, I swear that I had tested something similar and it did take into account the build tags but now I see what you describe.

As a quick workaround, we can change -tags foo bar to -tags foo,bar.

From https://golang.org/cmd/go/#hdr-Compile_packages_and_dependencies

-tags tag,list a comma-separated list of build tags to consider satisfied during the build. For more information about build tags, see the description of build constraints in the documentation for the go/build package. (Earlier versions of Go used a space-separated list, and that form is deprecated but still recognized.)

simonpasquier avatar Oct 03 '19 14:10 simonpasquier

@simonpasquier Thanks! I didn't know that Go 1.13 supports comma-based separation now, that failed for me yesterday with an older Go version, but now it works :)

juliusv avatar Oct 03 '19 14:10 juliusv

@juliusv I didn't know either until today :-) I suspect that they moved away from the space-separated list because quoting is hard.

simonpasquier avatar Oct 03 '19 14:10 simonpasquier

I suspect this can be closed?

jan--f avatar Sep 19 '23 11:09 jan--f

@jan--f From what I understand, the original bug still exists, I just ended up working around it for my use case back then.

juliusv avatar Sep 19 '23 14:09 juliusv