Skip to content

[VEP-4, phase 3] Switch flag definitions to be on pflag instead of flag and add make them package private. #10697

New issue
New issue
Closed
Closed
[VEP-4, phase 3] Switch flag definitions to be on pflag instead of flag and add make them package private.#10697
Labels
Component: CLIComponent: GeneralChanges throughout the code baseType: EnhancementLogical improvement (somewhere between a bug and feature)Type: Internal Cleanup
Milestone
 
v15.0
@ajm188

Description

@ajm188
ajm188
opened on Jul 14, 2022

This is the general, and tracking, issue for VEP-4, phase 3, in which flags currently defined on the global flag.CommandLine in packages throughout vitess are moved simultaneously to a pflag.FlagSet and out of the global namespace (and therefore, out of the usage text for commands that don't consume those flags at all).

Table of Contents

Packages to move #

This issue is considered done when each of these is complete

(I am slowly filing issues for each of these packages, please bear with me!)

N.B. this list was generated with the following command git grep -E "\bflag\.[A-Z]" -- go/ | cut -d':' -f1 | xargs -I{} dirname {} | uniq and then applying some light filtering (e.g. I know that vtadmin is fine, and I don't care about wrangler/testlib). If you disagree or have questions about a package, please don't hesitate to ask!!

Full command with filtering I used 
git grep -E "\bflag\.[A-Z]" -- go/ | cut -d':' -f1 | xargs -I{} dirname {} | sort | uniq | grep -Ev '(testlib|vtadmin|endtoend)' | xargs -I{} echo "- [ ] \`{}\`"

How to move a package's flags out of the global flagset — The Guide #

For each flag in the package you are tackling, you are going to want to determine if it truly should be global and available to every binary (probably flags that control logging and tracing behavior are good candidates), or if they only make sense for a particular binary or particular subset of binaries.

To see which cmd entrypoints actually import your package at all (which doesn't actually tell you if the value that flag is set to has any impact on that binary), you can run:

go list -f '{{ .ImportPath }}| {{ join .Deps " " }} ' ./go/cmd/... | awk -F"|" '$2 ~ " <full import path of your package here> " {print $1}'

The spacing in the -f argument to go list might seem arbitrary, but it's specific and important here, as it lets us do an exact match in the awk (e.g. you want to know if vitess.io/vitess/go/vt/vtgate is imported, but not get false positives if someone imports vitess.io/vitess/go/vt/vtgate/buffer, or anything else underneath the go/vt/vtgate/* tree).

As an example, here are the cmds that import grpcvtgateservice:

$ go list -f '{{ .ImportPath }}| {{ join .Deps " " }} ' ./go/cmd/... | awk -F"|" '$2 ~ " vitess.io/vitess/go/vt/vtgate/grpcvtgateservice " {print $1}'
vitess.io/vitess/go/cmd/vtcombo
vitess.io/vitess/go/cmd/vtgate
vitess.io/vitess/go/cmd/vtgateclienttest

Armed with this list, you need to make a judgement call as to whether each of these commands actually needs the flags defined in your package. I don't have much to say in the way of guidance here, except to ask the domain experts of the package and the cmd entrypoint. This is also why we have endtoend tests, and release candidates :)

Once you've decided, you can move your flags to a function (RegisterFlags is a good name, as is InstallFlags, which takes a *pflag.FlagSet as input), for example:

var myCoolFlag int

- flag.IntVar(&myCoolFlag, "my-cool-flag", 0, "this is my cool flag, but it's in the global flagset")
+ func RegisterFlags(fs *pflag.FlagSet) {
+     fs.IntVar(&myCoolFlag, "my-cool-flag", 0, "this is my cool flag, and now it's only installed on this one flagset")
+ }

Now that you've done that, you'll hook into servenv's new hook points to ensure your flag gets installed in the right places. If you decided your flag should truly be global, you will do:

func init() {
    servenv.OnParse(RegisterFlags)
}

If you've decided your flag belongs only on one or two commands, you'll do:

func init() {
    servenv.OnParseFor("vttablet", RegisterFlags)
    servenv.OnParseFor("vtgate", RegisterFlags) // this is also why I suggest writing a function rather than inlining
}

Metadata

Metadata

Assignees

No one assigned

    Labels

    Component: CLIComponent: GeneralChanges throughout the code baseType: EnhancementLogical improvement (somewhere between a bug and feature)Type: Internal Cleanup

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions