-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Scripting: Remove file scripts #24627
Conversation
This commit removes file scripts, which were deprecated in 5.5. closes elastic#21798
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments, nothing major.
@@ -225,19 +176,11 @@ public void testDefaultBehaviourFineGrainedSettings() throws IOException { | |||
deprecate = true; | |||
} | |||
buildScriptService(builder.build()); | |||
createFileScripts("dtest"); | |||
|
|||
for (ScriptContext scriptContext : scriptContexts) { | |||
// only file scripts are accepted by default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment can go.
@@ -306,12 +249,11 @@ public void testFineGrainedSettings() throws IOException { | |||
} | |||
|
|||
buildScriptService(builder.build()); | |||
createFileScripts("expression", "mustache", "dtest"); | |||
|
|||
for (ScriptType scriptType : ScriptType.values()) { | |||
//make sure file scripts have a different name than inline ones. | |||
//Otherwise they are always considered file ones as they can be found in the static cache. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments about file scripts can go.
|
||
for (ScriptType scriptType : ScriptType.values()) { | ||
//make sure file scripts have a different name than inline ones. | ||
//Otherwise they are always considered file ones as they can be found in the static cache. | ||
String script = scriptType == ScriptType.FILE ? "file_script" : "script"; | ||
String script = "script"; | ||
for (ScriptContext scriptContext : this.scriptContexts) { | ||
//fallback mechanism: 1) engine specific settings 2) op based settings 3) source based settings | ||
Boolean scriptEnabled = engineSettings.get(dangerousScriptEngine.getType() + "." + scriptType + "." + scriptContext.getKey()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't let me review line 278 where there is still an assertion about file scripts.
@@ -71,7 +70,6 @@ | |||
private static final Map<ScriptType, Boolean> DEFAULT_SCRIPT_ENABLED = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a field up there scriptsFilePath
that I think can go?
@@ -56,7 +56,6 @@ | |||
//TODO: this needs to be a base test class, and all scripting engines extend it | |||
public class ScriptServiceTests extends ESTestCase { | |||
|
|||
private ResourceWatcherService resourceWatcherService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are imports up above that can be removed.
@@ -225,19 +176,11 @@ public void testDefaultBehaviourFineGrainedSettings() throws IOException { | |||
deprecate = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can go?
Also, I think there are some packaging tests that set up a file script? |
@elasticmachine retest this please |
@jasontedor Packaging tests are updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
File scripts were removed in 6.0 with #24627. This removes an outdated file scripts reference from the conditional clauses section of the search templates docs.
File scripts were removed in 6.0 with #24627. This removes an outdated file scripts reference from the conditional clauses section of the search templates docs.
File scripts were removed in 6.0 with #24627. This removes an outdated file scripts reference from the conditional clauses section of the search templates docs.
File scripts were removed in 6.0 with elastic#24627. This removes an outdated file scripts reference from the conditional clauses section of the search templates docs.
This commit removes file scripts, which were deprecated in 5.5.
closes #21798