Preventing excessive global variables (project layout/design)

[zanitytech]

After using cobra for the first time I experienced a few issues especially when using "Flags". I'd like to hear everyones advice/recommendation on this.

My project structure looks like this:

- my_project/
    - commands/
      tasks.go
      tasks_add.go

I'm using the design approach of commandname.go then command_subcommand.go, I feel like this is the best way to organize a project (any recommendations here is welcome)

Now my main issue is in each command.go file, what is the best way to handle Flags() variables without them being global. In large projects if we delare global variables the workspace becomes quickly bloated.

For example:

var name string // how do we avoid these globals?

func init() {
	tasksCmd.Flags().StringVar(&myFlag, "name", "", "name of the task")
	rootCmd.AddCommand(transferCmd)
}

var tasksCmd = &cobra.Command{
	Use:   "tasks",
	Short: "...",
	Long:  `...`,
	RunE: func(cmd *cobra.Command, args []string) error {
		// how to access Flags here neatly without globals?
	}
}

So what's everyone preferred design pattern regarding this?

[johnSchnake]

You could have a separate package constants for those so they don't pollute everywhere else. It reads easily too since you could even nest it for different types of constants:

	tasksCmd.Flags().StringVar(&myFlag, constants.flags.name, "", "name of the task")

That gets around how to avoid the globals for just the names.

As for how to reference them, I always create a closure and a custom input type. It helps with how to test the commands easily as well.

For example, https://github.com/vmware-tanzu/sonobuoy/blob/main/cmd/sonobuoy/app/delete.go#L38-L58

So you can see that we create the variables and bind them to those flags in the same method that creates the cobra *Command. Then the actual logic is just in a method that gets those values handed to it. No more doing flag lookups at all.

Recommend highly!

P.S. I want to be respectful and give others a chance to respond to this if they have other patterns. In an effort to clean up the backlog, though, I'll close the ticket in a few days if there isn't much other activity on it. You can reopen if you have further questions though.

[zanitytech]

Thanks for the example John. I ended up doing something similar actually with just creating a struct for each command to hold all the flags this works okay. But I like the idea of wrapping everything in a custom type so I'll likely go down that route as per your reference code.

[jpmcb]

I often create a command "options" struct that holds those variables and can be accessed at the scope of the Go package. Helps wrangle the variables and flags a bit. Something like:

package myCommand

...

type commandOptions struct {
    name string
}

var MyCommand = &cobra.Command{
    RunE: myCommandFunc,
}

// This is global to the myCommand package
var myOptions = commandOptions{}

func init() {
    // Populate the options in the myOptions struct via flags
    MyCommand.Flags().StringVar(&myOptions.name, "name", "", "the name to provide to myCommand")
}

func myCommandFunc(cmd *cobra.Command, args []string) error {
    // do the work, access myOptions struct with various flags and things populated
}

Elsewhere, you'd need to include this package and add it to a top level command.

topLevelCmd.AddCommand(
    myCommand.MyCommand,
    myOtherCommand.MyOtherCommand
)

[johnSchnake]

My only reason for avoiding the init method all together was that it made the behavior of unit tests more wonky if I recall. You get that init behavior for the first test but then not the others. Thats why I like to separate out all the command/flag logic into one section and the actual logic (which doesnt know about cobra/cli/args at all) in another.

@jpmcb I just responded to another issue similar to this (#1290) where they were complaining about the default app layout from cobra leading to these globals and promoting data races. What do you think the chances are we could rework that default command to use the patterns discussed above?

[zanitytech]

@jpmcb Thanks for that, this was my intial goto before @johnSchnake comment, this approach feels clean enough, but getting rid of the init()'s is definitely good practice I think the default design should follow this pattern

[jpmcb]

For reference, here's a chunk of code using init

@johnSchnake - I like the idea of reworking the recommended app layout

We can keep this open for a bit in case anyone else has an design pattern they'd like to share, then we can close this and make a new issue tackling re-working the recommended default design.

[johnSchnake]

With the CLI moving to its own repo, these sorts of issues are moving as well. RFC there spf13/cobra-cli#12