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

Scripting: Remove deprecated params.ctx #36848

Merged
merged 11 commits into from
Dec 21, 2018
Merged

Scripting: Remove deprecated params.ctx #36848

merged 11 commits into from
Dec 21, 2018

Conversation

hub-cap
Copy link
Contributor

@hub-cap hub-cap commented Dec 19, 2018

When the script contexts were created in 6, the use of params.ctx was
deprecated. This commit cleans up that code and ensures that params.ctx
is null in both watcher script contexts.

Relates: #34059

@hub-cap
Watcher: Remove deprecated params.ctx
35ed56d 
When the script contexts were created in 6, the use of params.ctx was
deprecated. This commit cleans up that code and ensures that params.ctx
is null in both watcher script contexts.

Relates: elastic#34059
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@hub-cap
Copy link
Contributor Author

hub-cap commented Dec 19, 2018

Im not sure if this should have the >deprecation label or not, I dont know how we identify the code that is being removed due to a previous deprecation.

@hub-cap hub-cap mentioned this pull request Dec 19, 2018
Merged
@hub-cap hub-cap added the WIP label Dec 19, 2018
@hub-cap
Copy link
Contributor Author

hub-cap commented Dec 19, 2018

Ive added the WIP label so that i can spend some time testing this in a deployment. There is also some interesting code that is very similar between Variables.createCtx and Variables.createCtxAsMap that could potentially be removed / consolidated, so that might come out of this too.

@hub-cap
Copy link
Contributor Author

hub-cap commented Dec 20, 2018

I also found UpdateScript when looking around tonight, so I should probably make this slightly more generic and remove it as well.

@hub-cap
Copy link
Contributor Author

hub-cap commented Dec 20, 2018

ok so about the createCtx vs createCtxAsMap, the ctx is already an exposed variable in scripts, so they do not use createCtxAsMap, but the mustache templates do not, so they are given the map with a key ctx to the values of createCtx. therefore this is not something we can remove. Therefore I am removing the WIP label to this, and changing the title of the PR as well since I added the UpdateScript in as well.

@hub-cap hub-cap added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache and removed WIP labels Dec 20, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@hub-cap hub-cap changed the title Watcher: Remove deprecated params.ctx Scripting: Remove deprecated params.ctx Dec 20, 2018
@hub-cap hub-cap requested a review from rjernst December 20, 2018 14:20
rjernst
rjernst approved these changes Dec 20, 2018
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

I think we can still eventually remove ctx, it just needs proper deprecation in mustache scripts.

LGTM.

@hub-cap
Copy link
Contributor Author

hub-cap commented Dec 20, 2018

@elasticmachine run gradle build tests 2
@elasticmachine run default distro tests

@hub-cap
Copy link
Contributor Author

hub-cap commented Dec 20, 2018

Yea definitely, its just a much larger fix since ctx is all over the place in the mustache rendered scripts.

jakelandis
jakelandis reviewed Dec 20, 2018
View reviewed changes
...in/watcher/src/test/java/org/elasticsearch/xpack/watcher/condition/ScriptConditionTests.java Outdated
@@ -206,7 +206,7 @@ public void testScriptConditionAccessCtx() throws Exception {
assertThat(condition.execute(ctx).met(), is(true));
}

public void testParamsCtxDeprecated() throws Exception {
public void testParamsCtxNull() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this test (and the one below) still make sense ?

The test is based on the absence of something that used to be there. I would be +1 to simply remove the deprecation tests (instead of changing them to null tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think its fair to remove these 2 tests (this and the other one mentioned i forgot to rename).

jakelandis
jakelandis reviewed Dec 20, 2018
View reviewed changes
...her/src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java
@@ -202,9 +202,7 @@ public Object execute() {
return getParams().get("ctx");
}
};
assertThat(watcherScript.execute(), is(watcherScript.getCtx()));
Copy link
Contributor

Choose a reason for hiding this comment

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

If we keep this test we should change the name accordingly. (similar as above)

@jakelandis
Copy link
Contributor

A couple minor comments. LGTM (assuming fixing the deprecated test name, or removing the deprecation tests)

@hub-cap
Copy link
Contributor Author

hub-cap commented Dec 21, 2018

@elasticmachine run gradle build tests 1
@elasticmachine run gradle build tests 2

@hub-cap hub-cap merged commit 7cbf03c into elastic:master Dec 21, 2018
jasontedor added a commit to ywelsch/elasticsearch that referenced this pull request Dec 21, 2018
@jasontedor
Merge remote-tracking branch 'elastic/master' into pr/36137
8de54b8 
* elastic/master: (539 commits)
  SQL: documentation improvements and updates (elastic#36918)
  [DOCS] Merges list of discovery and cluster formation settings (elastic#36909)
  Only compress responses if request was compressed (elastic#36867)
  Remove duplicate paragraph (elastic#36942)
  Fix URI to cluster stats endpoint on specific nodes (elastic#36784)
  Fix typo in unitTest task (elastic#36930)
  RecoveryMonitor#lastSeenAccessTime should be volatile (elastic#36781)
  [CCR] Add `ccr.auto_follow_coordinator.wait_for_timeout` setting (elastic#36714)
  Scripting: Remove deprecated params.ctx (elastic#36848)
  Refactor the REST actions to clarify what endpoints are deprecated. (elastic#36869)
  Add JDK 12 to CI rotation (elastic#36915)
  Improve error message for 6.x style realm settings (elastic#36876)
  Send clear session as routable remote request (elastic#36805)
  [DOCS] Remove redundant ILM attributes (elastic#36808)
  SQL: Fix bug regarding histograms usage in scripting (elastic#36866)
  Update index mappings when ccr restore complete (elastic#36879)
  Docs: Bump version to alpha2 after release
  Enable IPv6 URIs in reindex from remote (elastic#36874)
  Watcher: Remove unused local variable in doExecute (elastic#36655)
  [DOCS] Synchs titles of X-Pack APIs
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Dec 21, 2018
@jasontedor
Merge branch 'master' into package-ingest-geoip-as-a-module
30d92d1 
* master: (31 commits)
  Move ingest-geoip default databases out of config (elastic#36949)
  [ILM][DOCS] add extra scenario to policy update docs (elastic#36871)
  [Painless] Add String Casting Tests (elastic#36945)
  SQL: documentation improvements and updates (elastic#36918)
  [DOCS] Merges list of discovery and cluster formation settings (elastic#36909)
  Only compress responses if request was compressed (elastic#36867)
  Remove duplicate paragraph (elastic#36942)
  Fix URI to cluster stats endpoint on specific nodes (elastic#36784)
  Fix typo in unitTest task (elastic#36930)
  RecoveryMonitor#lastSeenAccessTime should be volatile (elastic#36781)
  [CCR] Add `ccr.auto_follow_coordinator.wait_for_timeout` setting (elastic#36714)
  Scripting: Remove deprecated params.ctx (elastic#36848)
  Refactor the REST actions to clarify what endpoints are deprecated. (elastic#36869)
  Add JDK 12 to CI rotation (elastic#36915)
  Improve error message for 6.x style realm settings (elastic#36876)
  Send clear session as routable remote request (elastic#36805)
  [DOCS] Remove redundant ILM attributes (elastic#36808)
  SQL: Fix bug regarding histograms usage in scripting (elastic#36866)
  Update index mappings when ccr restore complete (elastic#36879)
  Docs: Bump version to alpha2 after release
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakelandis jakelandis jakelandis left review comments

@rjernst rjernst rjernst approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >deprecation v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hub-cap @elasticmachine @jakelandis @rjernst @jimczi