Skip to content

Scripting: Deprecate native scripts #24692

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
Merged
merged 2 commits into from
May 16, 2017
Merged

Scripting: Deprecate native scripts #24692

merged 2 commits into from
May 16, 2017

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented May 15, 2017

Native scripts are no longer documented and instead using a ScriptEngine
is recommended. This change adds a deprecation warning for removal in
6.0.

relates #19966

@rjernst
Scripting: Deprecate native scripts
10cca16 
Native scripts are no longer documented and instead using a ScriptEngine
is recommended. This change adds a deprecation warning for removal in
6.0.

relates elastic#19966
@rjernst rjernst added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >deprecation v5.5.0 v6.0.0 labels May 15, 2017
@rjernst
Copy link
Member Author

rjernst commented May 16, 2017

@elasticmachine retest this please

@rjernst rjernst requested a review from imotov May 16, 2017 08:11
@nik9000
Copy link
Member

nik9000 commented May 16, 2017

LGTM so long as you add something to the release notes on backport.

imotov
imotov reviewed May 16, 2017
View reviewed changes
Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

The logging looks good, but I agree with @nik9000, release notes would be useful. I think it might be also helpful to deprecate NativeScriptFactory interface. Otherwise, new users might discover that native scripts are deprecated only after implementing one of them.

@rjernst
Copy link
Member Author

rjernst commented May 16, 2017

@nik9000 What do you mean by "release notes"? The migration docs are for 6.0, in which I will add a note with a followup PR which removes native scripts on master.

imotov
imotov approved these changes May 16, 2017
View reviewed changes
Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@rjernst
Copy link
Member Author

rjernst commented May 16, 2017

I spoke with @nik9000 about release notes, and he did indeed mean migration docs. We have not used these for deprecations in the past, at least not in any consistent fashion. I will add a migration note in the followup removing native scripts as I noted above.

@rjernst rjernst merged commit 25dd644 into elastic:master May 16, 2017
@rjernst rjernst deleted the deprecate_native_scripts branch May 16, 2017 18:57
@rjernst
Copy link
Member Author

rjernst commented May 16, 2017

Also note that this change will show up under deprecations in the 5.5.0 release notes, since this PR is marked with the deprecation label.

rjernst added a commit that referenced this pull request May 16, 2017
@rjernst
Scripting: Deprecate native scripts (#24692)
89bd168 
Native scripts are no longer documented and instead using a ScriptEngine
is recommended. This change adds a deprecation warning for removal in
6.0.

relates #19966
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imotov imotov imotov approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@rjernst @nik9000 @imotov @clintongormley