(PUP-4191,PUP-4203) Use execute function to run gem uninstall #3708

queeno · 2015-03-12T15:58:42Z

When attempting to uninstall a gem using a custom gem provider inheriting gem.rb, the wrong gemcmd is issued.

For example, the sensu-puppet module introduces embedded ruby gem support through a custom sensu_gem provider, inheriting puppet's gem.rb.

In sensu-gem, the :gemcmd is declared to match the embedded sensu gem path:
commands :gemcmd => "/opt/sensu/embedded/bin/gem"

When the following resource is run:

package { 'sensu-plugin':
  ensure   => absent,
  provider => 'sensu_gem',
}

/usr/bin/gem uninstall -x -a sensu-plugin is executed rather than
/opt/sensu/embedded/bin/gem uninstall -x -a sensu-plugin.

I noticed that in gem.rb the install and self.gemlist methods build a command array and pass it to the execute function. This results in using the right gemcmd.

This PR makes the uninstall method behave just like the self.gemlist and install methods.
An array of arguments is built and passed to the execute function, which runs the right command and returns the output. Since some versions of gem may return exit code 0 on error, a hard failure is triggered if the output contains the word 'ERROR'. This is compliant with the install method behaviour.

puppetcla · 2015-03-12T17:00:18Z

Waiting for CLA signature by @queeno

@queeno - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/

Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html

queeno · 2015-03-12T17:30:11Z

@puppetcla CLA signed.

puppetcla · 2015-03-12T18:00:18Z

CLA signed by all contributors.

melissa · 2015-03-12T18:37:45Z

lib/puppet/provider/package/gem.rb

@@ -122,7 +122,12 @@ def query
  end

  def uninstall
-    gemcmd "uninstall", "-x", "-a", resource[:name]
+    command = [command(:gemcmd), "uninstall"]
+    command << "-x" << "-a" << resource[:name]


Could you expand these flags to make the command more clear? (--executables and --all)

melissa · 2015-03-12T18:42:43Z

@queeno thank you for your contribution!

Could you add tests to verify this behavior?

melissa · 2015-03-12T19:42:57Z

I'm also not sure if this is the place to bring this up, but this might be a good opportunity to add in uninstall_options to this provider. Something along the lines of

diff --git a/lib/puppet/provider/package/gem.rb b/lib/puppet/provider/package/gem.rb
index 301cfd2..ebb88f0 100644
--- a/lib/puppet/provider/package/gem.rb
+++ b/lib/puppet/provider/package/gem.rb
@@ -8,11 +8,12 @@ Puppet::Type.type(:package).provide :gem, :parent => Puppet::Provider::Package d
     interpreted as the path to a local gem file.  If source is not present at all,
     the gem will be installed from the default gem repositories.

-    This provider supports the `install_options` attribute, which allows command-line flags to be passed to the gem command.
+    This provider supports the `install_options` and `uninstall_options` attribute, which
+    allows command-line flags to be passed to the gem command.
     These options should be specified as a string (e.g. '--flag'), a hash (e.g. {'--flag' => 'value'}),
     or an array where each element is either a string or a hash."

-  has_feature :versionable, :install_options
+  has_feature :versionable, :install_options, :uninstall_options

   commands :gemcmd => "gem"

@@ -124,8 +125,10 @@ Puppet::Type.type(:package).provide :gem, :parent => Puppet::Provider::Package d
   def uninstall
     command = [command(:gemcmd), "uninstall"]
     command << "-x" << "-a" << resource[:name]
+
+    command += uninstall_options if resource[:uninstall_options]
     output = execute(command)
-
+
     # Apparently some stupid gem versions don't exit non-0 on failure
     self.fail "Could not uninstall: #{output.chomp}" if output.include?("ERROR")
   end
@@ -137,4 +140,8 @@ Puppet::Type.type(:package).provide :gem, :parent => Puppet::Provider::Package d
   def install_options
     join_options(resource[:install_options])
   end
