Merged
Conversation
nathanjcochran
commented
Nov 25, 2025
Comment on lines
+45
to
+55
| // Bind persistent flags to viper | ||
| // Use cmd.Flags() which includes inherited persistent flags from parents | ||
| if err := errors.Join( | ||
| viper.BindPFlag("debug", cmd.Flags().Lookup("debug")), | ||
| viper.BindPFlag("service_id", cmd.Flags().Lookup("service-id")), | ||
| viper.BindPFlag("analytics", cmd.Flags().Lookup("analytics")), | ||
| viper.BindPFlag("password_storage", cmd.Flags().Lookup("password-storage")), | ||
| viper.BindPFlag("color", cmd.Flags().Lookup("color")), | ||
| ); err != nil { | ||
| return fmt.Errorf("failed to bind flags: %w", err) | ||
| } |
Member
Author
There was a problem hiding this comment.
I moved these bind calls up into the root command's PersistentPreRunE, even though there wasn't really a problem with how these were being bound before (since they're global flags that always apply anyways). I figured it was more consistent to do it this way, and this should hopefully help encourage people to follow the new pattern of binding flags in the (Persistent)PreRunE functions instead of in the builder functions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This follows up on an issue originally described here. Namely, we were previously having trouble binding subcommands'
--outputflags to viper, and were therefore doing it manually in the subcommands'RunEfunctions instead, like this :While that technically worked fine, it stood out as a weird workaround.
Turns out the problem was simply that we were trying to bind multiple
*pflag.Flaginstances to the sameoutputkey in viper, since we were binding the flags in the subcommand builder functions (which all run unconditionally, no matter which subcommand is actually being invoked). As a result, theviper.BindPFlag("output", ...)calls would overwrite each other, and only the last--output*pflag.Flaginstance would actually end up being bound to viper.This issue is apparently fairly common, and is described here: spf13/viper#233. A solution is recommended here: spf13/viper#233 (comment).
This PR updates the code to bind the
--outputflags to viper instead of the manual workaround, and follows through on the recommendation linked above - namely, it moves all of theviper.BindPFlag()calls into the subcommands'PreRunEfunctions (via a helper function that makes this a one-liner), so that only the flags for commands that are actually being invoked are bound to viper. That ensures that only the relevant--outputflag is bound and takes effect.CC @murrayju since you're the one who originally ran into this issue.