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 argument parsing bugs by using dedicated Viper instances #60

Merged
merged 10 commits into from
Jan 4, 2023

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Jan 3, 2023

This stopped working as expected, instead generating a --generic style version:

pulumictl get version --language python

The first bad commit is f5ca64e and last good commit is 2218df7.

It turns out the issue is with viper arg parsing. Introducing a new "github.com/pulumi/pulumictl/cmd/pulumictl/convert-version" module with a new command reused the global viper instance. So this got called twice in different commands:

	viper.BindPFlag("language", command.Flags().Lookup("language"))

And it broke. Basically the new convert-version command broke the old get version command.

This is rather alarming, so the fix introduces dedicated viper state into every sub-command to avoid this collision in the future.

@t0yv0
Copy link
Member Author

t0yv0 commented Jan 3, 2023

Err, this might not be quite right. githubToken = viper.GetString("token") may fail to work in sub-commands that don't bind it correctly to the local viper instance anymore. Some of this sharing was intentional?

@t0yv0
Copy link
Member Author

t0yv0 commented Jan 4, 2023

Appended a few fixes, this should work now. When doing viper.GetString("token") most subcommands imply the "token" that was bound in main.go so this lookup should still use global viper state; for most of the variables they're bound locally and should use local viper.New().

@t0yv0 t0yv0 merged commit 8dea303 into master Jan 4, 2023
@t0yv0 t0yv0 deleted the t0yv0/fix-arg-parsing branch January 4, 2023 00:09
@t0yv0 t0yv0 mentioned this pull request Jan 4, 2023
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