-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix: remove support for NoOptDefaultValue #14985
fix: remove support for NoOptDefaultValue #14985
Conversation
This is what had to be backported to v0.47 right? |
Yes ! Sorry forgot to tag |
(cherry picked from commit 3c53870)
@@ -101,9 +101,6 @@ message FlagOptions { | |||
// default_value is the default value as text. | |||
string default_value = 4; | |||
|
|||
// default value is the default value as text if the flag is used without any value. | |||
string no_opt_default_value = 5; | |||
|
|||
// deprecated is the usage text to show if this flag is deprecated. | |||
string deprecated = 6; |
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.
Actually, as we are deleting a field, maybe we should decrease the identifier of the others too, given it has not been yet released. cc @JeancarloBarrios
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.
True I missed that thanks !!
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.
I would suggest we don't do this actually... so the issue is that this is part of the api
module not 0.47 at all. So backporting doesn't really do anything. The thing that we need to do is tag api
and then backport the required API module version to 0.47. Removing the field is one thing, but changing the numbers really would break any case that someone built their binary with an earlier version of api
. This is maybe the first case in the wild where the issues raised in ADR 046 can be observed. Let's discuss a tagging/versioning strategy next week.
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 makes sense! Didn't notice the generated proto was pulsar.
One small unrelated issue is that now API possibly not support v0.47 entirely given CometBFT (#14899)
Description
Removes NoOptDefault Value flag from autcocli due to a bug in pflag.
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change