Skip to content

Avoid calling flag.Parse() twice. #1997

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

Merged
merged 9 commits into from
Jan 21, 2020
Merged

Avoid calling flag.Parse() twice. #1997

merged 9 commits into from
Jan 21, 2020

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Jan 17, 2020

flag.Parse() was called twice previously, first to get -config.file value to parse the config file, and second to parse remaining command line parameters – overwriting values from config file.

Unfortunately that confuses command line parameters that accept multiple values (eg. -memberlist.join or -experimental.tsdb.block-ranges-period in #1942). In this PR we get -config.file via a separate FlagSet (which doesn't report any error or usage) to avoid calling flag.Parse() on default FlagSet twice.

This should not be visible to end user in any way.

flag.Parse() was called twice previously, first to get -config.file
value to parse the config file, and second to parse remaining command
line parameters – overwriting values from config file.

Unfortunately that confuses command line parameters that accept multiple
values. Here we get -config.file via a separate FlagSet to avoid calling
flag.Parse() (on default FlagSet) twice.

Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany pstibrany mentioned this pull request Jan 17, 2020
3 tasks
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces a regression. When I run Cortex from this PR branch with the TSDB storage I get this:

level=error ts=2020-01-17T16:47:19.9634104Z caller=log.go:141 msg="error initializing cortex" err="Must set -dynamodb.url in aws mode\nerror initialising module: table-manager\ngithub.com/cortexproject/cortex/pkg/cortex.(*Cortex).initModule\n\t/workspace/src/github.com/cortexproject/cortex/pkg/cortex/cortex.go:239\ngithub.com/cortexproject/cortex/pkg/cortex.(*Cortex).init\n\t/workspace/src/github.com/cortexproject/cortex/pkg/cortex/cortex.go:227\ngithub.com/cortexproject/cortex/pkg/cortex.New\n\t/workspace/src/github.com/cortexproject/cortex/pkg/cortex/cortex.go:185\nmain.main\n\t/workspace/src/github.com/cortexproject/cortex/cmd/cortex/main.go:73\nruntime.main\n\t/usr/local/Cellar/go/1.13/libexec/src/runtime/proc.go:203\nruntime.goexit\n\t/usr/local/Cellar/go/1.13/libexec/src/runtime/asm_amd64.s:1357"

I've the feeling it has to do with defaults. The flagext.RegisterFlags() does also set default values on the config. With this PR's change, we're calling RegisterFlags() after loading the config file, so we're overriding what we loaded from the config with the default values from flags. flagext.RegisterFlags() should be called before parsing the config file.

Would be great if we could have a unit test on this (maybe moving all config file + CLI flags logic in a single function to unit test), but if it's too complicated just skip it and do some manual tests please 😉

@pstibrany
Copy link
Contributor Author

This PR introduces a regression.... [del]
I've the feeling it has to do with defaults. The flagext.RegisterFlags() does also set default values on the config. With this PR's change, we're calling RegisterFlags() after loading the config file, so we're overriding what we loaded from the config with the default values from flags. flagext.RegisterFlags() should be called before parsing the config file.

Would be great if we could have a unit test on this (maybe moving all config file + CLI flags logic in a single function to unit test), but if it's too complicated just skip it and do some manual tests please 😉

Thanks for your review and my apologies! I was happy that flags work correctly that I forgot to check actual usage of config file 🤦‍♂ You're right about RegisterFlags -- they actually set default values from the flags to config file. I missed that completely! Now it's fixed, and I've also added unit tests for main and various combinations of config file and flags, including default values set by flags (by testing -target=all).

@pstibrany
Copy link
Contributor Author

Tests work by 1) introducing "test mode" to main() function, which only does config/flags parsing, dumps resulting YAML and then stop, and 2) capturing stdout/stderr and checking for expected messages.

Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, interesting approach for testing the main function. I've never seen something like that before.

Signed-off-by: Peter Štibraný <[email protected]>
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good job. The testMode doesn't make the main() much dirty and looks a good strategy to have smoke tests on CLI flags / YAML config parsing. LGTM!

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.

3 participants