-
Notifications
You must be signed in to change notification settings - Fork 124
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
Rename configuration file to cobra-cli.yaml #6
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
LGTM to me. Since its a behavior change on before a release I'll defer the merging to the (already) tagged folks. |
@@ -57,23 +57,34 @@ func init() { | |||
} | |||
|
|||
func initConfig() { | |||
viper.AutomaticEnv() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason this was moved above config file loading? I'm not familiar with what this does, but if possible, I'd like to keep this PR scoped to just the config file changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call to viper.AutomaticEnv()
was originally done before the call to viper.ReadInConfig()
and since I moved the latter to within the if/else logic, I moved the viper.AutomaticEnv()
higher up to avoid duplicating it. But I can keep it closer to viper.ReadInConfig()
by coding it twice.
Thanks @liggitt for realizing that I missed the dot-prefix for all the config file references! |
515d607
to
35b8f75
Compare
With the renaming of the generator to cobra-cli, this commit also renames the configuration file as ".cobra-cli.yaml". For backwards- compatibility, if "$HOME/.cobra-cli.yaml" can't be read, we fallback to "$HOME/.cobra.yaml". Co-authored-by: Jordan Liggitt <jordan@liggitt.net> Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
35b8f75
to
abd44aa
Compare
Should we go ahead and merge this since the v1.3.0 release is done? |
This PR is being marked as stale due to a long period of inactivity |
@marckhouzam |
This PR is being marked as stale due to a long period of inactivity |
This PR is being marked as stale due to a long period of inactivity |
This PR is being marked as stale due to a long period of inactivity |
This PR is being marked as stale due to a long period of inactivity |
This PR is being marked as stale due to a long period of inactivity |
@marckhouzam - if you're feeling good with this one, let's go ahead and merge it 👀 |
Thanks @jpmcb. I'll rebase then merge |
During the discussion about creating the
cobra-cli
repo @spf13 hinted that we should rename the configuration file tocobra-cli.yaml
but keep supporting thecobra.yaml
name as a fallback. Please see spf13/cobra#1597 (comment)This PR does this. If a different approach is preferred, just let me know.
/cc @liggitt @jpmcb