[cli] [vtctl] Migrate all vtctl commands to pflag

[ajm188]

Description

See the lengthy comment in go/cmd/vtctl/vtctl.go's init function for the bulk of the explanation of what's happening here, but this mostly works.

The one thing that seems to be a potential problem is commands that take shard names or ranges as positional arguments, if and only if those names begin with a dash (e.g. "-80,80-"), because pflag sees the dash and tries to find a shorthand option with that name (leading to fun errors like unknown shorthand flag: '8' in -80,80-), but putting a double-dash to stop parsing works (and is something we've been pushing users toward so I feel okay about it, but let me know if there's a strong disagreement.

Related Issue(s)

Closes #11304.

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]

I think this is the only thing up for debate.
Looks like it affects both v1 and v2 versions of the VRepl workflows.
@mattlord what are your thoughts?
@ajm188 do we have any alternatives besides requiring people to provide the additional --?

[deepthi]

Something we'll need to note in the release notes, so I've added that label.

[rohit-nayak-ps]

It doesn't affect v2 workflows because the shards are passed as flags like vtctlclient Reshard -- --source_shards '0' --target_shards '-80,80-' Create customer.cust2cust where as in v1 workflows the shards are passed as Args like Reshard -- --v1 --cells=zone1 --tablet_types=replica,primary merchant-type.m2m3 -80,80- -40,40-c0,c0-

One possibility is to require quoting the shard arguments to the v1 workflows. That will require a trivial code change, however it will still make it backwardly incompatible. Since we have deprecated v1 workflows, the approach in this PR might be acceptable.

[ajm188]

One possibility is to require quoting the shard arguments to the v1 workflows.

this doesn't work (i tried it)

$ go run ./go/cmd/pflagtester -80,80-
unknown shorthand flag: '8' in -80,80-
Usage of /var/folders/8q/6w7llb2j03v454_4pcm68w7w0000gn/T/go-build3458657591/b001/exe/pflagtester:
      --foo int    (default 2)
unknown shorthand flag: '8' in -80,80-
exit status 2
$ go run ./go/cmd/pflagtester "-80,80-"
unknown shorthand flag: '8' in -80,80-
Usage of /var/folders/8q/6w7llb2j03v454_4pcm68w7w0000gn/T/go-build4228639101/b001/exe/pflagtester:
      --foo int    (default 2)
unknown shorthand flag: '8' in -80,80-
exit status 2

[ajm188]

rohit is correct thought that this only affects v1 workflows

[rohit-nayak-ps]

One possibility is to require quoting the shard arguments to the v1 workflows. That will require a trivial code change, however it will still make it backwardly incompatible. Since we have deprecated v1 workflows, the approach in this PR might be acceptable.

I mean't after the code change referred to above, we can support quotes. But it just replaces one incompatibility with another ...

[ajm188]

ahh, gotcha gotcha, sorry i misunderstood!

[ajm188]

@deepthi a683a87 i also removed the double-dashes from the VDiff notes since those are no longer required (cc @mattlord)

[rohit-nayak-ps]

Did you intend to leave the commented line? There is one more below.

[ajm188]

ohhh weird, no i didn't!

[rohit-nayak-ps]

lgtm

[mattlord]

In total, I think we've made the double dash usage required to ensure correct behavior as part of this work (e.g. sub-command --help). IMO, we should make a more general announcement/summary that the double dash usage is required (even though in some cases things will work fine / the same w/o it).

[ajm188]

i'm adding a deprecation notice to vtctl.RunCommand that i hope will help.

[mattlord]

Why remove the double dashes? Compare the output of these two things for an example of why I think we should always use them and why I feel the way I do about noting that it's (virtually) required now in the release notes:

$ vtctlclient --server=localhost:15999 VDiff --help
$ vtctlclient --server=localhost:15999 VDiff -- --help

This same comment applies to all of the other places we remove them in the PR.

[ajm188]

because that gets in the way of moving to cobra (cobra does one full-blown parse by merging together the parent/subcommand flag-sets, so if you have the -- after the sub-command name then all the sub-command flags get treated as positional arguments instead of flags)

[mattlord]

Nota Bene? 🏅 😉

[mattlord]

Ah, I guess this is why you removed the double dashes? I still think we should leave them as the help output is still different, and it would be nice to have some uniformity across the client commands/binaries. Unless I'm misunderstanding something?

[ajm188]

#11320 (comment)

[mattlord]

