From 2624122f6b17810c344faa2f465e6809159b5ed1 Mon Sep 17 00:00:00 2001
From: Simon Aquino <simon.aquino@digital.cabinet-office.gov.uk>
Date: Thu, 12 Mar 2015 15:00:57 +0000
Subject: [PATCH 1/6] (PUP-4191) Use command(:gemcmd) to retrieve cmd path in
 gem.rb uninstall

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.
---
 lib/puppet/provider/package/gem.rb | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/puppet/provider/package/gem.rb b/lib/puppet/provider/package/gem.rb
index 7cd61f4f47a..301cfd2558c 100644
--- a/lib/puppet/provider/package/gem.rb
+++ b/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]
+    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
 
   def update

From 8ce98eb2aa6d69c27c4b5815021b8c0316711c06 Mon Sep 17 00:00:00 2001
From: Simon Aquino <simon.aquino@digital.cabinet-office.gov.uk>
Date: Fri, 13 Mar 2015 10:32:00 +0000
Subject: [PATCH 2/6] (PUP-4191) Expand the gem uninstall flags for readability

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.
---
 lib/puppet/provider/package/gem.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/puppet/provider/package/gem.rb b/lib/puppet/provider/package/gem.rb
index 301cfd2558c..75f7c2a192f 100644
--- a/lib/puppet/provider/package/gem.rb
+++ b/lib/puppet/provider/package/gem.rb
@@ -123,7 +123,7 @@ def query
 
   def uninstall
     command = [command(:gemcmd), "uninstall"]
-    command << "-x" << "-a" << resource[:name]
+    command << "--executables" << "--all" << resource[:name]
     output = execute(command)
     
     # Apparently some stupid gem versions don't exit non-0 on failure

From d7a34f40165133d12a54e56aa34f032a43a683bf Mon Sep 17 00:00:00 2001
From: Simon Aquino <simon.aquino@digital.cabinet-office.gov.uk>
Date: Fri, 13 Mar 2015 15:46:39 +0000
Subject: [PATCH 3/6] (PUP-4191) Segregate the gem_spec tests in an rspec
 context

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.
---
 spec/unit/provider/package/gem_spec.rb | 284 +++++++++++++------------
 1 file changed, 143 insertions(+), 141 deletions(-)

diff --git a/spec/unit/provider/package/gem_spec.rb b/spec/unit/provider/package/gem_spec.rb
index ecab6c908a6..7157b59344f 100755
--- a/spec/unit/provider/package/gem_spec.rb
+++ b/spec/unit/provider/package/gem_spec.rb
@@ -3,184 +3,186 @@
 
 provider_class = Puppet::Type.type(:package).provider(:gem)
 
-describe provider_class do
-  let(:resource) do
-    Puppet::Type.type(:package).new(
-      :name     => 'myresource',
-      :ensure   => :installed
-    )
-  end
-
-  let(:provider) do
-    provider = provider_class.new
-    provider.resource = resource
-    provider
-  end
-
-  before :each do
-    resource.provider = provider
-  end
-
-  describe "when installing" do
-    it "should use the path to the gem" do
-      provider_class.stubs(:command).with(:gemcmd).returns "/my/gem"
-      provider.expects(:execute).with { |args| args[0] == "/my/gem" }.returns ""
-      provider.install
+context 'installing myresource' do
+  describe provider_class do
+    let(:resource) do
+      Puppet::Type.type(:package).new(
+        :name     => 'myresource',
+        :ensure   => :installed
+      )
     end
 
-    it "should specify that the gem is being installed" do
-      provider.expects(:execute).with { |args| args[1] == "install" }.returns ""
-      provider.install
+    let(:provider) do
+      provider = provider_class.new
+      provider.resource = resource
+      provider
     end
 
-    it "should specify that documentation should not be included" do
-      provider.expects(:execute).with { |args| args[2] == "--no-rdoc" }.returns ""
-      provider.install
+    before :each do
+      resource.provider = provider
     end
 
-    it "should specify that RI should not be included" do
-      provider.expects(:execute).with { |args| args[3] == "--no-ri" }.returns ""
-      provider.install
-    end
+    describe "when installing" do
+      it "should use the path to the gem" do
+        provider_class.stubs(:command).with(:gemcmd).returns "/my/gem"
+        provider.expects(:execute).with { |args| args[0] == "/my/gem" }.returns ""
+        provider.install
+      end
 
