Skip to content

Move dbconfigs to pflag and remove deprecated flags#11336

Merged
mattlord merged 7 commits intovitessio:mainfrom
planetscale:dbconfigs_pflags
Sep 30, 2022
Merged

Move dbconfigs to pflag and remove deprecated flags#11336
mattlord merged 7 commits intovitessio:mainfrom
planetscale:dbconfigs_pflags

Conversation

@mattlord
Copy link
Copy Markdown
Member

@mattlord mattlord commented Sep 23, 2022 •
edited
Loading

Description

This migrates the dbconfig package flags from go flag to pflag. It also takes the opportunity to finally remove all of the long deprecated flags (first deprecated in 3.0).

Related Issue(s)

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

@mattlord mattlord added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving labels Sep 23, 2022
@vitess-bot
Copy link
Copy Markdown
Contributor

vitess-bot bot commented Sep 23, 2022 •
edited
Loading

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.

@mattlord mattlord added Component: General Changes throughout the code base and removed Component: Query Serving labels Sep 23, 2022
@mattlord mattlord changed the title Move dbconfigs to pflag to remove deprecated flags Move dbconfigs to pflag and remove deprecated flags Sep 25, 2022
@mattlord
Migrate dbconfigs to pflag
cfefc7d 
And remove deprecated flags

Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord
Remove no longer needed function
6ada721 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord mattlord marked this pull request as ready for review September 29, 2022 19:25
ajm188
ajm188 approved these changes Sep 29, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@ajm188 ajm188 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great!!! awesome stuff ❤️

go/vt/vttls/vttls.go
// Type returns the value type, part of the pflag Value interface
// for allowing this to be used as a generic flag.
func (mode *SslMode) Type() string {
return "SslMode"
Copy link
Copy Markdown
Contributor

@ajm188 ajm188 Sep 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo this should return "string" but idk what the best-practice for pflag is here see for example how StringSliceVar shows up as "strings" in the help output

Copy link
Copy Markdown
Member Author

@mattlord mattlord Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I thought too, that's what I started with but then had failures. This was only needed because SslMode is an alias for string.

Copy link
Copy Markdown
Contributor

@ajm188 ajm188 Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no no, i mean the value returned by this function should be the literal string "string", not the string "SslMode"

Copy link
Copy Markdown
Member Author

@mattlord mattlord Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I thought you meant. It's also what I meant. 😄

Copy link
Copy Markdown
Contributor

@ajm188 ajm188 Sep 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh weird! TIL

@mattlord
Address review comments and test failures
a619749 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord
Revert this bit as it changed behavior and caused some failures
3c47aa3 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord
Fix vttestserver unit tests
097c6ab 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord
Fix vttestserver unit tests
b7e81d1 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@mattlord
Update vttestserver help output
85d95f3 
Signed-off-by: Matt Lord <mattalord@gmail.com>
@deepthi deepthi added this to the v15.0 milestone Sep 30, 2022
@mattlord mattlord merged commit c82dd87 into vitessio:main Sep 30, 2022
@mattlord mattlord deleted the dbconfigs_pflags branch September 30, 2022 21:14
@GuptaManan100 GuptaManan100 mentioned this pull request Oct 1, 2022
Merged
@mattlord mattlord mentioned this pull request Oct 26, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rohit-nayak-ps rohit-nayak-ps Awaiting requested review from rohit-nayak-ps

@deepthi deepthi Awaiting requested review from deepthi

@GuptaManan100 GuptaManan100 Awaiting requested review from GuptaManan100

1 more reviewer

@ajm188 ajm188 ajm188 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

v15.0

Development

Successfully merging this pull request may close these issues.

Switch flag definitions to be on pflag instead of flag in package go/vt/dbconfigs

3 participants

@mattlord @ajm188 @deepthi