Skip to content

Allow version to be accessible via the -v shorthand#11512

Merged
deepthi merged 3 commits intovitessio:mainfrom
planetscale:andrew/version-shorthand
Oct 17, 2022
Merged

Allow version to be accessible via the -v shorthand#11512
deepthi merged 3 commits intovitessio:mainfrom
planetscale:andrew/version-shorthand

Conversation

@ajm188
Copy link
Copy Markdown
Contributor

@ajm188 ajm188 commented Oct 15, 2022

Description

Before/after adding my absurdly "I am very annoyed by computers right now" lengthily-named function:

➜  vitess git:(vmain) ✗ vttablet -v
unable to redefine 'v' shorthand in "vttablet" flagset: it's already used for "version" flagpanic: unable to redefine 'v' shorthand in "vttablet" flagset: it's already used for "version" flag

goroutine 1 [running]:
github.com/spf13/pflag.(*FlagSet).AddFlag(0x140001ae100, 0x1400099a000)
        github.com/spf13/pflag@v1.0.5/flag.go:874 +0x3a8
github.com/spf13/pflag.(*FlagSet).AddGoFlag(0x140001ae100, 0x140000c62c0)
        github.com/spf13/pflag@v1.0.5/golangflag.go:90 +0x80
github.com/spf13/pflag.(*FlagSet).AddGoFlagSet.func1(0x140000acde0?)
        github.com/spf13/pflag@v1.0.5/golangflag.go:99 +0x2c
flag.(*FlagSet).VisitAll(0xf?, 0x140008bfd08)
        flag/flag.go:394 +0x50
github.com/spf13/pflag.(*FlagSet).AddGoFlagSet(0x140001ae100, 0x140000c4300)
        github.com/spf13/pflag@v1.0.5/golangflag.go:98 +0x48
vitess.io/vitess/go/internal/flag.Parse(0x140001ae100)
        vitess.io/vitess/go/internal/flag/flag.go:47 +0x38
vitess.io/vitess/go/vt/servenv.ParseFlags({0x105b071dc, 0x8})
        vitess.io/vitess/go/vt/servenv/servenv.go:310 +0x30
main.main()
        vitess.io/vitess/go/cmd/vttablet/vttablet.go:83 +0x50
➜  vitess git:(vmain) ✗ make
Fri Oct 14 20:10:52 EDT 2022: Building source tree
➜  vitess git:(vmain) ✗ vttablet -v
Version: 16.0.0-SNAPSHOT (Git revision b942dc3378f433278e4a4b22fb9bd86a0b63efd4 branch 'vmain') built on Fri Oct 14 20:10:52 EDT 2022 by andrew@<a computer> using go1.18.6 darwin/arm64

Related Issue(s)

rohit-nayak-ps/vreplication-docs#2 (comment)

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

Signed-off-by: Andrew Mason <andrew@planetscale.com>
@ajm188 ajm188 added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: General Changes throughout the code base Component: CLI Backport to: release-15.0 labels Oct 15, 2022
@ajm188 ajm188 added this to the v15.0 milestone Oct 15, 2022
@ajm188 ajm188 requested a review from deepthi as a code owner October 15, 2022 00:15
@vitess-bot
Copy link
Copy Markdown
Contributor

vitess-bot bot commented Oct 15, 2022

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:

  • Is it really necessary to add this flag?
  • Flag names should be clear and intuitive (as far as possible)
  • Help text should be descriptive.
  • Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow should be required, the maintainer team should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should include a link to an issue that describes the bug.

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.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from VTop, if used there.

Signed-off-by: Andrew Mason <andrew@planetscale.com>
@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Oct 15, 2022

Thank you!
Unit tests are failing

go.vt.vtadmin flag redefined: v
panic: go.vt.vtadmin flag redefined: v
goroutine 1 [running]:
github.com/spf13/pflag.(*FlagSet).AddFlag(0xc0000a5100, 0xc000133ea0)
	/home/runner/go/pkg/mod/github.com/spf13/pflag@v1.0.5/flag.go:848 +0x5e6
vitess.io/vitess/go/internal/flag.preventGlogVFlagFromClobberingVersionFlagShorthand(0xc00011c120?)
	/home/runner/work/vitess/vitess/go/internal/flag/flag.go:118 +0x85
vitess.io/vitess/go/internal/flag.ParseFlagsForTest()
	/home/runner/work/vitess/vitess/go/internal/flag/flag.go:161 +0x346
vitess.io/vitess/go/vt/vtadmin.TestMain(0xcccb11?)
	/home/runner/work/vitess/vitess/go/vt/vtadmin/api_test.go:63 +0x1e
main.main()
	_testmain.go:167 +0x1d3
FAIL	vitess.io/vitess/go/vt/vtadmin	0.024s

Signed-off-by: Andrew Mason <andrew@planetscale.com>
@ajm188
Copy link
Copy Markdown
Contributor Author

ajm188 commented Oct 16, 2022

Thank you! Unit tests are failing

go.vt.vtadmin flag redefined: v
panic: go.vt.vtadmin flag redefined: v
goroutine 1 [running]:
github.com/spf13/pflag.(*FlagSet).AddFlag(0xc0000a5100, 0xc000133ea0)
	/home/runner/go/pkg/mod/github.com/spf13/pflag@v1.0.5/flag.go:848 +0x5e6
vitess.io/vitess/go/internal/flag.preventGlogVFlagFromClobberingVersionFlagShorthand(0xc00011c120?)
	/home/runner/work/vitess/vitess/go/internal/flag/flag.go:118 +0x85
vitess.io/vitess/go/internal/flag.ParseFlagsForTest()
	/home/runner/work/vitess/vitess/go/internal/flag/flag.go:161 +0x346
vitess.io/vitess/go/vt/vtadmin.TestMain(0xcccb11?)
	/home/runner/work/vitess/vitess/go/vt/vtadmin/api_test.go:63 +0x1e
main.main()
	_testmain.go:167 +0x1d3
FAIL	vitess.io/vitess/go/vt/vtadmin	0.024s

ah, my shim is non-idempotent, which only matters in tests

(sorry, i meant to quote reply and I somehow edited your comment instead!)

@frouioui frouioui mentioned this pull request Oct 17, 2022
100 tasks
@deepthi
Copy link
Copy Markdown
Collaborator

deepthi commented Oct 17, 2022

Upgrade/downgrade tests don't depend on --version so overriding.
The failures are because we upgraded golang on release-15 to 1.18.7.

@deepthi deepthi merged commit abf6c55 into vitessio:main Oct 17, 2022
@deepthi deepthi deleted the andrew/version-shorthand branch October 17, 2022 17:28
@vitess-bot
Copy link
Copy Markdown
Contributor

vitess-bot bot commented Oct 17, 2022

I was unable to backport this Pull Request to the following branches: release-15.0.

@ajm188
Copy link
Copy Markdown
Contributor Author

ajm188 commented Oct 18, 2022

manually back porting this one, hold tight

ajm188 pushed a commit to planetscale/vitess that referenced this pull request Oct 18, 2022
* Allow version to be accessible via the -v shorthand

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

* update help text

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

* fix nonidempotence for tests

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

Signed-off-by: Andrew Mason <andrew@planetscale.com>
deepthi pushed a commit that referenced this pull request Oct 18, 2022
* Allow version to be accessible via the -v shorthand

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

* update help text

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

* fix nonidempotence for tests

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

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

Signed-off-by: Andrew Mason <andrew@planetscale.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: CLI Component: General Changes throughout the code base Type: Enhancement Logical improvement (somewhere between a bug and feature)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants