Skip to content
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

Remove deprecated local parameter from alias APIs #3059

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

PeteGillinElastic
Copy link
Member

@PeteGillinElastic PeteGillinElastic commented Oct 23, 2024

This updates the specification to reflect the changes in elastic/elasticsearch#115393.

Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
indices.exists_alias 🔴 37/38 38/38
indices.get_alias 🔴 68/70 70/70

You can validate these APIs yourself by using the make validate target.

@PeteGillinElastic PeteGillinElastic changed the title Remove local parameter from GET _alias and HEAD _alias APIs Remove deprecated local parameter from alias APIs Oct 23, 2024
Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
indices.exists_alias 🔴 37/38 38/38
indices.get_alias 🔴 68/70 70/70

You can validate these APIs yourself by using the make validate target.

@PeteGillinElastic PeteGillinElastic marked this pull request as ready for review October 28, 2024 16:03
@PeteGillinElastic PeteGillinElastic requested a review from a team as a code owner October 28, 2024 16:03
@PeteGillinElastic
Copy link
Member Author

This PR removes the ?local query parameter from three APIs in the _json_spec. It makes this equivalent change in the typescript requests for two of the three. I'm not sure of the best way to do it for the third. In CatAliasesRequest.ts, the Request interface extends the class CatRequestBase which implements CommonCatQueryParameters which lists the local parameter (although not in a query_parameters which I don't quite understand). I'm not sure of the best way to remove the parameter from this API without affecting the twenty-something other places which use that base class. Any advice? (There's also a @behavior annotation and I don't know what that means. Sorry, I'm kind of groping around in the dark here!)

Also: For the two index APIs, I had to make parallel changes to the *Request.ts and the *.json in order to avoid adding validation errors. But for this CAT API, it looks like all of the parameters which are listed in cat.aliases.json and also in CommonCatQueryParameters are listed in validation-errors.json, as if the validation logic isn't recognizing that inheritance. When I remove the local parameter from the JSON without touching the typescript files, it therefore removes that validation error rather than adding a new one. I'm not sure what's going on there.

Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
indices.exists_alias 🟢 37/37 37/37
indices.get_alias 🟢 68/68 68/68

You can validate these APIs yourself by using the make validate target.

Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
indices.exists_alias 🟢 37/37 37/37
indices.get_alias 🟢 68/68 68/68

You can validate these APIs yourself by using the make validate target.

@PeteGillinElastic
Copy link
Member Author

PeteGillinElastic commented Oct 30, 2024

Regarding updating the TS to reflect the change to the _cat/aliases API:

  • There are 26 requests which implemented CommonCatQueryParameters (via CatRequestBase) which includes the local parameter.
  • Before this change, only 10 of those actually have the local parameter according to the JSON; the other 16 do not.
  • After the change currently in this PR, those numbers are 9 and 17.

I would tentatively make the argument that, as it stands, this PR is not making things materially worse, and that fixing that wider problem is out of scope for this change. What do you think?

(I did have a go at moving the parameter out of the interface and into the 9 concrete request classes which have the local parameter according to the JSON. It created some fairly major changes to the output. This reinforces my feeling that it's out of scope for what I'm doing right now. You can see my attempt at that over here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant