refactor: Create structs for each tools command to better isolate commands - cont'd #3340
Conversation
✅ Deploy Preview for zarf-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
@AustinAbro321 ptal |
| // Flags for using an external Git server | ||
| cmd.Flags().StringVar(&updateCredsInitOpts.GitServer.Address, "git-url", v.GetString(common.VInitGitURL), lang.CmdInitFlagGitURL) | ||
| cmd.Flags().StringVar(&updateCredsInitOpts.GitServer.PushUsername, "git-push-username", v.GetString(common.VInitGitPushUser), lang.CmdInitFlagGitPushUser) | ||
| cmd.Flags().StringVar(&updateCredsInitOpts.GitServer.PushPassword, "git-push-password", v.GetString(common.VInitGitPushPass), lang.CmdInitFlagGitPushPass) |
There was a problem hiding this comment.
Looks like this is getting set to a default value as seen in the markdown changes. IMO we should avoid setting the default here, a user might run zarf tools update-creds --registry-username="user" --registry-password="password" and have their git creds updated to the default unexpectedly.
This changed because previously v.GetString(common.VInitGitPushUser) was given a default after this command was loaded, but now tools is loaded last. We don't want to remove v.GetString(common.VInitGitPushUser) from this line since a user may set init.git.push_username in their config.toml file. Instead we could move rootCmd.AddCommand(tools.NewToolsCommand()) to be first in NewZarfCommand().
There was a problem hiding this comment.
Yeah, I've noticed that change, but I didn't dig too deep into where it came from. I'll check if the order you're talking about has an effect. Honestly, I'd prefer to rewrite it such that the order shouldn't matter.
There was a problem hiding this comment.
We don't want to remove
v.GetString(common.VInitGitPushUser)from this line since a user may setinit.git.push_usernamein their config.toml file. Instead we could moverootCmd.AddCommand(tools.NewToolsCommand())to be first inNewZarfCommand().
Sounds like we should consider defaulting and config values, and how these interact. I think once I reach the point where I will want to decouple config values and flags it might get addressed.
Before this change we have two separate methods, one for initializing Viper - InitViper, and another to retrieve it - GetViper. This may lead to either Viper not being initiated, thus leading to nil pointer dereference error or initializing more than once. After this change, we will ensure Viper is initialized only once. Signed-off-by: Maciej Szulik <[email protected]>
…mands Signed-off-by: Maciej Szulik <[email protected]>
Description
Continuation of the effort to isolate commands. Followup to #3322.
This PR additionally changes how viper is initiated, see the first commit for that specifically.
Related Issue
Relates to #2773
Checklist before merging