+
+  def uninstall_options
+    join_options(resource[:uninstall_options])
+  end
 end

queeno · 2015-03-12T22:21:35Z

No problem @melissa, it's a pleasure contributing to such an amazing product.

I think all your suggestions make sense, will work on them tomorrow morning 😄

joshcooper · 2015-03-13T06:17:34Z

lib/puppet/provider/package/gem.rb

@@ -122,7 +122,12 @@ def query
  end

  def uninstall
-    gemcmd "uninstall", "-x", "-a", resource[:name]
+    command = [command(:gemcmd), "uninstall"]


It is unfortunate that you have to do this... When you declare a command in your provider, puppet automatically creates a method, e.g. gemcmd, which ultimately calls into Puppet::Util::Execution.execute to execute the command. Unfortunately, it appears sensu_gem is able to override the value of command(:gem), but not the path used in gemcmd. So it's surprising when you use one vs the other:

0 ~/work/puppet (master) $ bundle exec irb ... irb(main):012:0> require 'puppet' => true irb(main):013:0> Puppet::Type.type(:package).provide :sensu_gem, :parent => :gem do irb(main):014:1* desc "Sensu Embedded Ruby Gem support. If a URL is passed via `source`, then irb(main):015:1" that URL is used as the remote gem repository; if a source is present but is irb(main):016:1" not a valid URL, it will be interpreted as the path to a local gem file. If irb(main):017:1" source is not present at all, the gem will be installed from the default gem irb(main):018:1" repositories." irb(main):019:1> irb(main):020:1* has_feature :versionable, :install_options irb(main):021:1> irb(main):022:1* commands :gemcmd => "/opt/sensu/embedded/bin/gem" irb(main):023:1> end => Puppet::Type::Package::ProviderSensu_gem ... irb(main):028:0> provider = Puppet::Type.type(:package).provider(:sensu_gem) => Puppet::Type::Package::ProviderSensu_gem irb(main):029:0> provider.command(:gemcmd) => nil

This is because I don't have /opt/sensu/embedded/bin/gem, so this is returning the overridden value as expected. But

irb(main):030:0> provider.gemcmd Puppet::ExecutionFailure: Execution of '/Users/josh/.rbenv/versions/2.1.5/bin/gem' returned 1: RubyGems is a sophisticated package manager for Ruby.

Is executing using the gem command from the base class.

So I'm 👍 with the changes, just sad puppet is not consistent in how commands are overridden.

@joshcooper I totally agree with you.

queeno · 2015-03-13T20:24:19Z

@melissa I believe I addressed all the issues raised.
I introduced uninstalled_options and I expanded the gem.rb unit tests, so we now test for uninstalling gems as well.

kylog · 2015-03-13T21:26:08Z

This all seems like goodness to me (thanks @queeno!), but this feels like two different tickets/PRs:

fix uninstall
add uninstall_options
I think that separation will be clearer to someone who bumps into the 'gem uninstall' bug, since they would only be interested in the bugfix addressed in the original commit of this PR.

Thoughts?

melissa · 2015-03-13T21:48:51Z

@kylog would separate commits be acceptable? It's a nice addition to have, even if it isn't necessarily related to the original topic of this PR.

kylog · 2015-03-13T21:55:25Z

Yes, definitely!

Sorry I wasn't clear. Both the bug fix and the feature add of install_options are definitely nice additions.

However, IMHO it's clearer to a future reader (e.g. someone who is interested in just the bugfix or just the addition of install_options) if there are two tickets and if each commit maps to just one of the two tickets. (I don't really care if it's separate PRs although that's what I suggested above.)

Does that make any sense? It's Friday afternoon so I may be uttering gibberish ;)

melissa · 2015-03-13T22:08:42Z

Cool! My bad for tacking things on. @queeno apologies for giving you the extra work. Feel free to take the :uninstall_options work out if you want to. I can add that in later in that case. Otherwise, if you want to reference https://tickets.puppetlabs.com/browse/PUP-4203 for that work, that'd be awesome!

