[VEP-4, phase 1] Flag Deprecation Warnings #9733

ajm188 · 2022-02-18T18:42:02Z

Description

This PR introduces a helper function, _flag.Parse to check for the flag usages that will break when we switch to flag. For more details, see VEP-4.

The general plan here is:

Merge this. If I'm fast enough, get a second branch into 13.0. If not, adjust all versions in the next steps by 1.
Adjust scripts in examples/ directory so they stop issuing warnings.
Drop-in replace all calls to _flag.Parse with pflag.Parse. Verify all binaries start correctly.
Delete this package.

Testing:

Here's the local example:

➜  local git:(andrew/flag-deprecation) ✗ ./101_initial_cluster.sh 
add /vitess/global
add /vitess/zone1
add zone1 CellInfo
W0217 09:40:27.230091   62398 flag.go:30] Use of single-dash long flags is deprecated and will be removed in the next version of Vitess. Please use --topo_global_root instead
W0217 09:40:27.230545   62398 flag.go:30] Use of single-dash long flags is deprecated and will be removed in the next version of Vitess. Please use --topo_global_server_address instead
W0217 09:40:27.230549   62398 flag.go:30] Use of single-dash long flags is deprecated and will be removed in the next version of Vitess. Please use --topo_implementation instead
W0217 09:40:27.230552   62398 flag.go:43] Detected a positional argument beginning with a dash; This will be treated as a flag argument in the next version of Vitess. Please update your invocation to include a "--" before to continue treating --root as a positional argument.
W0217 09:40:27.230567   62398 flag.go:43] Detected a positional argument beginning with a dash; This will be treated as a flag argument in the next version of Vitess. Please update your invocation to include a "--" before to continue treating --server-address as a positional argument.
W0217 09:40:27.230578   62398 vtctl.go:88] WARNING: vtctl should only be used for VDiff workflows. Consider using vtctldclient for all other commands.
Created cell: zone1

Related Issue(s)

vitessio/enhancements#7

Checklist

Should this PR be backported? yes, opening a branch now
Tests were added or are not required n/a -- did manual testing
Documentation was added or is not required

Deployment Notes

ajm188 · 2022-02-20T14:37:08Z

go/internal/flag/flag.go

+
+			// Second line: usage and optional default, if not the zero value
+			// for the type.
+			buf.WriteString(usage)


If you, like me, were wondering, but what if the usage is empty? does the standard library really still print a tab character? oh boy does it!!

(playground)

haha good catch

ajm188 · 2022-02-20T14:37:53Z

go/internal/flag/flag.go

