-
Notifications
You must be signed in to change notification settings - Fork 7
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
updated things to mostly work when a config file is not writeable #28
Conversation
Looks good - made 1 comment... |
stim/rootcmd.go
Outdated
@@ -28,6 +29,8 @@ func initRootCommand(viper *viper.Viper) *cobra.Command { | |||
viper.BindPFlag("noprompt", cmd.PersistentFlags().Lookup("noprompt")) | |||
cmd.PersistentFlags().StringP("auth-method", "", "", "Default authentication method (ex: ldap, github, etc.)") | |||
viper.BindPFlag("auth.method", cmd.PersistentFlags().Lookup("auth-method")) | |||
cmd.PersistentFlags().StringP("isautomated", "", "false", "Error on anything that needs to prompt and was not passed in as an ENV var or command flag") |
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.
nit-pick but can we do is-automated
to be consistent with the other vars?
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.
+1
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.
I originally had it that way but I was worried about how that translates into the config.yaml file. The only other example of this is auth-method, which ends up as auth.method as the lookup which looks like:
auth:
method: "test"
in yaml. I didnt like is-automated, translating to is.automated, which ends up in the config.yaml as:
is:
automated: true
so just moved to just is-automated. I can have the --is-automated then just not translate the lookup in to the config the same way. let me know what you guys think.
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.
That makes sense. However, I don't think that dashes in parameters need to always directly translate into child elements in the config. I could see us having some complex nesting in the config but want to simplify it for the command line parameters. With that said, I'm ok either way as we still need to clean up the config/arguments so we can address it then.
This PR fixes issue #27