-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Merge flags-refactor branch to main #18280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… use dashes (vitessio#17964) Signed-off-by: mounicasruthi <[email protected]> Signed-off-by: Mounica Sruthi <[email protected]> Signed-off-by: Rohit Nayak <[email protected]> Co-authored-by: Deepthi Sigireddi <[email protected]> Co-authored-by: Rohit Nayak <[email protected]>
Signed-off-by: mounicasruthi <[email protected]> Signed-off-by: Mounica Sruthi <[email protected]> Signed-off-by: Rohit Nayak <[email protected]> Co-authored-by: Deepthi Sigireddi <[email protected]> Co-authored-by: Rohit Nayak <[email protected]>
Signed-off-by: mounicasruthi <[email protected]>
Signed-off-by: mounicasruthi <[email protected]>
Signed-off-by: mounicasruthi <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18280 +/- ##
=======================================
Coverage 67.46% 67.47%
=======================================
Files 1602 1603 +1
Lines 262245 262329 +84
=======================================
+ Hits 176930 177010 +80
- Misses 85315 85319 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
deepthi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! LGTM.
The only thing we are missing right now is an announcement in the release notes. That should go into https://github.com/vitessio/vitess/blob/main/changelog/23.0/23.0.0/summary.md
Technically we don't have to block this PR merge on that though. It can be done in the follow up PRs that are already queued up.
|
@rohit-nayak-ps we need to regenerate the website docs. Should we defer that until the remaining work is complete? |
Let's defer till we have merged everything. |
Description
This PR merges the changes from previous flag refactor PRs as part of the ongoing effort to standardize CLI flag naming conventions across Vitess by replacing underscores (_) with dashes (-).
The motivation for this change is tracked in #17880, and the standardization helps improve consistency and align with the best practices.
This merge includes the following completed PRs:
#17964 — Refactor
vstream_dynamic_packet_sizeandvstream_packet_sizeflags to use dashes_to-_) / 572 (-) → after: 1484 (_) / 576 (-)#17975 — Standardize
topoflags to use hyphen-based naming--topo-global-server-address,--topo-zk-tls-key, etc.)._) / 576 (-) → after: 1366 (_) / 694 (-)#18009 — Refactor
grpcflags for standardized naming_) / 694 (-) → after: 1132 (_) / 948 (-)#18064 — Refactor flags - Part 3
_) / 948 (-) → after: 738 (_) / 1342 (-)#18095 — Refactor flags - Part 4
_) / 1342 (-) → after: 465 (_) / 1615 (-)Helper File
To support this migration, a new utility file
go/vt/utils/flags.gowas created.SetFlagIntVar,SetFlagBoolVar, etc.)Future Plan
As mentioned in the original proposal:
_and-variants of flags to give users time to adapt.Related Issue(s)
#17880
Checklist
Deployment Notes