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

Rename configuration file to cobra-cli.yaml #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

marckhouzam
Copy link
Collaborator

During the discussion about creating the cobra-cli repo @spf13 hinted that we should rename the configuration file to cobra-cli.yaml but keep supporting the cobra.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

Copy link
Collaborator

@jpmcb jpmcb left a comment

Choose a reason for hiding this comment

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

Makes sense!

@johnSchnake
Copy link
Collaborator

LGTM to me. Since its a behavior change on before a release I'll defer the merging to the (already) tagged folks.

cmd/root.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@@ -57,23 +57,34 @@ func init() {
}

func initConfig() {
viper.AutomaticEnv()
Copy link
Collaborator

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

Copy link
Collaborator Author

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.

@marckhouzam
Copy link
Collaborator Author

Thanks @liggitt for realizing that I missed the dot-prefix for all the config file references!

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>
@marckhouzam
Copy link
Collaborator Author

Should we go ahead and merge this since the v1.3.0 release is done?

@marckhouzam marckhouzam added this to the v1.3.1 milestone Mar 17, 2022
@github-actions
Copy link

This PR is being marked as stale due to a long period of inactivity

@umarcor
Copy link
Contributor

umarcor commented May 17, 2022

@marckhouzam cmd/root.go was modified in 9e07cb5. Do you mind rebasing this PR?

@github-actions
Copy link

This PR is being marked as stale due to a long period of inactivity

@github-actions
Copy link

This PR is being marked as stale due to a long period of inactivity

@github-actions
Copy link

This PR is being marked as stale due to a long period of inactivity

@github-actions
Copy link

github-actions bot commented Apr 2, 2023

This PR is being marked as stale due to a long period of inactivity

@github-actions
Copy link

github-actions bot commented Jun 2, 2023

This PR is being marked as stale due to a long period of inactivity

@jpmcb
Copy link
Collaborator

jpmcb commented Jun 23, 2023

@marckhouzam - if you're feeling good with this one, let's go ahead and merge it 👀

@marckhouzam
Copy link
Collaborator Author

Thanks @jpmcb. I'll rebase then merge

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.

5 participants