+		}
+
+		if strings.HasPrefix(arg, "-") {
+			log.Warningf("Detected a positional argument beginning with a dash; this will be treated as a flag argument in the next version of Vitess. Please update your invocation to include a \"--\" before to continue treating %s as a positional argument.", arg)


@deepthi let me know if you have suggestions on how to improve this warning text. i don't think it's clear, but i can't think of a way to improve it, either :/

I might be overthinking, but to make it clearer I would add:
include a \"--\" before to ... -> include a \"--\" before %s to ... with %s being arg

I like this suggestion 👍

ajm188 · 2022-02-20T14:40:12Z

go/internal/flag/flag.go

+		var buf strings.Builder
+		goflag.CommandLine.VisitAll(func(f *goflag.Flag) {
+			defer buf.Reset()
+			defer func() { fmt.Fprintf(goflag.CommandLine.Output(), "%s\n", buf.String()) }()


we have to wrap this defer inside an anonymous function to delay the execution of buf.String()

the other thing i briefly considered was:

defer buf.Reset() ... if bf, ok := f.Value.(maybeBoolFlag); ok && ... { fmt.Fprintf(...) goto print } // print first and second lines for non-bool flags to `buf` print: fmt.Fprintf(goflag.CommandLine.Output(), "%s\n", buf.String())

but ended up deciding the goto was more gross than the defer func() { ... }()

IMO, the defer does not seem bad. Another alternative would be to create an anonymous function where we have the current defer, and call that function where we return.

frouioui

I am excited for this VEP, phase 1 is looking good to me @ajm188 🚀

We should update the v14/v13 docs to reflect this deprecation and how the new flags should be written.

ajm188 · 2022-02-22T19:16:02Z

Ugh, okay this is breaking a lot of the cluster tests that check the "first line" output of vtctl because those first few lines are now all deprecation warnings ... I'm going to fix up all their invocations and see if that helps without breaking upgrade/downgrade testing.

ajm188 · 2022-02-22T19:23:32Z

🤞 🤞

deepthi

We should change all the examples in the same PR. Otherwise the output will be full of warnings and difficult to parse by humans (unless you plan to fast-follow with an examples PR and want to keep PRs small and manageable).

ajm188 · 2022-02-22T20:17:15Z

I'm gonna DoNotMerge this, since I think there's a few more things to do, in particular:

update all the tests to not break
~~update the example scripts~~ i changed my mind, let's do this in a follow-up PR
write the release notes

when i finish those 3 things i'll un-DoNotMerge and ping you again. sound good?

This package is internal, and extremely temporary. After the necessary backwards-compatability deprecation cycle to switch to `pflag`, will be removed. Signed-off-by: Andrew Mason <andrew@planetscale.com>

…ion-aware parser Signed-off-by: Andrew Mason <andrew@planetscale.com>

Signed-off-by: Andrew Mason <andrew@planetscale.com>

…r structure Signed-off-by: Andrew Mason <andrew@planetscale.com>

Signed-off-by: Andrew Mason <andrew@planetscale.com>

…ds compatibility Signed-off-by: Andrew Mason <andrew@planetscale.com>

Signed-off-by: Andrew Mason <andrew@planetscale.com>

ajm188 · 2022-03-08T16:31:39Z

Alright, hold on to your butts, I'm force-pushing with what will, pending some back-and-forth on the release notes, be the final version of this change. I did all my debugging over in #9831, which I will close.

updated all the things, this approval should in no way count (but thank you!!)

…= v13.0.0 Signed-off-by: Andrew Mason <andrew@planetscale.com>

Signed-off-by: Andrew Mason <andrew@planetscale.com>

deepthi

Nice work!

ajm188 added Type: Enhancement Logical improvement (somewhere between a bug and feature) release notes (needs details) This PR needs to be listed in the release notes in a dedicated section (deprecation notice, etc...) Component: General Changes throughout the code base labels Feb 18, 2022

ajm188 added this to In progress in Flag Restructure via automation Feb 18, 2022

ajm188 requested a review from deepthi as a code owner February 18, 2022 18:42

ajm188 requested a review from derekperkins February 18, 2022 18:42

ajm188 mentioned this pull request Feb 18, 2022

[backport] Flag Deprecation (13.0) #9734

Closed

ajm188 commented Feb 20, 2022

View reviewed changes

frouioui previously approved these changes Feb 21, 2022

View reviewed changes

Flag Restructure automation moved this from In progress to Reviewer approved Feb 21, 2022

ajm188 force-pushed the andrew/flag-deprecation branch from 809636e to 7681a70 Compare February 22, 2022 14:31

ajm188 force-pushed the andrew/flag-deprecation branch from e506b22 to dad50aa Compare February 22, 2022 19:23

deepthi reviewed Feb 22, 2022

View reviewed changes

ajm188 added the Do Not Merge label Feb 22, 2022

ajm188 force-pushed the andrew/flag-deprecation branch 5 times, most recently from 50fdd45 to f76a451 Compare February 23, 2022 22:17

ajm188 mentioned this pull request Mar 6, 2022

Online DDL: cleanup old vtctld+topo flow #9816

Merged

3 tasks

Andrew Mason added 7 commits March 8, 2022 06:16

Add helper function to issue deprecation warnings when parsing flags

9cab4aa

This package is internal, and extremely temporary. After the necessary backwards-compatability deprecation cycle to switch to `pflag`, will be removed. Signed-off-by: Andrew Mason <andrew@planetscale.com>

Update cmd callsites that don't parse via servenv to use deprecat…

788131e

…ion-aware parser Signed-off-by: Andrew Mason <andrew@planetscale.com>

Override Usage func to show flags with double-dashes in -h output

b2216e8

Signed-off-by: Andrew Mason <andrew@planetscale.com>

Extract and rework the deprecation warning text, plus a little cleane…

56b9bf6

…r structure Signed-off-by: Andrew Mason <andrew@planetscale.com>

Fixup import ordering for context while I'm here

094fae9

Signed-off-by: Andrew Mason <andrew@planetscale.com>

Handle positional arguments to filter out the double-dash for backwar…

c907556

…ds compatibility Signed-off-by: Andrew Mason <andrew@planetscale.com>

Refactor to support subflag usages and custom usage funcs

21d3647

Signed-off-by: Andrew Mason <andrew@planetscale.com>

Andrew Mason added 5 commits March 8, 2022 06:16

Add ability to filter out certain flags from the Usage

1a94ad0

Signed-off-by: Andrew Mason <andrew@planetscale.com>

Documentation for SetUsage nonsense

35a0d0d

Signed-off-by: Andrew Mason <andrew@planetscale.com>

Override custom Usages, preserving intended effects

813e6bd

Signed-off-by: Andrew Mason <andrew@planetscale.com>

Special consideration for v1 reshard workflow cli

f348d64

Signed-off-by: Andrew Mason <andrew@planetscale.com>

Update flags in tests

7a84605

Signed-off-by: Andrew Mason <andrew@planetscale.com>

ajm188 force-pushed the andrew/flag-deprecation branch from c39304a to 43a688e Compare March 8, 2022 16:32

ajm188 requested review from doeg, notfelineit, harshit-gangal, systay and shlomi-noach as code owners March 8, 2022 16:32

ajm188 removed the Do Not Merge label Mar 8, 2022

frouioui self-requested a review March 8, 2022 16:36

Flag Restructure automation moved this from Reviewer approved to Review in progress Mar 8, 2022

ajm188 force-pushed the andrew/flag-deprecation branch from 43a688e to dad87fc Compare March 8, 2022 18:15

Andrew Mason added 2 commits March 8, 2022 13:52

Filter out double-dash separator when running vtctl/vtctlclient <…

c8a5bbe

…= v13.0.0 Signed-off-by: Andrew Mason <andrew@planetscale.com>

Add release notes for CLI deprecations with advice

20485b3

Signed-off-by: Andrew Mason <andrew@planetscale.com>

ajm188 force-pushed the andrew/flag-deprecation branch from b886d83 to 20485b3 Compare March 8, 2022 18:52

deepthi approved these changes Mar 8, 2022

View reviewed changes

Flag Restructure automation moved this from Review in progress to Reviewer approved Mar 8, 2022

deepthi merged commit 8fde81b into vitessio:main Mar 8, 2022

deepthi deleted the andrew/flag-deprecation branch March 8, 2022 22:27

Flag Restructure automation moved this from Reviewer approved to Done Mar 8, 2022

deepthi mentioned this pull request Mar 11, 2022

Examples: fix warnings #9875

Merged

3 tasks

GuptaManan100 mentioned this pull request May 31, 2022

Fix commands in readme files to use double dashes for arguments #10389

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VEP-4, phase 1] Flag Deprecation Warnings #9733

[VEP-4, phase 1] Flag Deprecation Warnings #9733

ajm188 commented Feb 18, 2022

ajm188 Feb 20, 2022

frouioui Feb 21, 2022

ajm188 Feb 20, 2022

frouioui Feb 21, 2022

deepthi Feb 22, 2022

ajm188 Feb 20, 2022

frouioui Feb 21, 2022

frouioui left a comment

ajm188 commented Feb 22, 2022

ajm188 commented Feb 22, 2022

deepthi left a comment

ajm188 commented Feb 22, 2022 •

edited

ajm188 commented Mar 8, 2022

deepthi left a comment

[VEP-4, phase 1] Flag Deprecation Warnings #9733

[VEP-4, phase 1] Flag Deprecation Warnings #9733

Conversation

ajm188 commented Feb 18, 2022

Description

Testing:

Related Issue(s)

Checklist

Deployment Notes

ajm188 Feb 20, 2022

Choose a reason for hiding this comment

frouioui Feb 21, 2022

Choose a reason for hiding this comment

ajm188 Feb 20, 2022

Choose a reason for hiding this comment

frouioui Feb 21, 2022

Choose a reason for hiding this comment

deepthi Feb 22, 2022

Choose a reason for hiding this comment

ajm188 Feb 20, 2022

Choose a reason for hiding this comment

frouioui Feb 21, 2022

Choose a reason for hiding this comment

frouioui left a comment

Choose a reason for hiding this comment

ajm188 commented Feb 22, 2022

ajm188 commented Feb 22, 2022

deepthi left a comment

Choose a reason for hiding this comment

ajm188 commented Feb 22, 2022 • edited

ajm188 commented Mar 8, 2022

deepthi left a comment

Choose a reason for hiding this comment

ajm188 commented Feb 22, 2022 •

edited