queeno · 2015-03-13T22:22:45Z

@kylog I know exactly how that feels. It's Friday night here in London and I've already gone into the next phase: I'm totally out of control 😛

I wouldn't mind opening a new PR for the install_options addition, however I would then need to rebase it once this PR is merged. I quite agree with @melissa, let's keep this nice and simple in this PR.

Thanks @melissa for opening a new issue in Puppet Jira for me. Will make the necessary changes in my branch and report back.

queeno · 2015-03-13T22:55:33Z

@kylog @melissa all is well 👍
I've updated the commits and the relative jira issue PUP-4203.

Fingers crossed for the Travis build!

If you can't spot anything else, this PR should be good to go 😄

kylog · 2015-03-14T17:14:25Z

Last nit: can you put your commit messages in the imperative? (https://github.com/puppetlabs/puppet/blame/master/CONTRIBUTING.md#L53 for example/rationale.)

Other than that I'm 👍

Thanks!!

The gemcmd method in gem.rb uninstall runs the wrong command when invoked by a custom provider, which also overrides the gemcmd path. For this reason, command(:gemcmd) is used to retrieve the right path, which is then passed to the execute method with the relevant parameters for execution.

The gem uninstall flags (-x, -a) used in the gem.rb uninstall method do not inform the user about their behaviour. This commit expands their names for readibility purposes.

The gem_spec unit test does not allow tests for scenarios other than installing gems. This commit segregates the tests in an rspec context in order to tests for gem removal.

No unit tests are in place to check the uninstall behaviour of the gem provider. This commit adds unit tests to verify the gem.rb uninstall method functionality.

A user cannot pass extra flags to the gem provider in order to uninstall ruby gems. The uninstall method in the gem provider now consumes the uninstall_options attribute of the package resource, which may be used to specify extra flags to uninstall ruby gems.

queeno · 2015-03-14T21:40:57Z

No problem @kylog!
I've just rewritten my commit messages, hope they're compliant with the guidelines now 😄

kylog · 2015-03-14T22:02:03Z

One last imperative: can you amend 78f8308 so it's first line reads "(PUP-4203) Add unit tests for gem.rb uninstall_options"? Thanks!

Btw, timing wise I should mention that we're holding off on merges for a couple days while we tidy up our initial 4.0.0 release candidate. But after we open master back up for merges, this looks good. Thanks!!

The newly introduced uninstall_option functionality in the gem provider uninstall method is untested. Unit tests have been added in order to test this new functionality.

queeno · 2015-03-14T23:10:37Z

Ooops... no problem! 😄
All done and Travis passed! Please let me know if you spot anything else.

Thanks @kylog for your time reviewing this PR!

kylog · 2015-03-18T17:27:24Z

👍

melissa · 2015-03-30T18:35:44Z

Merged in at a018038

MikaelSmith · 2015-03-30T18:37:35Z

We manually merged with git rebase master --whitespace=fix to clean up some trailing whitespaces.

This was referenced Mar 12, 2015

Fix #318: Introducing custom uninstall in sensu_gem sensu/sensu-puppet#332

Merged

New configuration: gem uninstall sensu-plugin is failing in 1.5.0 sensu/sensu-puppet#318

Closed

melissa reviewed Mar 12, 2015
View reviewed changes

joshcooper reviewed Mar 13, 2015
View reviewed changes

queeno force-pushed the use_execute_function_to_run_gem_uninstall branch from 228bd8b to 44ab6b2 Compare March 13, 2015 22:36

queeno changed the title ~~(PUP-4191) Use execute function to run gem uninstall~~ (PUP-4191,PUP-4203) Use execute function to run gem uninstall Mar 13, 2015

queeno force-pushed the use_execute_function_to_run_gem_uninstall branch from 44ab6b2 to d8f23dd Compare March 14, 2015 21:32

queeno added 5 commits March 14, 2015 21:34

(PUP-4191) Expand the gem uninstall flags for readability

8ce98eb

The gem uninstall flags (-x, -a) used in the gem.rb uninstall method do not inform the user about their behaviour. This commit expands their names for readibility purposes.

(PUP-4191) Segregate the gem_spec tests in an rspec context

d7a34f4

The gem_spec unit test does not allow tests for scenarios other than installing gems. This commit segregates the tests in an rspec context in order to tests for gem removal.

(PUP-4191) Add unit tests for the uninstall method in gem.rb

01e8906

No unit tests are in place to check the uninstall behaviour of the gem provider. This commit adds unit tests to verify the gem.rb uninstall method functionality.

queeno force-pushed the use_execute_function_to_run_gem_uninstall branch from d8f23dd to 78f8308 Compare March 14, 2015 21:37

(PUP-4203) Add unit tests for gem.rb uninstall_options

dd21713

The newly introduced uninstall_option functionality in the gem provider uninstall method is untested. Unit tests have been added in order to test this new functionality.

queeno force-pushed the use_execute_function_to_run_gem_uninstall branch from 78f8308 to dd21713 Compare March 14, 2015 23:06

branan added the Client label Mar 17, 2015

kylog mentioned this pull request Mar 18, 2015

add-puppet-gem-provider-for-puppet-agent #3728

Closed

melissa added Merge After Unfreeze labels Mar 18, 2015

branan added Merge After Unfreeze and removed Merge After Unfreeze labels Mar 26, 2015

melissa closed this Mar 30, 2015

branan mentioned this pull request Mar 31, 2015

(PUP-4237) Add puppet-agent gem provider #3777

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PUP-4191,PUP-4203) Use execute function to run gem uninstall #3708

