refactor(cli): restructure commands, option flags and validation

[hiddeco]

Touches into #1163

This pull request achieves multiple things:

1. Commands no longer make use of lengthy in-line funcs to perform an action. Instead, they are built around "option" structs with methods to add flags, parse command arguments, validate, and run the actual command logic. By doing this, we introduce a separation of concerns while also allowing things to (hypothetically) be tested without relying on further Cobra functionalities.
2. Flags are now registered as required, mutually exclusive, taking file and/or directory inputs, etc. using Cobra features. Which improves out-of-the-box validation, while also enriching the shell completion instructions. For more information around this, see e.g. https://pkg.go.dev/github.com/spf13/cobra#Command.MarkFlagRequired

Example boilerplate new command

struct fooOptions {
	SomeArg  string
	SomeFlag string
}

func NewCommand() *cobra.Command {
	cmdOpts := &fooOptions{}

	cmd := &cobra.Command{
		Use:   "foo (ARG) --some-flag=value",
		Args:   option.ExactArgs(1),
		Short: "Perform a demo operation",
		// Other Cobra fields omitted for brevity...
		RunE: func(cmd *cobra.Command, args []string) error {
			cmdOpts.complete(args)

			if err := cmdOpts.validate(); err != nil {
				return err
			}

			return cmdOpts.run()
		},
	}

	// Register the option flags on the command.
	cmdOpts.addFlags(cmd)

	return cmd
}

func (o *fooOpts) addFlags(cmd *cobra.Command) {
	cmd.Flags().StringVar(&o.SomeFlag, "some-flag", "", "An example flag which requires a value")

	if err := cmd.MarkFlagRequired("some-flag"); err != nil {
		panic(errors.Wrap(err, "could not mark some-flag flag as required"))
	}
}

func (o *fooOpts) complete(args []string) {
	o.SomeArg = strings.TrimSpace(args[0])
}

func (o *fooOpts) validate() error {
	var errs []error

	if o.SomeArg = "" {
		errs = append(errs, errors.New("arg is required"))
	}

	if o.SomeFlag == "" {
		errs = append(errs, errors.New("some-flag is required"))
	}

	return errors.Join(err...)
}

func (o *fooOpts) run() error {
	// Do something intelligent with arg and flag...

	return nil
}

[netlify]

✅ Deploy Preview for docs-kargo-akuity-io ready!

Name	Link
🔨 Latest commit	8b3f941
🔍 Latest deploy log	https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/65e0a080d863ef0008f7fc35
😎 Deploy Preview	https://deploy-preview-1547.kargo.akuity.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 926 lines in your changes are missing coverage. Please review.

Project coverage is 30.63%. Comparing base (f303e9b) to head (8b3f941).

Files	Patch %	Lines
internal/cli/cmd/apply/apply.go	0.00%	83 Missing ⚠️
internal/cli/cmd/promote/promote.go	0.00%	76 Missing ⚠️
internal/cli/cmd/create/create.go	0.00%	63 Missing ⚠️
internal/cli/cmd/login/login.go	0.00%	63 Missing ⚠️
internal/cli/cmd/get/freight.go	0.00%	57 Missing ⚠️
internal/cli/cmd/delete/delete.go	0.00%	56 Missing ⚠️
internal/cli/cmd/get/warehouse.go	0.00%	53 Missing ⚠️
internal/cli/cmd/create/project.go	0.00%	52 Missing ⚠️
internal/cli/cmd/get/promotions.go	0.00%	49 Missing ⚠️
internal/cli/cmd/get/stages.go	0.00%	45 Missing ⚠️
... and 13 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1547      +/-   ##
==========================================
- Coverage   30.96%   30.63%   -0.34%     
==========================================
  Files         183      183              
  Lines       19303    19516     +213     
==========================================
  Hits         5978     5978              
- Misses      13058    13271     +213     
  Partials      267      267              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[krancour]

Ya... it's tricky. Almost but not all commands use them.

Personally, I'd rather repeat ourselves sometimes that have inapplicable flags show up on commands. i.e. If we had to choose, UX wins over DRY, imho.

[hiddeco]

A problem I have observed though is that we sometimes ourselves forget to actually wire them in (see e.g. https://github.com/akuity/kargo/blob/5912e5a9dfd2912fdd6a46e1b35655b6225c4454/internal/cli/cmd/approve/approve.go in current main, which AFAIK should have the flags set).

By registering them once at root and loading them into some config which is passed to downward commands, this could be avoided.

[krancour]

Is there a way to do it at root and unregister them in spots?

[hiddeco]

I think you can theoretically use MarkHidden to hide them from the help menu for specific commands.

[krancour]

These are new since the last time I went deep on Cobra. I am super excited about how much this cleans things up.

[krancour]

Kudos for this...

I've frequently seen this pattern of using a struct to encapsulate options and have rarely seen much value in it...

Coupled with the decision to extract validation into its own function, that pattern begins to make sense.

You've converted me.

Nit: Could be unexported.

[krancour]

@hiddeco overall, I love the approach. I commented on "apply" with some nits, questions, and kudos. I know this is only a draft right now, but I see nothing blocking so far.

[krancour]

I have not reviewed every single line, but reviewed enough of the commands to assert that the new pattern that's emerging is a major improvement.

My one nit is that run() as a func of an "options" type seems slightly counter-intuitive. I liked an example you showed me offline yesterday where e.g. instead of applyOptions, it may have been applyRun. Or even something like applyCmdImpl might be nice.

This is not in any way a blocker or a hill that I would die on.

[hiddeco]

My one nit is that run() as a func of an "options" type seems slightly counter-intuitive. [..]

When I started implementing that, I ran into some issues:

• fooRun#run felt odd ("run" twice)
• fooCmd#run felt better, but would conflict with e.g. cmd := &cobra.Command{}

Given it "runs" things with the options, I then felt that this convention was not that bad and stopped looking for alternatives.