We don't need both double dashes do we? Is the first one not enough?

[ajm188]

#11320 (comment) sort of explains it. the double-dash breaks further parsing, so everything after it (--v1, in this case) because a positional argument. so you want this after your last "true" flag

[mattlord]

IF the double dash after Reshard also works I'd much prefer that as we've been pushing people in that direction for a while.

[ajm188]

#11320 (comment)

[ajm188]

so, new problem (maybe)

i think in order to get the vtctld_web_test to pass, i need to apply the following diff:

diff --git a/web/vtctld2/src/app/shared/flags/flag.ts b/web/vtctld2/src/app/shared/flags/flag.ts
index d8fc03d023..15107e001a 100644
--- a/web/vtctld2/src/app/shared/flags/flag.ts
+++ b/web/vtctld2/src/app/shared/flags/flag.ts
@@ -110,7 +110,7 @@ export class Flag {
     if (this.getValue() === true) {
       return [`-${this.id}`];
     }
-    return [`-${this.id}=${this.getStrValue()}`];
+    return [`--${this.id}=${this.getStrValue()}`];
   }
 }

This creates at least one problem:

1. I don't know how to rebuild/regenerate the vtctld2 bundle (make web_build is failing for me, something about sass i think but i didn't look too much at the output yet because ......... 👇 )
2. this is sort of a breaking change? but at the same time, vtadmin is going GA, and vtctld2 is deprecated, so maybe it's fine? (with an associated release notes callout)

@deepthi what do you think?

[deepthi]

I don't know how to rebuild/regenerate the vtctld2 bundle (make web_build is failing for me, something about sass i think but i didn't look too much at the output yet because ......... 👇 )

yes, we will need to rebuild/regen that. we can ask @notfelineit to help, she had the build working at some point.

this is sort of a breaking change? but at the same time, vtadmin is going GA, and vtctld2 is deprecated, so maybe it's fine? (with an associated release notes callout)

Should be fine

[deepthi]

@ajm188 I suspect more changes are needed in flag.ts

    if (this.getStrValue() === '' || !this.positional) {
      return [];
    }
    if (this.namedPositional !== undefined) {
      if (this.getValue() === true) {
        return [`-${this.namedPositional}`];
      }
      // Named positional arguments are used for workflow creation, mainly.
      return [`-${this.namedPositional}=${this.getStrValue()}`];
    }
    // Positional arguments only need a value not a key.
    return [this.getStrValue()];
  }

  public getFlags() {
    if (this.getStrValue() === '' || this.positional) {
      return [];
    }
    // Non-positional arguments need a key value pair.
    if (this.getValue() === true) {
      return [`-${this.id}`];
    }
    return [`--${this.id}=${this.getStrValue()}`];
  }

That's 3 more places where we have - vs --.

[ajm188]

yeah, i just went ahead and double-dashed everything. i also tried all morning to get make web_bootstrap web_build working, but i think it might be impossible on an m1 (i need an older python to get around a node-gyp build dependency error that starts in 3.9, but then brew won't install older pythons, but then you can install an x86 brew with rosetta, but then node-gyp starts looking for python2, which i don't even know if i can install anymore, and i just gave up at that point)

@notfelineit when you get a chance can you re-run the web build? (not urgent ❤️ )

[ajm188]

note the failing test will pass after we re-build the frontend bundle:

=== RUN   TestDashboardValidate
    vtctld_web_main_test.go:481: 
        	Error Trace:	vtctld_web_main_test.go:481
        	            				vtctld_web_test.go:205
        	Error:      	elements differ
        	            	
        	            	extra elements in list A:
        	            	([]interface {}) (len=1) {
        	            	 (string) (len=14) "--ping-tablets"
        	            	}
        	            	
        	            	
        	            	extra elements in list B:
        	            	([]interface {}) (len=1) {
        	            	 (string) (len=13) "-ping-tablets"
        	            	}
        	            	
        	            	
        	            	listA:
        	            	([]string) (len=2) {
        	            	 (string) (len=8) "Validate",
        	            	 (string) (len=14) "--ping-tablets"
        	            	}
        	            	
        	            	
        	            	listB:
        	            	([]string) (len=2) {
        	            	 (string) (len=8) "Validate",
        	            	 (string) (len=13) "-ping-tablets"
        	            	}
        	Test:       	TestDashboardValidate
Validate command response: There was a problem validating all nodes: unknown shorthand flag: 'p' in -ping-tablets