Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken list parsing in cli arguments --tags and --experiments #772

Merged
merged 2 commits into from
May 22, 2018

Conversation

lox
Copy link
Contributor

@lox lox commented May 18, 2018

Currently, buildkite-agent start --tags "a=1,b=2" sets a single tag of a=1,b=2. This is a shortcoming of the upstream urfave.cli library (see urfave/cli#62) which expects --tags "a=1" --tags "a=2".

This adds a normalization for --tags and --experiments that makes them behave as expected.

lox added 2 commits May 18, 2018 17:18
Currently, `buildkite-agent start --tags "a=1,b=2" sets a single tag of
"a=1,b=2". This is a shortcoming of the upstream urfave.cli library
which expects --tags "a=1" --tags "a=2".

This adds a normalization for --tags and --experiments that makes them
behave as expected.
@lox lox requested a review from keithpitt May 18, 2018 07:23
@keithpitt
Copy link
Member

Oh, I assume it's fine with --tags "foo=bang,bar,bing"

@keithpitt
Copy link
Member

(Where you specifically want foo to be a comma separated list..)

@lox
Copy link
Contributor Author

lox commented May 21, 2018

Nope! Won't work!

@lox
Copy link
Contributor Author

lox commented May 21, 2018

Wouldn't work in config either.

@keithpitt
Copy link
Member

As in, it has never worked? Or, it won't work with the current changes?

I know that some customers use this syntax for things like:

--tags "supports=ruby1.9.3,mysql,node-1.2.3"

@keithpitt
Copy link
Member

(Actually, I can't remember if before cli used to expand them into multiple values...)

@lox
Copy link
Contributor Author

lox commented May 21, 2018

Yeah, it's really perplexed me too. I'm pretty sure the example you just gave would have worked via the cli argument (because no splitting occurred), but failed via a config key.

@lox
Copy link
Contributor Author

lox commented May 21, 2018

See

value = strings.Split(configFileValue, ",")

@lox
Copy link
Contributor Author

lox commented May 21, 2018

I have no idea how to fix this with the current config syntax, beyond possibly suggesting people can escape commas? We'd need to add support for that.

@keithpitt
Copy link
Member

Just chatted with @lox in real life, we've never supported commas in tag values before, so it's fine to ship as-is!

@lox lox merged commit c739ea9 into master May 22, 2018
@lox lox deleted the fix-list-parsing-in-cli-arguments branch May 22, 2018 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants