-
Notifications
You must be signed in to change notification settings - Fork 80
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 common query parameters #3297
Conversation
1dabf33
to
bbe5d70
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
Thanks! Going down to 58 errors is incredible, but that's also a lot of review time. I've reviewed the cat APIs for now: 23/80 files. It would make sense to create a standalone pull request with just those files.
Two things that I've not mentioned in my inline comments:
- aliases, indices and recovery APIs support IndicesOptions (expand_wildcards, allow_no_indices, ignore_throttled and ignore_available, mostly) but they're not in the rest-api-spec.
- Many APIs, such as the alias API, can get the alias name from the path and the query string.
GET _cat/aliases?alias=.kibana
is a thing. I don't know if it should be encoded.
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 have 26 files left. I do suggest merging #3340 separately.
specification/ingest/delete_geoip_database/DeleteGeoipDatabaseRequest.ts
Outdated
Show resolved
Hide resolved
specification/ingest/put_geoip_database/PutGeoipDatabaseRequest.ts
Outdated
Show resolved
Hide resolved
specification/license/post_start_trial/StartTrialLicenseRequest.ts
Outdated
Show resolved
Hide resolved
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.
Done! As expected, many rest-api-spec files were incorrect.
specification/shutdown/delete_node/ShutdownDeleteNodeRequest.ts
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
* Period to wait for a response. If no response is received before the timeout expires, the request fails and returns an error. | ||
* @server_default 30s | ||
*/ | ||
timeout?: Duration |
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.
does this class match this server rest action? if so then there's no timeout (and we're missing some other parameters)
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.
Good catch, thank you! Fixed in c84c876
(#3297)
checked everything, ready to approve once my doubt on |
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
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!
* Fix many _cat API issues * Fix "master_timeout" query parameter issues * Fix "timeout" query parameter issues * Fix "pretty" & "human" query parameter issues * Use correct type for the 'time' query parameter * Address review feedback * Reduce diff with main branch * Remove timeout parameter from put_component_template --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit 7aa6d9c)
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.17 8.17
# Navigate to the new working tree
cd .worktrees/backport-8.17
# Create a new branch
git switch --create backport-3297-to-8.17
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 7aa6d9cd53e143573e1de75ea225e4a04cdd8dc9
# Push it to GitHub
git push --set-upstream origin backport-3297-to-8.17
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.17 Then, create a pull request where the |
* Fix many _cat API issues * Fix "master_timeout" query parameter issues * Fix "timeout" query parameter issues * Fix "pretty" & "human" query parameter issues * Use correct type for the 'time' query parameter * Address review feedback * Reduce diff with main branch * Remove timeout parameter from put_component_template --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit 7aa6d9c)
* Fix many _cat API issues * Fix "master_timeout" query parameter issues * Fix "timeout" query parameter issues * Fix "pretty" & "human" query parameter issues * Use correct type for the 'time' query parameter * Address review feedback * Reduce diff with main branch * Remove timeout parameter from put_component_template --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit 7aa6d9c)
* Fix common query parameters (#3297) * Fix many _cat API issues * Fix "master_timeout" query parameter issues * Fix "timeout" query parameter issues * Fix "pretty" & "human" query parameter issues * Use correct type for the 'time' query parameter * Address review feedback * Reduce diff with main branch * Remove timeout parameter from put_component_template --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit 7aa6d9c) * correct rebase * this time correct rebase --------- Co-authored-by: Sylvain Wallez <[email protected]> Co-authored-by: Laura Trotta <[email protected]>
Fix common query parameter issues:
master_timeout
is removed fromCommonCatQueryParameters
and added where relevantcat.help
response body is changed tostring
as there's no JSON representation (note that thes
andhelp
parameters present in the json spec do not exist in the code)master_timeout
andtimeout
issuesThese changes should also apply to 8.x and can be backported. They remove 109 validation errors 🎉
The remaining query parameter issues seems to be specific to the 9.x branch and require closer investigation.