search

weaveworks / weave-gitops-enterprise

This repo provides the enterprise level features for the weave-gitops product, including CAPI cluster creation and team workspaces.
https://docs.gitops.weave.works/
Apache License 2.0
160 stars 29 forks source link

Stop using viper.GetString for flags #697

Open Skarlso opened 2 years ago

Skarlso commented 2 years ago 
    baseBranch := viper.GetString("capi-templates-repository-base-branch")
    if msg.BaseBranch != "" {
        baseBranch = msg.BaseBranch
    }

These viper.GetString calls make flags brittle and things like these:

cmd.Flags().StringVar(&p.capiTemplatesRepositoryBaseBranch, "capi-templates-repository-base-branch", "", "")

Will make this struct flag:

type Params struct {
...
     capiTemplatesRepositoryBaseBranch         string
...
}

Untraceable. One might think that this flag is not used, but the actual name of the flag can be found throughout the codebase for which the value is retrieved with GetString.

A better option is to pass around the actual Params and flags so we don't end up accidentally deleting a used flag. :)

bigkevmcd commented 2 years ago

To be fair, if the params and viper calls are leaving the cmd code, and leaking "throughout the codebase" we have other problems, that shouldn't happen either.

Can you link to a few examples?

Skarlso commented 2 years ago

So, now I understand that GetString is being used to it falls back to environment property in case the flag is not defined. However, the names should be a const so we don't mistype them and they are discoverable and don't have to copy paste them all over the codebase.

Examples are aplenty if you search for viper.GetString(". :) It's mostly in helpers.go and server/cluster.go which might in the process of being refactored?