-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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 inconsistencies in the rest api specs for tasks
#27163
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
hi @olcbean thanks for your PR, and good catch! I wonder if there's a way to add a test that uses these two parameters, that would have been the only way to catch this naming issue. Do you have any idea? |
@javanna I am not sure that adding tests specifically for these params would be worthwhile, as the tests would be rather naive: just verifying that there is no exception. Only for these two parameters. I really think that a more comprehensive approach should be considered. What about trying to validate the I would be interested to work on this and believe that in a couple of weeks I should have something usable. I am thinking to cover (at least) the following scenarios :
My first thought would be to do this in Currently there is no comprehensive Let me know what you think! |
hey @olcbean thanks for your proposal. We historically relied on test failures to notify us of problems in the spec. I do see that this approach has limitations and doesn't give us full coverage. On the other hand, what you propose sounds like quite some work on something that will be obsolete once we generate the spec from the server code like @jasontedor mentioned in #27158 . It is not too easy to judge whether what you propose is worth the effort or not. I tend to think it is not at the moment. @elastic/es-clients thoughts on this? |
I appreciate the proposal and the offer but I agree with @javanna here. |
Agreed as well, having the spec being generated sounds like equal amount the work and be more beneficial as a first step. Fuzzing the rest API spec afterwards could definitely proof useful 👍 |
@javanna I agree that such an approach is an overkill for only four parameters in 2 apis. But currently there are at least 29 (out of 118) apis referring to at least 94 incorrect parameter names. Generating the |
@javanna my thoughts are that we have been talking about generating the spec from code for over 4 years now ever since @karmi created the first draft by parsing the java code. Do we have a timeline for this, issue to track it, champion to develop this? I would love for this to happen as we struggle with this every time a new version is released, but with all of the things going on I am a bit worried it would fall through the cracks again. |
thanks @olcbean I just merged this. |
* 6.x: test: Break apart the multi type aspect of rolling upgrade tests, Upgrade to Jackson 2.8.10 (#27230) [Docs] Fix minor paragraph indentation error for multiple Indices params (#25535) Fix inconsistencies in the rest api specs for `tasks` (#27163) Adjust RestHighLevelClient method modifiers (#27238) Add more information on `_failed_to_convert_` exception (#27034) Remove ElasticsearchQueryCachingPolicy (#27190) Backport the size-based index rollver to v6.1.0 Add size-based condition to the index rollover API (#27160) Remove the single argument Environment constructor (#27235) Fix RestGetAction name typo
* master: (25 commits) Disable bwc tests in preparation of backporting #26931 TemplateUpgradeService should only run on the master (#27294) Die with dignity while merging Fix profiling naming issues (#27133) Correctly encode warning headers Fixed references to Multi Index Syntax (#27283) Add an active Elasticsearch WordPress plugin link (#27279) Setting url parts as required to reflect the code base (#27263) keys in aggs percentiles need to be in quotes. (#26905) Align routing param type with search.json (#26958) Update to support bulk updates by query (#27172) Remove duplicated SnapshotStatus (#27276) add split index reference in indices.asciidoc Add ability to split shards (#26931) [Docs] Fix minor paragraph indentation error for multiple Indices params (#25535) Upgrade to Jackson 2.8.10 (#27230) Fix inconsistencies in the rest api specs for `tasks` (#27163) Adjust RestHighLevelClient method modifiers (#27238) Remove unused parameters in AnalysisRegistry (#27232) Add more information on `_failed_to_convert_` exception (#27034) ...
* ccr: (127 commits) Disable bwc tests in preparation of backporting elastic#26931 TemplateUpgradeService should only run on the master (elastic#27294) Die with dignity while merging Fix profiling naming issues (elastic#27133) Correctly encode warning headers Fixed references to Multi Index Syntax (elastic#27283) Add an active Elasticsearch WordPress plugin link (elastic#27279) Setting url parts as required to reflect the code base (elastic#27263) keys in aggs percentiles need to be in quotes. (elastic#26905) Align routing param type with search.json (elastic#26958) Update to support bulk updates by query (elastic#27172) Remove duplicated SnapshotStatus (elastic#27276) add split index reference in indices.asciidoc Add ability to split shards (elastic#26931) [Docs] Fix minor paragraph indentation error for multiple Indices params (elastic#25535) Upgrade to Jackson 2.8.10 (elastic#27230) Fix inconsistencies in the rest api specs for `tasks` (elastic#27163) Adjust RestHighLevelClient method modifiers (elastic#27238) Remove unused parameters in AnalysisRegistry (elastic#27232) Add more information on `_failed_to_convert_` exception (elastic#27034) ...
Modify parameters names to bring the
tasks
rest-api-spec
up to date with the code base.Fixes #27124