-    it "should specify the package name" do
-      provider.expects(:execute).with { |args| args[4] == "myresource" }.returns ""
-      provider.install
-    end
+      it "should specify that the gem is being installed" do
+        provider.expects(:execute).with { |args| args[1] == "install" }.returns ""
+        provider.install
+      end
 
-    it "should not append install_options by default" do
-      provider.expects(:execute).with { |args| args.length == 5 }.returns ""
-      provider.install
-    end
+      it "should specify that documentation should not be included" do
+        provider.expects(:execute).with { |args| args[2] == "--no-rdoc" }.returns ""
+        provider.install
+      end
 
-    it "should allow setting an install_options parameter" do
-      resource[:install_options] = [ '--force', {'--bindir' => '/usr/bin' } ]
-      provider.expects(:execute).with { |args| args[5] == '--force' && args[6] == '--bindir=/usr/bin' }.returns ''
-      provider.install
-    end
+      it "should specify that RI should not be included" do
+        provider.expects(:execute).with { |args| args[3] == "--no-ri" }.returns ""
+        provider.install
+      end
 
-    describe "when a source is specified" do
-      describe "as a normal file" do
-        it "should use the file name instead of the gem name" do
-          resource[:source] = "/my/file"
-          provider.expects(:execute).with { |args| args[2] == "/my/file" }.returns ""
-          provider.install
-        end
+      it "should specify the package name" do
+        provider.expects(:execute).with { |args| args[4] == "myresource" }.returns ""
+        provider.install
       end
-      describe "as a file url" do
-        it "should use the file name instead of the gem name" do
-          resource[:source] = "file:///my/file"
-          provider.expects(:execute).with { |args| args[2] == "/my/file" }.returns ""
-          provider.install
-        end
+
+      it "should not append install_options by default" do
+        provider.expects(:execute).with { |args| args.length == 5 }.returns ""
+        provider.install
       end
-      describe "as a puppet url" do
-        it "should fail" do
-          resource[:source] = "puppet://my/file"
-          expect { provider.install }.to raise_error(Puppet::Error)
-        end
+
+      it "should allow setting an install_options parameter" do
+        resource[:install_options] = [ '--force', {'--bindir' => '/usr/bin' } ]
+        provider.expects(:execute).with { |args| args[5] == '--force' && args[6] == '--bindir=/usr/bin' }.returns ''
+        provider.install
       end
-      describe "as a non-file and non-puppet url" do
-        it "should treat the source as a gem repository" do
-          resource[:source] = "http://host/my/file"
-          provider.expects(:execute).with { |args| args[2..4] == ["--source", "http://host/my/file", "myresource"] }.returns ""
-          provider.install
+
+      describe "when a source is specified" do
+        describe "as a normal file" do
+          it "should use the file name instead of the gem name" do
+            resource[:source] = "/my/file"
+            provider.expects(:execute).with { |args| args[2] == "/my/file" }.returns ""
+            provider.install
+          end
         end
-      end
-      describe "with an invalid uri" do
-        it "should fail" do
-          URI.expects(:parse).raises(ArgumentError)
-          resource[:source] = "http:::::uppet:/:/my/file"
-          expect { provider.install }.to raise_error(Puppet::Error)
+        describe "as a file url" do
+          it "should use the file name instead of the gem name" do
+            resource[:source] = "file:///my/file"
+            provider.expects(:execute).with { |args| args[2] == "/my/file" }.returns ""
+            provider.install
+          end
+        end
+        describe "as a puppet url" do
+          it "should fail" do
+            resource[:source] = "puppet://my/file"
+            expect { provider.install }.to raise_error(Puppet::Error)
+          end
+        end
+        describe "as a non-file and non-puppet url" do
+          it "should treat the source as a gem repository" do
+            resource[:source] = "http://host/my/file"
+            provider.expects(:execute).with { |args| args[2..4] == ["--source", "http://host/my/file", "myresource"] }.returns ""
+            provider.install
+          end
+        end
+        describe "with an invalid uri" do
+          it "should fail" do
+            URI.expects(:parse).raises(ArgumentError)
+            resource[:source] = "http:::::uppet:/:/my/file"
+            expect { provider.install }.to raise_error(Puppet::Error)
+          end
         end
       end
     end
-  end
 
