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.

Sign up for GitHub

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

Jump to bottom

rest-api-spec : setting path parts as required to reflect the code base #27263

Merged
merged 1 commit into from
Nov 6, 2017
Merged

rest-api-spec : setting path parts as required to reflect the code base #27263

merged 1 commit into from
Nov 6, 2017

Conversation

olcbean
Copy link
Contributor

@olcbean olcbean commented Nov 3, 2017

Setting path parts as required when needed in the rest-spec-apis.

@elasticmachine
Copy link
Collaborator

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?

dakrone
dakrone requested changes Nov 3, 2017
View reviewed changes
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.

Thanks @olcbean, I left a comment

rest-api-spec/src/main/resources/rest-api-spec/api/tasks.get.json
@@ -8,6 +8,7 @@
"parts": {
"task_id": {
"type": "string",
"required" : true,
Copy link
Member

Choose a reason for hiding this comment

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

This is not actually required, you can do GET /_tasks no problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dakrone you are correct! But when you call GET /_tasks without task_id, it is mapped to :

elasticsearch/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestListTasksAction.java

Lines 52 to 56 in 3deba0e

public RestListTasksAction(Settings settings, RestController controller, Supplier<DiscoveryNodes> nodesInCluster) {
super(settings);
this.nodesInCluster = nodesInCluster;
controller.registerHandler(GET, "/_tasks", this);
}
defined by tasks.list in the rest-api-specs.

tasks.get is for :

elasticsearch/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetTaskAction.java

Lines 37 to 40 in 3deba0e

public RestGetTaskAction(Settings settings, RestController controller) {
super(settings);
controller.registerHandler(GET, "/_tasks/{task_id}", this);
}

Copy link
Member

Choose a reason for hiding this comment

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

That's true, and there is the other .json spec file that mentions solely /_tasks

Hmm... I'm not sure what the best thing for this situation is, perhaps leaving it as required is the best since it only pertains to this particular API (not tasks.list.json)

Copy link
Member

Choose a reason for hiding this comment

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

are there downsides for this change in the spec? I think it makes sense in the context of the get task API.

Copy link
Member

Choose a reason for hiding this comment

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

@javanna yeah I think it does too, I just wanted to think about it for a little bit. I'll merge this.

@javanna javanna added :Core/Infra/REST API REST infrastructure and utilities >non-issue labels Nov 6, 2017
@dakrone
Copy link
Member

dakrone commented Nov 6, 2017

@elasticmachine test this please

@dakrone dakrone merged commit 7f593a2 into elastic:master Nov 6, 2017
@dakrone
Copy link
Member

dakrone commented Nov 6, 2017

Thanks @olcbean!

dakrone pushed a commit that referenced this pull request Nov 6, 2017
@olcbean @dakrone
Setting url parts as required to reflect the code base (#27263)
0858ed4
jasontedor added a commit that referenced this pull request Nov 7, 2017
@jasontedor
Merge branch 'master' into ccr
3b6d0ce 
* 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)
  ...
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Nov 7, 2017
@jasontedor
Merge branch 'ccr' into pr/27094
13d373b 
* 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)
  ...
jasontedor added a commit that referenced this pull request Nov 7, 2017
@jasontedor
Merge branch '6.x' into ccr-6.x
efe0981 
* 6.x:
  Update shrink's bwc version to 6.1.0
  add split index reference in indices.asciidoc
  Add ability to split shards (#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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javanna javanna javanna left review comments

@dakrone dakrone dakrone approved these changes

Assignees
No one assigned
Labels
:Core/Infra/REST API REST infrastructure and utilities >non-issue v6.1.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@olcbean @elasticmachine @dakrone @javanna @colings86