Rethink Cobra code structure to remove use of global variables

[phillebaba]

Describe what should be investigated or refactored

The getting started guide in the Cobra documentations demos the library by using the init() function to register the command and configure flags. Which has resulted in a lot of projects following this pattern for their own code. The subsequent examples in the documentation does not use this pattern.

Error propagation becomes a large challenge when using functions init() as errors cannot be returned and they are magically called when the module is loaded. Additionally testing can become a hassle. This has led to problems like #2756 because there is no easy path to deal with these problems. As the functions are called automatically there is no easy way to pass function parameters, which instead led to using global variables to pass critical information. This includes things like command flags, embedded file systems, and build flags. Using such a pattern creates loose coupling between where the data is set and consumed.

My suggestion is that we refactor the CLI code base a bit to remove any use of the init() function. This would mean following a pattern similar to what has been suggested in this issue.

spf13/cobra#1041 (comment)

To make things clean we could have a single function for each subcommand/package which would then be responsible for calling and configuring commands downstream. This way we could pass any dependencies down from main to the subcommand which needs it.

Links to any relevant code

zarf/main.go

Lines 43 to 44 in 81f1c70

	config.CosignPublicKey = cosignPublicKey
	lint.ZarfSchema = zarfSchema

zarf/src/cmd/initialize.go

Lines 179 to 228 in 81f1c70

	func init() {
	v := common.InitViper()
	rootCmd.AddCommand(initCmd)
	// Init package variable defaults that are non-zero values
	// NOTE: these are not in common.setDefaults so that zarf tools update-creds does not erroneously update values back to the default
	v.SetDefault(common.VInitGitPushUser, types.ZarfGitPushUser)
	v.SetDefault(common.VInitRegistryPushUser, types.ZarfRegistryPushUser)
	// Init package set variable flags
	initCmd.Flags().StringToStringVar(&pkgConfig.PkgOpts.SetVariables, "set", v.GetStringMapString(common.VPkgDeploySet), lang.CmdInitFlagSet)
	// Continue to require --confirm flag for init command to avoid accidental deployments
	initCmd.Flags().BoolVar(&config.CommonOptions.Confirm, "confirm", false, lang.CmdInitFlagConfirm)
	initCmd.Flags().StringVar(&pkgConfig.PkgOpts.OptionalComponents, "components", v.GetString(common.VInitComponents), lang.CmdInitFlagComponents)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.StorageClass, "storage-class", v.GetString(common.VInitStorageClass), lang.CmdInitFlagStorageClass)
	// Flags for using an external Git server
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.GitServer.Address, "git-url", v.GetString(common.VInitGitURL), lang.CmdInitFlagGitURL)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.GitServer.PushUsername, "git-push-username", v.GetString(common.VInitGitPushUser), lang.CmdInitFlagGitPushUser)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.GitServer.PushPassword, "git-push-password", v.GetString(common.VInitGitPushPass), lang.CmdInitFlagGitPushPass)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.GitServer.PullUsername, "git-pull-username", v.GetString(common.VInitGitPullUser), lang.CmdInitFlagGitPullUser)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.GitServer.PullPassword, "git-pull-password", v.GetString(common.VInitGitPullPass), lang.CmdInitFlagGitPullPass)
	// Flags for using an external registry
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.RegistryInfo.Address, "registry-url", v.GetString(common.VInitRegistryURL), lang.CmdInitFlagRegURL)
	initCmd.Flags().IntVar(&pkgConfig.InitOpts.RegistryInfo.NodePort, "nodeport", v.GetInt(common.VInitRegistryNodeport), lang.CmdInitFlagRegNodePort)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.RegistryInfo.PushUsername, "registry-push-username", v.GetString(common.VInitRegistryPushUser), lang.CmdInitFlagRegPushUser)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.RegistryInfo.PushPassword, "registry-push-password", v.GetString(common.VInitRegistryPushPass), lang.CmdInitFlagRegPushPass)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.RegistryInfo.PullUsername, "registry-pull-username", v.GetString(common.VInitRegistryPullUser), lang.CmdInitFlagRegPullUser)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.RegistryInfo.PullPassword, "registry-pull-password", v.GetString(common.VInitRegistryPullPass), lang.CmdInitFlagRegPullPass)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.RegistryInfo.Secret, "registry-secret", v.GetString(common.VInitRegistrySecret), lang.CmdInitFlagRegSecret)
	// Flags for using an external artifact server
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.ArtifactServer.Address, "artifact-url", v.GetString(common.VInitArtifactURL), lang.CmdInitFlagArtifactURL)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.ArtifactServer.PushUsername, "artifact-push-username", v.GetString(common.VInitArtifactPushUser), lang.CmdInitFlagArtifactPushUser)
	initCmd.Flags().StringVar(&pkgConfig.InitOpts.ArtifactServer.PushToken, "artifact-push-token", v.GetString(common.VInitArtifactPushToken), lang.CmdInitFlagArtifactPushToken)
	// Flags that control how a deployment proceeds
	// Always require adopt-existing-resources flag (no viper)
	initCmd.Flags().BoolVar(&pkgConfig.DeployOpts.AdoptExistingResources, "adopt-existing-resources", false, lang.CmdPackageDeployFlagAdoptExistingResources)
	initCmd.Flags().BoolVar(&pkgConfig.DeployOpts.SkipWebhooks, "skip-webhooks", v.GetBool(common.VPkgDeploySkipWebhooks), lang.CmdPackageDeployFlagSkipWebhooks)
	initCmd.Flags().DurationVar(&pkgConfig.DeployOpts.Timeout, "timeout", v.GetDuration(common.VPkgDeployTimeout), lang.CmdPackageDeployFlagTimeout)
	initCmd.Flags().IntVar(&pkgConfig.PkgOpts.Retries, "retries", v.GetInt(common.VPkgRetries), lang.CmdPackageFlagRetries)
	initCmd.Flags().StringVarP(&pkgConfig.PkgOpts.PublicKeyPath, "key", "k", v.GetString(common.VPkgPublicKey), lang.CmdPackageFlagFlagPublicKey)
	initCmd.Flags().SortFlags = true
	}

