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.

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

Fix the ability to remove old plugin #28540

Merged
merged 3 commits into from Feb 6, 2018

Conversation

Projects
None yet
3 participants
@jasontedor
Copy link
Member

commented Feb 6, 2018

We now read the plugin descriptor when removing an old plugin. This is to check if we are removing a plugin that is extended by another plugin. However, when reading the descriptor we enforce that it is of the same version that we are. This is not the case when a user has upgraded Elasticsearch and is now trying to remove an old plugin. This commit fixes this by skipping the version enforcement when reading the plugin descriptor only when removing a plugin.

Closes #28538

Fix the ability to remove old plugin
We now read the plugin descriptor when removing an old plugin. This is
to check if we are removing a plugin that is extended by another
plugin. However, when reading the descriptor we enforce that it is of
the same version that we are. This is not the case when a user has
upgraded Elasticsearch and is now trying to remove an old plugin. This
commit fixes this by skipping the version enforcement when reading the
plugin descriptor only when removing a plugin.
@rjernst
Copy link
Member

left a comment

This looks ok, but I would rather not have 2 variants of readFromProperties. What about moving the enforcement out to another method in install/startup, so reading the properties is just reading properties?

@@ -137,6 +147,13 @@ public void testBasic() throws Exception {
assertRemoveCleaned(env);
}

public void testRemoveOldVersion() throws Exception {
createPlugin("fake", VersionUtils.randomVersionBetween(random(), Version.CURRENT.minimumCompatibilityVersion(), Version.CURRENT));

This comment has been minimized.

Copy link
@rjernst

rjernst Feb 6, 2018

Member

Since randomVersionBetween is inclusive, I think the top end of this range should be VersionUtils.getPreviousVersion()?

This comment has been minimized.

Copy link
@jasontedor

jasontedor Feb 6, 2018

Author Member

I pushed a commit for this.

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2018

This looks ok, but I would rather not have 2 variants of readFromProperties. What about moving the enforcement out to another method in install/startup, so reading the properties is just reading properties?

I have mixed feelings about this. Currently we validate everything in readFromProperties. We have never had a need to skip a portion of the validation (because we did not previously need to read the properties descriptor except when we expected it to be perfect). I am okay reconsidering this, but I would prefer that to be in a follow-up?

@rjernst

rjernst approved these changes Feb 6, 2018

Copy link
Member

left a comment

I'm ok with considering that in a followup. LGTM.

@jasontedor jasontedor merged commit c2fcf15 into elastic:master Feb 6, 2018

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

jasontedor added a commit that referenced this pull request Feb 6, 2018

Fix the ability to remove old plugin
We now read the plugin descriptor when removing an old plugin. This is
to check if we are removing a plugin that is extended by another
plugin. However, when reading the descriptor we enforce that it is of
the same version that we are. This is not the case when a user has
upgraded Elasticsearch and is now trying to remove an old plugin. This
commit fixes this by skipping the version enforcement when reading the
plugin descriptor only when removing a plugin.

Relates #28540

jasontedor added a commit that referenced this pull request Feb 6, 2018

Fix the ability to remove old plugin
We now read the plugin descriptor when removing an old plugin. This is
to check if we are removing a plugin that is extended by another
plugin. However, when reading the descriptor we enforce that it is of
the same version that we are. This is not the case when a user has
upgraded Elasticsearch and is now trying to remove an old plugin. This
commit fixes this by skipping the version enforcement when reading the
plugin descriptor only when removing a plugin.

Relates #28540

@jasontedor jasontedor deleted the jasontedor:remove-old-plugin branch Feb 6, 2018

rjernst added a commit to rjernst/elasticsearch that referenced this pull request Feb 8, 2018

Plugins: Separate plugin semantic validation from properties format v…
…alidation

This commit moves the semantic validation (like which version a plugin
was built for or which java version it is compatible with) from reading
a plugin descriptor, leaving the checks on the format of the descriptor
intact.

relates elastic#28540

rjernst added a commit that referenced this pull request Feb 13, 2018

Plugins: Separate plugin semantic validation from properties format v…
…alidation (#28581)

This commit moves the semantic validation (like which version a plugin
was built for or which java version it is compatible with) from reading
a plugin descriptor, leaving the checks on the format of the descriptor
intact.

relates #28540

rjernst added a commit that referenced this pull request Feb 13, 2018

Plugins: Separate plugin semantic validation from properties format v…
…alidation (#28581)

This commit moves the semantic validation (like which version a plugin
was built for or which java version it is compatible with) from reading
a plugin descriptor, leaving the checks on the format of the descriptor
intact.

relates #28540

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.