-
Notifications
You must be signed in to change notification settings - Fork 172
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
refactor: Create structs for each command to better isolate commands #3322
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Maciej Szulik <[email protected]>
✅ Deploy Preview for zarf-docs canceled.
|
message.Warn(lang.CmdDevSha256sumRemoteWarning) | ||
logger.From(cmd.Context()).Warn("this is a remote source. If a published checksum is available you should use that rather than calculating it directly from the remote link") | ||
// TODO(soltysh): get rid of pkgConfig global | ||
cmd.Flags().StringVar(&pkgConfig.InitOpts.GitServer.PushUsername, "git-account", types.ZarfGitPushUser, lang.CmdDevFlagGitAccount) |
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.
Yeah good TODO
Codecov ReportAttention: Patch coverage is
|
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.
This is a good direction for cmd imo. Lots of room for future improvements as well.
RunE: func(cmd *cobra.Command, _ []string) error { | ||
c, err := cluster.NewCluster() | ||
// Run performs the execution of 'connect' sub command. | ||
func (o *ConnectOptions) Run(cmd *cobra.Command, args []string) error { |
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.
Good choice to isolate these out into function declarations on the pkg
spinner := message.NewProgressSpinner(lang.CmdConnectPreparingTunnel, target) | ||
defer spinner.Stop() | ||
|
||
c, err := cluster.NewCluster() |
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.
Definitely not in scope for this PR, but sometime in the future it'd be great to ensure every CLI command can call a clearly defined API fn that encapsulates all of the configuration. I don't think this intent is well captured in an issue atm, but Connect has complex logic that could be abstracted out and made me think of the discussions we've had around it.
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.
Yes, definitely. Have a look at https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/events/events.go - that's my ideal command. There's a very strict boundary between the CLI bits (EventsFlags
struct) and actual command options (EventsOptions
struct).
The flow is you create EventsFlags
for the actual command (the goal is to allow easily bundling commands together (wait is a perfect example which is used in delete, and could easily be used elsewhere). ToOptions
transforms flags into EventOptions
struct, which has two important methods Validate
which goal is to validate correctness of all the options fields, and finally Run
which executes the code.
I'll be working on followup steps, once we get this in, but due to the amount of changes I want to do it slowly 😅 for ease of reviews.
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.
This all tracks - having explicit boundaries and a validation step are great.
rootCmd.AddCommand(connectCmd) | ||
connectCmd.AddCommand(connectListCmd) | ||
// ConnectOptions holds the command-line options for 'connect list' sub-command. | ||
type ConnectListOptions struct{} |
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 good pattern
rootCmd.AddCommand(NewConnectCommand()) | ||
rootCmd.AddCommand(NewDestroyCommand()) | ||
rootCmd.AddCommand(NewDevCommand()) | ||
rootCmd.AddCommand(NewInitCommand()) | ||
rootCmd.AddCommand(NewInternalCommand(rootCmd)) | ||
rootCmd.AddCommand(NewPackageCommand()) |
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.
This is the biggest win of the refactor imo -- we don't need to understand what magic implicit invocation order you get from a bunch of init()s being executed. Clear and declarative 👍
One note though, zarf say
's command should probably be added to this sequence. It's currently in Execute below, but it's better off grouped with everything else.
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.
Yes!
The other part I actually started considering is grouping commands, have a look at kubectl
output, it groups commands into several groups (basic, deploy, cluster management, etc...). We don't have that many, yet, but if we want to consider this construct will make it very easy to do just that.
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 a lot of sense to me, i'm in favor of it!
Description
This refactor is a first step to isolating all the command, which should allow easier consumption of zarf in other places.
Related Issue
Relates to #2773
Checklist before merging