Additional context

N/A

[mkcp]

I wrote up a similar issue #3183 that ended up being a duplicate. Copying the context over to here, cause there's code links and an example in zarf say

Describe what should be investigated or refactored

Right now generate most of Zarf's CLI commands and flags inside of cmd pkg inits. While these inits() do have a reliable order, the order is not at all obvious when reading the code. We can make the code more maintainable by being explicit about when and how we generate commands and flags. Let's move this functionality into root and call the generate functions linearly, rather than in a bunch of init()s.

One pain point in this migration is that the logical commands in Zarf, e.g. zarf package ..., are each grouped by file names but are all in the cmd pkg. The many init functions help group and split these up, so we take that pain back on by moving them together. Taking additional time to move commands into their own subpackages, e.g. cmd/dev will help avoid overpopulating cmd with implementation details of specific commands.

Links to any relevant code

Connect: https://github.com/zarf-dev/zarf/blob/main/src/cmd/connect.go#L106-L116
Destroy: https://github.com/zarf-dev/zarf/blob/main/src/cmd/destroy.go#L106-L113
Dev: https://github.com/zarf-dev/zarf/blob/main/src/cmd/dev.go#L332-L376
Zarf init: https://github.com/zarf-dev/zarf/blob/main/src/cmd/initialize.go#L181-L230
Internal: https://github.com/zarf-dev/zarf/blob/main/src/cmd/internal.go#L370-L385
Package: https://github.com/zarf-dev/zarf/blob/main/src/cmd/package.go#L464-L485
Root: https://github.com/zarf-dev/zarf/blob/main/src/cmd/root.go#L151-L182
Version: https://github.com/zarf-dev/zarf/blob/main/src/cmd/version.go#L85-L88

Additional context

Zarf Say https://github.com/zarf-dev/zarf/blob/main/src/cmd/say/say.go is an example of how we'd contain the complexity of a specific command to one package, then declare that command explicitly in root.