-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Resurrect _cluster/voting_config_exclusions/{node_name} #75951
Resurrect _cluster/voting_config_exclusions/{node_name} #75951
Conversation
9292630
to
6a371ef
Compare
6a371ef
to
f6d235c
Compare
f6d235c
to
cc6f169
Compare
See #73930 (comment) re: Team:Clients -- I don't think there's anything the clients team needs to do here. |
Pinging @elastic/clients-team (Team:Clients) |
@@ -0,0 +1,33 @@ | |||
{ |
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.
Since this route will always produce an error even in compatibility mode, should we not create an API specification for it? I'm not sure we want this API to be in generated clients
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.
Since it's under rest-api-spec/src/yamlRestCompatTest/resources/rest-api-spec/api/
, I think we're already good on that, right? That is, this is just intended to be a spec definition for the purposes of the yml tests themselves, it's not supposed to leak up and out.
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.
Hmm, I don't know the rules for which files are picked up by the artifacts API and which aren't. If this file doesn't make it's way into the built artifact for rest-api-spec
then this is fine.
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 am pretty confident that this will not get picked up to be visible by clients, if it does we can quickly fix it (or revert this PR).
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.
LGTM
Adds back the
_cluster/voting_config_exclusions/{node_name}
route via rest api compatibility, but just so that we can have a better error message. Related to #51816, and a little bit of a follow up to #75406.See also #76316 -- the last four commits here are a best effort at making sure that the test I'm adding will do what I want it to when that bug doesn't exist anymore.