-  describe "#latest" do
-    it "should return a single value for 'latest'" do
-      #gemlist is used for retrieving both local and remote version numbers, and there are cases
-      # (particularly local) where it makes sense for it to return an array.  That doesn't make
-      # sense for '#latest', though.
-      provider.class.expects(:gemlist).with({ :justme => 'myresource'}).returns({
+    describe "#latest" do
+      it "should return a single value for 'latest'" do
+        #gemlist is used for retrieving both local and remote version numbers, and there are cases
+        # (particularly local) where it makes sense for it to return an array.  That doesn't make
+        # sense for '#latest', though.
+        provider.class.expects(:gemlist).with({ :justme => 'myresource'}).returns({
           :name     => 'myresource',
           :ensure   => ["3.0"],
           :provider => :gem,
-          })
-      expect(provider.latest).to eq("3.0")
-    end
+        })
+        expect(provider.latest).to eq("3.0")
+      end
 
-    it "should list from the specified source repository" do
-      resource[:source] = "http://foo.bar.baz/gems"
-      provider.class.expects(:gemlist).
-        with({:justme => 'myresource', :source => "http://foo.bar.baz/gems"}).
-        returns({
-          :name     => 'myresource',
-          :ensure   => ["3.0"],
-          :provider => :gem,
+      it "should list from the specified source repository" do
+        resource[:source] = "http://foo.bar.baz/gems"
+        provider.class.expects(:gemlist).
+          with({:justme => 'myresource', :source => "http://foo.bar.baz/gems"}).
+          returns({
+            :name     => 'myresource',
+            :ensure   => ["3.0"],
+            :provider => :gem,
           })
-      expect(provider.latest).to eq("3.0")
+          expect(provider.latest).to eq("3.0")
+      end
     end
-  end
 
-  describe "#instances" do
-    before do
-      provider_class.stubs(:command).with(:gemcmd).returns "/my/gem"
-    end
+    describe "#instances" do
+      before do
+        provider_class.stubs(:command).with(:gemcmd).returns "/my/gem"
+      end
 
-    it "should return an empty array when no gems installed" do
-      provider_class.expects(:execute).with(%w{/my/gem list --local}).returns("\n")
-      expect(provider_class.instances).to eq([])
-    end
+      it "should return an empty array when no gems installed" do
+        provider_class.expects(:execute).with(%w{/my/gem list --local}).returns("\n")
+        expect(provider_class.instances).to eq([])
+      end
 
-    it "should return ensure values as an array of installed versions" do
-      provider_class.expects(:execute).with(%w{/my/gem list --local}).returns <<-HEREDOC.gsub(/        /, '')
+      it "should return ensure values as an array of installed versions" do
+        provider_class.expects(:execute).with(%w{/my/gem list --local}).returns <<-HEREDOC.gsub(/        /, '')
         systemu (1.2.0)
         vagrant (0.8.7, 0.6.9)
-      HEREDOC
+        HEREDOC
 
-      expect(provider_class.instances.map {|p| p.properties}).to eq([
-        {:ensure => ["1.2.0"],          :provider => :gem, :name => 'systemu'},
-        {:ensure => ["0.8.7", "0.6.9"], :provider => :gem, :name => 'vagrant'}
-      ])
-    end
+        expect(provider_class.instances.map {|p| p.properties}).to eq([
+          {:ensure => ["1.2.0"],          :provider => :gem, :name => 'systemu'},
+          {:ensure => ["0.8.7", "0.6.9"], :provider => :gem, :name => 'vagrant'}
+        ])
+      end
 
-    it "should ignore platform specifications" do
-      provider_class.expects(:execute).with(%w{/my/gem list --local}).returns <<-HEREDOC.gsub(/        /, '')
+      it "should ignore platform specifications" do
+        provider_class.expects(:execute).with(%w{/my/gem list --local}).returns <<-HEREDOC.gsub(/        /, '')
         systemu (1.2.0)
         nokogiri (1.6.1 ruby java x86-mingw32 x86-mswin32-60, 1.4.4.1 x86-mswin32)
-      HEREDOC
+        HEREDOC
 
-      expect(provider_class.instances.map {|p| p.properties}).to eq([
-        {:ensure => ["1.2.0"],          :provider => :gem, :name => 'systemu'},
-        {:ensure => ["1.6.1", "1.4.4.1"], :provider => :gem, :name => 'nokogiri'}
-      ])
-    end
+        expect(provider_class.instances.map {|p| p.properties}).to eq([
+          {:ensure => ["1.2.0"],          :provider => :gem, :name => 'systemu'},
+          {:ensure => ["1.6.1", "1.4.4.1"], :provider => :gem, :name => 'nokogiri'}
+        ])
+      end
 
-    it "should not fail when an unmatched line is returned" do
-      provider_class.expects(:execute).with(%w{/my/gem list --local}).
-        returns(File.read(my_fixture('line-with-1.8.5-warning')))
-
-      expect(provider_class.instances.map {|p| p.properties}).
-        to eq([{:provider=>:gem, :ensure=>["0.3.2"], :name=>"columnize"},
-                   {:provider=>:gem, :ensure=>["1.1.3"], :name=>"diff-lcs"},
-                   {:provider=>:gem, :ensure=>["0.0.1"], :name=>"metaclass"},
-                   {:provider=>:gem, :ensure=>["0.10.5"], :name=>"mocha"},
-                   {:provider=>:gem, :ensure=>["0.8.7"], :name=>"rake"},
-                   {:provider=>:gem, :ensure=>["2.9.0"], :name=>"rspec-core"},
-                   {:provider=>:gem, :ensure=>["2.9.1"], :name=>"rspec-expectations"},
-                   {:provider=>:gem, :ensure=>["2.9.0"], :name=>"rspec-mocks"},
-                   {:provider=>:gem, :ensure=>["0.9.0"], :name=>"rubygems-bundler"},
-                   {:provider=>:gem, :ensure=>["1.11.3.3"], :name=>"rvm"}])
+      it "should not fail when an unmatched line is returned" do
+        provider_class.expects(:execute).with(%w{/my/gem list --local}).
+          returns(File.read(my_fixture('line-with-1.8.5-warning')))
+
+        expect(provider_class.instances.map {|p| p.properties}).
+          to eq([{:provider=>:gem, :ensure=>["0.3.2"], :name=>"columnize"},
+                 {:provider=>:gem, :ensure=>["1.1.3"], :name=>"diff-lcs"},
+                 {:provider=>:gem, :ensure=>["0.0.1"], :name=>"metaclass"},
+                 {:provider=>:gem, :ensure=>["0.10.5"], :name=>"mocha"},
+                 {:provider=>:gem, :ensure=>["0.8.7"], :name=>"rake"},
+                 {:provider=>:gem, :ensure=>["2.9.0"], :name=>"rspec-core"},
+                 {:provider=>:gem, :ensure=>["2.9.1"], :name=>"rspec-expectations"},
+                 {:provider=>:gem, :ensure=>["2.9.0"], :name=>"rspec-mocks"},
+                 {:provider=>:gem, :ensure=>["0.9.0"], :name=>"rubygems-bundler"},
+                 {:provider=>:gem, :ensure=>["1.11.3.3"], :name=>"rvm"}])
+      end
     end
-  end
 
-  describe "listing gems" do
-    describe "searching for a single package" do
-      it "searches for an exact match" do
-        provider_class.expects(:execute).with(includes('^bundler$')).returns(File.read(my_fixture('gem-list-single-package')))
-        expected = {:name => 'bundler', :ensure => %w[1.6.2], :provider => :gem}
-        expect(provider_class.gemlist({:justme => 'bundler'})).to eq(expected)
+    describe "listing gems" do
+      describe "searching for a single package" do
+        it "searches for an exact match" do
+          provider_class.expects(:execute).with(includes('^bundler$')).returns(File.read(my_fixture('gem-list-single-package')))
+          expected = {:name => 'bundler', :ensure => %w[1.6.2], :provider => :gem}
+          expect(provider_class.gemlist({:justme => 'bundler'})).to eq(expected)
+        end
       end
     end
   end

From 01e8906de1e18a076919cfce75021c85fce95899 Mon Sep 17 00:00:00 2001
From: Simon Aquino <simon.aquino@digital.cabinet-office.gov.uk>
Date: Fri, 13 Mar 2015 16:04:55 +0000
Subject: [PATCH 4/6] (PUP-4191) Add unit tests for the uninstall method in
 gem.rb

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.
---
 spec/unit/provider/package/gem_spec.rb | 49 ++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/spec/unit/provider/package/gem_spec.rb b/spec/unit/provider/package/gem_spec.rb
index 7157b59344f..1f348d37396 100755
--- a/spec/unit/provider/package/gem_spec.rb
+++ b/spec/unit/provider/package/gem_spec.rb
@@ -187,3 +187,52 @@
     end
   end
 end
+
+context 'uninstalling myresource' do
+  describe provider_class do
+    let(:resource) do
+      Puppet::Type.type(:package).new(
+        :name     => 'myresource',
+        :ensure   => :absent
+      )
+    end
+
+    let(:provider) do
+      provider = provider_class.new
+      provider.resource = resource
+      provider
+    end
+
+    before :each do
+      resource.provider = provider
+    end
+
+    describe "when uninstalling" do
+      it "should use the path to the gem" do
+        provider_class.stubs(:command).with(:gemcmd).returns "/my/gem"
+        provider.expects(:execute).with { |args| args[0] == "/my/gem" }.returns ""
+        provider.uninstall
+      end
+
+      it "should specify that the gem is being uninstalled" do
+        provider.expects(:execute).with { |args| args[1] == "uninstall" }.returns ""
+        provider.uninstall
+      end
+
+      it "should specify that the relevant executables should be removed without confirmation" do
+        provider.expects(:execute).with { |args| args[2] == "--executables" }.returns ""
+        provider.uninstall
+      end
+
+      it "should specify that all the matching versions should be removed" do
+        provider.expects(:execute).with { |args| args[3] == "--all" }.returns ""
+        provider.uninstall
+      end
+
+      it "should specify the package name" do
+        provider.expects(:execute).with { |args| args[4] == "myresource" }.returns ""
+        provider.uninstall
+      end
+    end
+  end
+end

From 410dc57fc76c79553600364514ca9d4fd194e326 Mon Sep 17 00:00:00 2001
From: Simon Aquino <simon.aquino@digital.cabinet-office.gov.uk>
Date: Fri, 13 Mar 2015 11:04:16 +0000
Subject: [PATCH 5/6] (PUP-4203) Introduce uninstall options in the gem
 provider

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.
---
 lib/puppet/provider/package/gem.rb | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/lib/puppet/provider/package/gem.rb b/lib/puppet/provider/package/gem.rb
index 75f7c2a192f..30182593c05 100644
--- a/lib/puppet/provider/package/gem.rb
+++ b/lib/puppet/provider/package/gem.rb
@@ -8,11 +8,12 @@
     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` attributes,
+    which allow 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,6 +125,9 @@ def query
   def uninstall
     command = [command(:gemcmd), "uninstall"]
     command << "--executables" << "--all" << resource[:name]
+
+    command += uninstall_options if resource[:uninstall_options]
+ 
     output = execute(command)
     
     # Apparently some stupid gem versions don't exit non-0 on failure
@@ -137,4 +141,8 @@ def update
   def install_options
     join_options(resource[:install_options])
   end
+
+  def uninstall_options
+    join_options(resource[:uninstall_options])
+  end
 end

From dd21713464a625a3ea042c77330b1ef5ec138dbe Mon Sep 17 00:00:00 2001
From: Simon Aquino <simon.aquino@digital.cabinet-office.gov.uk>
Date: Fri, 13 Mar 2015 22:31:33 +0000
Subject: [PATCH 6/6] (PUP-4203) Add unit tests for gem.rb uninstall_options

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.
---
 spec/unit/provider/package/gem_spec.rb | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/spec/unit/provider/package/gem_spec.rb b/spec/unit/provider/package/gem_spec.rb
index 1f348d37396..5f1841febaf 100755
--- a/spec/unit/provider/package/gem_spec.rb
+++ b/spec/unit/provider/package/gem_spec.rb
@@ -233,6 +233,17 @@
         provider.expects(:execute).with { |args| args[4] == "myresource" }.returns ""
         provider.uninstall
       end
+
+      it "should not append uninstall_options by default" do
+        provider.expects(:execute).with { |args| args.length == 5 }.returns ""
+        provider.uninstall
+      end
+
+      it "should allow setting an uninstall_options parameter" do
+        resource[:uninstall_options] = [ '--ignore-dependencies', {'--version' => '0.1.1' } ]
+        provider.expects(:execute).with { |args| args[5] == '--ignore-dependencies' && args[6] == '--version=0.1.1' }.returns ''
+        provider.uninstall
+      end
     end
   end
 end