[cmd/*] Switch to pflag for all CLI flag parsing

[ajm188]

Description

This PR begins the next step in the implementation of VEP-4, officially moving us to using pflag in all of our CLI flag parsing. It does not migrate any flag definitions (except one small proof-of-concept/example), or move any flags (at all) out of the global flagset, which can be done incrementally in follow-up PRs.

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

(if you think of a good place to house this, let me know!)

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
}

Related Issue(s)

• Restructure flag usage across vitess enhancements#7
• Closes [VEP-4, phase 2] Switch flag parsing in cmd entrypoints to use pflag  #10696

Checklist

• "Backport me!" label has been added if this change should be backported
• Tests were added or are not required
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
• If a new flag is being introduced, review whether it is really needed. The flag names should be clear and intuitive (as far as possible), and the flag's help should be descriptive. Additionally, flag names should use dashes (-) as word separators rather than underscores (_).
• If a workflow is added or modified, each items in Jobs should be named in order to mark it as required. If the workflow should be required, the GitHub Admin should be notified.

Bug fixes

• There should be at least one unit or end-to-end test.
• The Pull Request description should either include a link to an issue that describes the bug OR an actual description of the bug and how to reproduce, along with a description of the fix.

Non-trivial changes

• There should be some code comments as to why things are implemented the way they are.

New/Existing features

• Should be documented, either by modifying the existing documentation or creating new documentation.
• New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• vtctl command output order should be stable and awk-able.

[deepthi]

(if you think of a good place to house this, let me know!)

How about converting the TODOs in the tracking project to Issues?
Then the issue that this PR fixes (partially, I assume), can include a task list + the guide, and as we make progress we can check things off.

[ajm188]

(if you think of a good place to house this, let me know!)

How about converting the TODOs in the tracking project to Issues? Then the issue that this PR fixes (partially, I assume), can include a task list + the guide, and as we make progress we can check things off.

Let me know what you think!! #10697

[ajm188]

rebased to pick up the help text change here

[shlomi-noach]

👋 this PR seems to break vtctl/vtctlclient, who no longer accept any command-specific flags. For example:

$ vtctlclient ApplySchema --skip_preflight --ddl_strategy "online -allow-zero-in-date --allow-concurrent --postpone-completion" --sql "alter table corder engine=innodb" commerce
unknown flag: --skip_preflight

or

$ vtctlclient ApplySchema --ddl_strategy "online -allow-zero-in-date --allow-concurrent --postpone-completion" --sql "alter table corder engine=innodb" commerce
unknown flag: --ddl_strategy

or

$ vtctlclient ApplySchema --sql "alter table corder engine=innodb" commerce
unknown flag: --sql

I'm not sure how tests passed, as they ought to have captured this malfunction. At any case, right now main seems to be broken in this regard; we're going to revert this PR and re-examine it later on.

[ajm188]

(just closing the loop here) we discussed this in other fora, and the issue was a combination of being on different checkout and therefore needing the legacy -- as in v14 deprecation warnings.