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

Fix inconsistencies in the rest api specs for *_script #26971

Merged
merged 2 commits into from
Oct 13, 2017
Merged

Fix inconsistencies in the rest api specs for *_script #26971

merged 2 commits into from
Oct 13, 2017

Conversation

olcbean
Copy link
Contributor

@olcbean olcbean commented Oct 11, 2017

Removing inconsistencies in the resp api specs for the *_script by setting some path parts to optional.

Fixes #26923

CC @javanna @jdconrad @Mpdreamz

@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?

jdconrad
jdconrad approved these changes Oct 11, 2017
View reviewed changes
Copy link
Contributor

@jdconrad jdconrad left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this.

Mpdreamz
Mpdreamz requested changes Oct 11, 2017
View reviewed changes
rest-api-spec/src/main/resources/rest-api-spec/api/put_script.json Outdated
"type" : "string",
"description" : "Script language",
"required" : true
"description" : "Script language"
Copy link
Member

@Mpdreamz Mpdreamz Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Its not quite the Script language as string. It's a new parameter that will cause the passed script to be compiled and evaluated in a certain ScriptContext.

See: https://github.com/elastic/elasticsearch/blob/master/core/src/main/java/org/elasticsearch/script/ScriptModule.java#L41-L52

This should be an enum with multiple values:

search, aggs, executable, aggs_executable, update, ingest, filter, similarity, similarity_weight & template.

Thats the current list for master 6.0 and 6.x have a slightly smaller list:

https://github.com/elastic/elasticsearch/blob/6.0/core/src/main/java/org/elasticsearch/script/ScriptModule.java#L41-L52

https://github.com/elastic/elasticsearch/blob/6.x/core/src/main/java/org/elasticsearch/script/ScriptModule.java#L41-L52

Copy link
Member

Choose a reason for hiding this comment

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

An enum is not correct. These values are pluggable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mpdreamz thanks for putting it in context!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @rjernst, @olcbean LGTM now 👍

@Mpdreamz
Copy link
Member

Thanks for also taking context on board @olcbean 👍 requested some changes regarding it though.

@olcbean
Copy link
Contributor Author

olcbean commented Oct 11, 2017

@Mpdreamz nice catch! I was concentrated on the functionality, ignoring the descriptions ;) Can you have another look?

Mpdreamz
Mpdreamz approved these changes Oct 11, 2017
View reviewed changes
rest-api-spec/src/main/resources/rest-api-spec/api/put_script.json Outdated
"type" : "string",
"description" : "Script language",
"required" : true
"description" : "Script language"
Copy link
Member

Choose a reason for hiding this comment

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

Thanks @rjernst, @olcbean LGTM now 👍

@rjernst
Copy link
Member

rjernst commented Oct 12, 2017

@elasticmachine please test this

@rjernst rjernst merged commit bb013c6 into elastic:master Oct 13, 2017
@rjernst
Copy link
Member

rjernst commented Oct 13, 2017

Thanks @olcbean

rjernst pushed a commit that referenced this pull request Oct 13, 2017
@olcbean @rjernst
Fix inconsistencies in the rest api specs for *_script (#26971)
4088dd0
rjernst pushed a commit that referenced this pull request Oct 13, 2017
@olcbean @rjernst
Fix inconsistencies in the rest api specs for *_script (#26971)
fa80aec
@olcbean olcbean deleted the script_paths branch October 14, 2017 09:44
jasontedor added a commit to olcbean/elasticsearch that referenced this pull request Oct 16, 2017
@jasontedor
Merge branch 'master' into pr/23767
6bb1d2c 
* master: (356 commits)
  Do not set SO_LINGER on server channels (elastic#26997)
  Fix inconsistencies in the rest api specs for *_script (elastic#26971)
  fix inconsistencies in the rest api specs for cat.snapshots (elastic#26996)
  Add docs on full_id parameter in cat nodes API
  [TEST] Add test that replicates versioned updates with random flushes
  Use internal searcher for all indexing related operations in the engine
  Reformat paragraph in template docs to 80 columns
  Clarify settings and template on create index
  Fix reference to TcpTransport in documentation
  Allow Uid#decodeId to decode from a byte array slice (elastic#26987)
  Fix a typo in the similarity docs (elastic#26970)
  Use separate searchers for "search visibility" vs "move indexing buffer to disk (elastic#26972)
  Create weights lazily in filter and filters aggregation (elastic#26983)
  Use a dedicated ThreadGroup in rest sniffer (elastic#26897)
  Fire global checkpoint sync under system context
  Update by Query is modified to accept short `script` parameter. (elastic#26841)
  Cat shards bytes (elastic#26952)
  Add support for parsing inline script (elastic#23824) (elastic#26846)
  Change default value to true for transpositions parameter of fuzzy query (elastic#26901)
  Adding unreleased 5.6.4 version number to Version.java
  ...
jasontedor added a commit that referenced this pull request Oct 16, 2017
@jasontedor
Merge branch 'master' into ccr
8a3540c 
* master:
  Remove unnecessary exception for engine constructor
  Update docs about `script` parameter (#27010)
  Don't refresh on `_flush` `_force_merge` and `_upgrade` (#27000)
  Do not set SO_LINGER on server channels (#26997)
  Fix inconsistencies in the rest api specs for *_script (#26971)
  fix inconsistencies in the rest api specs for cat.snapshots (#26996)
  Add docs on full_id parameter in cat nodes API
  [TEST] Add test that replicates versioned updates with random flushes
  Use internal searcher for all indexing related operations in the engine
  Reformat paragraph in template docs to 80 columns
  Clarify settings and template on create index
  Fix reference to TcpTransport in documentation
  Allow Uid#decodeId to decode from a byte array slice (#26987)
  Fix a typo in the similarity docs (#26970)
  Use separate searchers for "search visibility" vs "move indexing buffer to disk (#26972)
jasontedor added a commit that referenced this pull request Oct 16, 2017
@jasontedor
Merge branch '6.x' into ccr-6.x
7cc7cd5 
* 6.x:
  Remove unnecessary exception for engine constructor
  Update docs about `script` parameter (#27010)
  Do not set SO_LINGER on server channels (#26997)
  Fix inconsistencies in the rest api specs for *_script (#26971)
  fix inconsistencies in the rest api specs for cat.snapshots (#26996)
  Add docs on full_id parameter in cat nodes API
  Add removal of types to the 6.0 breaking changes
  Create weights lazily in filter and filters aggregation (#26983)
  [TEST] Add test that replicates versioned updates with random flushes
  Use internal searcher for all indexing related operations in the engine
  Use separate searchers for "search visibility" vs "move indexing buffer to disk (#26972)
  Reformat paragraph in template docs to 80 columns
  Clarify settings and template on create index
  Fix reference to TcpTransport in documentation
  Allow Uid#decodeId to decode from a byte array slice (#26987)
  Fix a typo in the similarity docs (#26970)
@lcawl lcawl added v6.0.0-rc2 and removed v6.0.0 labels Oct 30, 2017
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjernst rjernst rjernst left review comments

@jdconrad jdconrad jdconrad approved these changes

@Mpdreamz Mpdreamz Mpdreamz approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@olcbean @elasticmachine @Mpdreamz @rjernst @jdconrad @colings86 @lcawl