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

Feat: conservative plugin (dependency) updates by default #13794

Merged
merged 3 commits into from Feb 21, 2022

Conversation

kares
Copy link
Contributor

@kares kares commented Feb 17, 2022

bin/logstash-plugin install logstash-input-s3-sns-sqs

New behavior (--conservative default) :

diff --git Gemfile.lock Gemfile.lock
index b3d2b0b89..5c7742fd0 100644
--- Gemfile.lock
+++ Gemfile.lock
@@ -476,6 +476,10 @@ GEM
       logstash-mixin-aws (>= 5.1.0)
       logstash-mixin-ecs_compatibility_support (~> 1.2)
       stud (~> 0.0.18)
+    logstash-input-s3-sns-sqs (2.1.3)
+      logstash-codec-json (~> 3.0)
+      logstash-core-plugin-api (>= 2.1.12, <= 2.99)
+      logstash-mixin-aws (>= 4.3)
     logstash-input-snmp (1.3.1)
       logstash-codec-plain
       logstash-core-plugin-api (>= 1.60, <= 2.99)
@@ -864,6 +868,7 @@ DEPENDENCIES
   logstash-input-pipe
   logstash-input-redis
   logstash-input-s3
+  logstash-input-s3-sns-sqs
   logstash-input-snmp
   logstash-input-snmptrap
   logstash-input-sqs

Previous behavior (--no-conservative) :

diff --git Gemfile.lock Gemfile.lock
index 054a98970..877284c1c 100644
--- Gemfile.lock
+++ Gemfile.lock
@@ -166,7 +166,7 @@ GEM
     jruby-jms (1.3.0-java)
       gene_pool
       semantic_logger
-    jruby-openssl (0.11.0-java)
+    jruby-openssl (0.12.1-java)
     jruby-stdin-channel (0.2.0-java)
     json (2.6.1-java)
     json-schema (2.8.1)
@@ -476,6 +476,10 @@ GEM
       logstash-mixin-aws (>= 5.1.0)
       logstash-mixin-ecs_compatibility_support (~> 1.2)
       stud (~> 0.0.18)
+    logstash-input-s3-sns-sqs (2.1.3)
+      logstash-codec-json (~> 3.0)
+      logstash-core-plugin-api (>= 2.1.12, <= 2.99)
+      logstash-mixin-aws (>= 4.3)
     logstash-input-snmp (1.3.1)
       logstash-codec-plain
       logstash-core-plugin-api (>= 1.60, <= 2.99)
@@ -705,7 +709,7 @@ GEM
       nio4r (~> 2.0)
     racc (1.5.2-java)
     rack (2.2.3)
-    rack-protection (2.1.0)
+    rack-protection (2.2.0)
       rack
     rack-test (1.1.0)
       rack (>= 1.0, < 3)
@@ -740,10 +744,10 @@ GEM
       concurrent-ruby (~> 1.0)
     sequel (5.53.0)
     simple_oauth (0.3.1)
-    sinatra (2.1.0)
+    sinatra (2.2.0)
       mustermann (~> 1.0)
       rack (~> 2.2)
-      rack-protection (= 2.1.0)
+      rack-protection (= 2.2.0)
       tilt (~> 2.0)
     snappy (0.0.12-java)
       snappy-jars (~> 1.1.0)
@@ -864,6 +868,7 @@ DEPENDENCIES
   logstash-input-pipe
   logstash-input-redis
   logstash-input-s3
+  logstash-input-s3-sns-sqs
   logstash-input-snmp
   logstash-input-snmptrap
   logstash-input-sqs

Release notes

The bin/logstash-plugin utility will, by default, behave more conservatively when updating plugin dependencies.

What does this PR do?

Introduces --conservative (default) / --no-conservative flag for plugin install and update.

Why is it important/What is the impact to the user?

The user will end up less surprised when running bin/logstash-plugin update or install (when dependencies such as a mixin needs an update to install the plugin).

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

How to test this PR locally

Run bin/logstash-plugin install logstash-input-s3-sns-sqs and compare the Gemfile.lock changes.

Related issues

Pretty much avoids surprise updates of a dependency such as #13777

@kares kares marked this pull request as ready for review February 17, 2022 15:29
@kares kares self-assigned this Feb 17, 2022
Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

This looks like a promising resolution to #13789, and although this PR adds a "feature" to allow direct control of the underlying bundler, it effectively does so in addition to solving the bug of surprises in unrelated dependencies being upgraded.

I am +1 to also back-porting to 7.17.

@yaauie
Copy link
Member

yaauie commented Feb 17, 2022

@kares it may be worth determining if this is effectively the same as the existing --preserve option for plugin installation.

@kares
Copy link
Contributor Author

kares commented Feb 21, 2022

may be worth determining if this is effectively the same as the existing --preserve option for plugin installation.

thanks - have seen that but did not look to close enough as it wasn't a Bundler flag.

the end result here seems to be the same for this use-case, but there's a difference what we do underneath:

  • conservative does a bundle update --conservative for the mixin than a bundle install
  • the install --preserve flag skips the initial bundle update and only does a bundle install

another difference is that it's an install flag only and an explicit bin/logstash-plugin update will still (just like previously) bring in updates to potentially all dependencies there are in the bundle.

also wonder what happens if a dependency update is needed on install that I guess the plugin-manager with --preserve either fails or does a (non-conservative) update by default anyway. last, why wasn't the --preserve enabled by default - maybe due its limits or "hacky" (Bundler pre-checker) nature or conservative updates where simply not considered 😕

wasn't planning to dive deep but maybe a path forward would be to deprecate the --preserve flag or just have 2 flags anyway - the important thing being that any install or update behaves conservatively by default - this would be a desired behavior to have at some point in LS.

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

very nice change, LGTM

@kares kares merged commit 67fd99b into elastic:main Feb 21, 2022
kares added a commit to kares/logstash that referenced this pull request Feb 21, 2022
@kares kares added the v8.1.0 label Feb 21, 2022
kares added a commit to kares/logstash that referenced this pull request Feb 21, 2022
@kares kares added v7.17.2 and removed v7.17.1 labels Feb 28, 2022
kaisecheng pushed a commit that referenced this pull request Feb 28, 2022
andsel pushed a commit that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin Manager is too optimisitic about dependencies when installing unrelated plugins
4 participants