(PUP-4191,PUP-4203) Use execute function to run gem uninstall #3708

queeno commented Mar 12, 2015

puppetcla commented Mar 12, 2015

queeno commented Mar 12, 2015

puppetcla commented Mar 12, 2015

melissa Mar 12, 2015

melissa commented Mar 12, 2015

melissa commented Mar 12, 2015

queeno commented Mar 12, 2015

joshcooper Mar 13, 2015

queeno Mar 13, 2015

queeno commented Mar 13, 2015

kylog commented Mar 13, 2015

melissa commented Mar 13, 2015

kylog commented Mar 13, 2015

melissa commented Mar 13, 2015

queeno commented Mar 13, 2015

queeno commented Mar 13, 2015

kylog commented Mar 14, 2015

queeno commented Mar 14, 2015

kylog commented Mar 14, 2015

queeno commented Mar 14, 2015

kylog commented Mar 18, 2015

melissa commented Mar 30, 2015

MikaelSmith commented Mar 30, 2015

(PUP-4191,PUP-4203) Use execute function to run gem uninstall #3708

(PUP-4191,PUP-4203) Use execute function to run gem uninstall #3708

Conversation

queeno commented Mar 12, 2015

puppetcla commented Mar 12, 2015

queeno commented Mar 12, 2015

puppetcla commented Mar 12, 2015

melissa Mar 12, 2015

Choose a reason for hiding this comment

melissa commented Mar 12, 2015

melissa commented Mar 12, 2015

queeno commented Mar 12, 2015

joshcooper Mar 13, 2015

Choose a reason for hiding this comment

queeno Mar 13, 2015

Choose a reason for hiding this comment

queeno commented Mar 13, 2015

kylog commented Mar 13, 2015

melissa commented Mar 13, 2015

kylog commented Mar 13, 2015

melissa commented Mar 13, 2015

queeno commented Mar 13, 2015

queeno commented Mar 13, 2015

kylog commented Mar 14, 2015

queeno commented Mar 14, 2015

kylog commented Mar 14, 2015

queeno commented Mar 14, 2015

kylog commented Mar 18, 2015

melissa commented Mar 30, 2015

MikaelSmith commented Mar 30, 2015