add --preserve option to plugin install #5224
Conversation
| @@ -90,7 +91,13 @@ def install_gems_list!(install_list) | |||
|
|
|||
| # Add plugins/gems to the current gemfile | |||
| puts("Installing" + (install_list.empty? ? "..." : " " + install_list.collect(&:first).join(", "))) | |||
| install_list.each { |plugin, version, options| gemfile.update(plugin, version, options) } | |||
| install_list.each do |plugin, version, options| | |||
| if preserve? | |||
There was a problem hiding this comment.
maybe add a debug log message here to make the behavior known on a per plugin basis?
There was a problem hiding this comment.
yap, added!
|
what about having a few test for this in the great integration test that @ph created? or might be add some checkups as unit test in https://github.com/elastic/logstash/tree/master/spec/pluginmanager ? what do you think? |
|
but in general I do like this option!!! good work! |
colinsurprenant
force-pushed
the
feature/plugin_install_preserve
branch
from
c4dd96a to
8905840
Compare
|
@purbon the integration tests in place for the end-user usage should be sufficient since the |
| install_list.each do |plugin, version, options| | ||
| if preserve? | ||
| plugin_gem = gemfile.find(plugin) | ||
| puts("Preserving Gemfile gem options for plugin #{plugin}") if plugin_gem && !plugin_gem.options.empty? |
There was a problem hiding this comment.
I think its would be cool to report the options in the logger.
There was a problem hiding this comment.
there is not logger instance in the plugin manager
There was a problem hiding this comment.
err I should have said, it would be cool to report what the options this plugins is currently using the the puts output.
There was a problem hiding this comment.
ahh yes. I'd prefer not too because the installed gem option are not just discarded in favour of the options in the Gemfile, they are actually merged in the Gemfile options but at this layer, we don't know yet the result of that merge. Very practically, I don't think the plugins manager ever pass options to install just yet so it can be assumed today that the current options in the Gemfile will be the final options but that could eventually not be the case ... so I don't think it's really worth investing too much time on this now, if we think it's really valuable we can take another look at that point?
There was a problem hiding this comment.
we agreed on zoom to not add this.
|
OK, I did some initial testing with the command line with this PR, work great so far. I would still prefer to have specific test for it like @purbon mentioned but they can be done in another PR. Code LGTM. |
|
ok, will open followup issue for integration tests. |
add missing overwrite method and preserve @gems_by_name when updating log options preservation discards no constrains constrains rewording
colinsurprenant
force-pushed
the
feature/plugin_install_preserve
branch
from
40089b3 to
ccefeae
Compare
|
rebased and merged into master, 5.0 and 2.x |
… logstash will start up up; use --preserve flag implemented in elastic/logstash#5224
… logstash will start up up; use --preserve flag implemented in elastic/logstash#5224
…h will start up up; use --preserve flag implemented in elastic/logstash#5224
Allow using the
--preserveoption to the plugin install command to preserve any current gem options in theGemfilefor the installation of a plugin gem which is already in theGemfile.This is required by #5170 to support specifying plugin gems from a github repo/branch in the
Gemfile. I believe this will also help avoiding the unnecessary publication of core plugins snapshots.Ultimately this allows running the rake test:install* command by keeping current
Gemfilegems options, for example a custom in-dev:pathoption, instead of overwriting them and forcing the use of the published gem.