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

Fixes for API specification #46522

Merged
merged 9 commits into from
Sep 16, 2019
Merged

Fixes for API specification #46522

merged 9 commits into from
Sep 16, 2019

Conversation

delvedor
Copy link
Member

After #42346, some documentation link the and definition of slm.delete_lifecycle were not correct, this pr fixes them.

Related: #44487 #45102
This should also be backported to 7.x.

cc @elastic/es-clients

@javanna javanna added the :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs label Sep 10, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@javanna javanna removed their request for review September 10, 2019 08:53
@delvedor delvedor changed the title Fix spec url and slm.delete_lifecycle definition Fixes for API specification Sep 10, 2019
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment, this ends up adding an invalid endpoint

}
},
{
"path":"/_slm/policy",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SLM delete policy does not support a path without the {policy_id} parameter, it's not allowed at all

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dakrone! I was under the impression that a path without the policy_id was allowed, because in the original implementation, before the new spec format, the policy_id was not marked as required: https://github.com/elastic/elasticsearch/blob/a2e0db7783055300de77f3a0e525a21ac2bd7530/x-pack/plugin/src/test/resources/rest-api-spec/api/slm.delete_lifecycle.json

I'll update the spec.

@dakrone dakrone added :Data Management/ILM+SLM Index and Snapshot lifecycle management and removed :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs labels Sep 10, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@polyfractal polyfractal added the :Core/Infra/REST API REST infrastructure and utilities label Sep 11, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@polyfractal
Copy link
Contributor

Had a look at error_trace failures. Those are being caused by this change:

-  "description": "Parameters that are accepted by all API endpoints.",
-  "documentation": "https://www.elastic.co/guide/en/elasticsearch/reference/current/common-options.html",
+  "documentation": {
+    "url": "https://www.elastic.co/guide/en/elasticsearch/reference/current/common-options.html",
+    "description": "Parameters that are accepted by all API endpoints."
+  },

It looks like the parsing for _common.json assumes there is only one object to parse. With this change there are now two objects, and I believe it's exiting the parsing early so some of the default options are not being applied correctly... which means error_trace is being used on APIs that don't support it.

Gonna tag :core/infra on this since the parser probably needs a tweak to conform to the new schema in #42346

@delvedor delvedor merged commit a2a502b into elastic:master Sep 16, 2019
delvedor added a commit to delvedor/elasticsearch that referenced this pull request Sep 16, 2019
delvedor added a commit to delvedor/elasticsearch that referenced this pull request Sep 16, 2019
@colings86 colings86 added >bug and removed :Core/Infra/REST API REST infrastructure and utilities labels Sep 17, 2019
delvedor added a commit that referenced this pull request Sep 17, 2019
@javanna javanna added the v7.5.0 label Sep 23, 2019
javanna pushed a commit that referenced this pull request Sep 23, 2019
@delvedor delvedor deleted the fix-spec branch November 18, 2019 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants