Please review pull request #397: (#2279) Handle providers with multiple installed versions - ruby gems opened by (mmrobins)
Description:
Ruby gems can have multiple versions installed, and the current
implementation of the gem provider only reads the very latest installed
version when self.instances is called. This leads to Puppet potentially
reinstalling the gem on every run if the Puppet manifest specifies a gem
that is installed, but is not the latest version installed.
This does require a change to how the package type checks if ensure is
insync? to handle multiple versions for the 'is' ensure value, but the
change is backward compatible with the providers that specify single
versions.
- Opened: Tue Jan 24 07:03:03 UTC 2012
- Based on: puppetlabs:2.7.x (ae544365086360cf63b8cd4995052ebb92ba3e66)
- Requested merge: mmrobins:ticket/2.7.x/2279-gem_reinstalls (e415daa44d80c15035bbe837f02134e8776b46bc)
Diff follows:
diff --git a/lib/puppet/provider/package/gem.rb b/lib/puppet/provider/package/gem.rb
index e616e5e..2cd907b 100755
--- a/lib/puppet/provider/package/gem.rb
+++ b/lib/puppet/provider/package/gem.rb
@@ -12,33 +12,26 @@
commands :gemcmd => "gem"
- def self.gemlist(hash)
- command = [command(:gemcmd), "list"]
+ def self.gemlist(options)
+ gem_list_command = [command(:gemcmd), "list"]
- if hash[:local]
- command << "--local"
+ if options[:local]
+ gem_list_command << "--local"
else
- command << "--remote"
+ gem_list_command << "--remote"
end
- if name = hash[:justme]
- command << name + "$"
+ if name = options[:justme]
+ gem_list_command << name + "$"
end
begin
- list = execute(command).split("\n").collect do |set|
- if gemhash = gemsplit(set)
- gemhash[:provider] = :gem
- gemhash
- else
- nil
- end
- end.compact
+ list = execute(gem_list_command).lines.map {|set| gemsplit(set) }
rescue Puppet::ExecutionFailure => detail
raise Puppet::Error, "Could not list gems: #{detail}"
end
- if hash[:justme]
+ if options[:justme]
return list.shift
else
return list
@@ -46,14 +39,19 @@ def self.gemlist(hash)
end
def self.gemsplit(desc)
- case desc
- when /^\*\*\*/, /^\s*$/, /^\s+/; return nil
- when /^(\S+)\s+\((.+)\)/
+ # `gem list` when output console has a line like:
+ # *** LOCAL GEMS ***
+ # but when it's not to the console that line
+ # and all blank lines are stripped
+ # so we don't need to check for them
+
+ if desc =~ /^(\S+)\s+\((.+)\)/
name = $1
- version = $2.split(/,\s*/)[0]
- return {
- :name => name,
- :ensure => version
+ versions = $2.split(/,\s*/)
+ {
+ :name => name,
+ :ensure => versions,
+ :provider => :gem
}
else
Puppet.warning "Could not match #{desc}"
diff --git a/lib/puppet/type/package.rb b/lib/puppet/type/package.rb
index ed797f5..18ee854 100644
--- a/lib/puppet/type/package.rb
+++ b/lib/puppet/type/package.rb
@@ -115,7 +115,6 @@ module Puppet
# Override the parent method, because we've got all kinds of
# funky definitions of 'in sync'.
def insync?(is)
- @latest ||= nil
@lateststamp ||= (Time.now.to_i - 1000)
# Iterate across all of the should values, and see how they
# turn out.
@@ -156,7 +155,9 @@ def insync?(is)
return true if is == :absent or is == :purged
when :purged
return true if is == :purged
- when is
+ # this handles version number matches and
+ # supports providers that can have multiple versions installed
+ when *Array(is)
return true
end
}
diff --git a/spec/unit/provider/package/gem_spec.rb b/spec/unit/provider/package/gem_spec.rb
index 284e63c..7c3cbde 100755
--- a/spec/unit/provider/package/gem_spec.rb
+++ b/spec/unit/provider/package/gem_spec.rb
@@ -4,93 +4,109 @@
provider_class = Puppet::Type.type(:package).provider(:gem)
describe provider_class do
- it "should have an install method" do
- @provider = provider_class.new
- @provider.should respond_to(:install)
+ let(:resource) do
+ Puppet::Type.type(:package).new(
+ :name => 'myresource',
+ :ensure => :installed
+ )
end
- describe "when installing" do
- before do
- # Create a mock resource
- @resource = stub 'resource'
-
- # A catch all; no parameters set
- @resource.stubs(:[]).returns nil
-
- # We have to set a name, though
- @resource.stubs(:[]).with(:name).returns "myresource"
- @resource.stubs(:[]).with(:ensure).returns :installed
-
- @provider = provider_class.new
- @provider.stubs(:resource).returns @resource
- end
+ let(:provider) do
+ provider = provider_class.new
+ provider.resource = resource
+ 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
+ provider.expects(:execute).with { |args| args[0] == "/my/gem" }.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
+ provider.expects(:execute).with { |args| args[1] == "install" }.returns ""
+ provider.install
end
it "should specify that dependencies should be included" do
- @provider.expects(:execute).with { |args| args[2] == "--include-dependencies" }.returns ""
- @provider.install
+ provider.expects(:execute).with { |args| args[2] == "--include-dependencies" }.returns ""
+ provider.install
end
it "should specify that documentation should not be included" do
- @provider.expects(:execute).with { |args| args[3] == "--no-rdoc" }.returns ""
- @provider.install
+ provider.expects(:execute).with { |args| args[3] == "--no-rdoc" }.returns ""
+ provider.install
end
it "should specify that RI should not be included" do
- @provider.expects(:execute).with { |args| args[4] == "--no-ri" }.returns ""
- @provider.install
+ provider.expects(:execute).with { |args| args[4] == "--no-ri" }.returns ""
+ provider.install
end
it "should specify the package name" do
- @provider.expects(:execute).with { |args| args[5] == "myresource" }.returns ""
- @provider.install
+ provider.expects(:execute).with { |args| args[5] == "myresource" }.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.stubs(:[]).with(:source).returns "/my/file"
- @provider.expects(:execute).with { |args| args[3] == "/my/file" }.returns ""
- @provider.install
+ resource[:source] = "/my/file"
+ provider.expects(:execute).with { |args| args[3] == "/my/file" }.returns ""
+ provider.install
end
end
describe "as a file url" do
it "should use the file name instead of the gem name" do
- @resource.stubs(:[]).with(:source).returns "file:///my/file"
- @provider.expects(:execute).with { |args| args[3] == "/my/file" }.returns ""
- @provider.install
+ resource[:source] = "file:///my/file"
+ provider.expects(:execute).with { |args| args[3] == "/my/file" }.returns ""
+ provider.install
end
end
describe "as a puppet url" do
it "should fail" do
- @resource.stubs(:[]).with(:source).returns "puppet://my/file"
- lambda { @provider.install }.should raise_error(Puppet::Error)
+ resource[:source] = "puppet://my/file"
+ lambda { provider.install }.should 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.stubs(:[]).with(:source).returns "http://host/my/file"
- @provider.expects(:execute).with { |args| args[3..5] == ["--source", "http://host/my/file", "myresource"] }.returns ""
- @provider.install
+ resource[:source] = "http://host/my/file"
+ provider.expects(:execute).with { |args| args[3..5] == ["--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.stubs(:[]).with(:source).returns "http:::::uppet:/:/my/file"
- lambda { @provider.install }.should raise_error(Puppet::Error)
+ resource[:source] = "http:::::uppet:/:/my/file"
+ lambda { provider.install }.should raise_error(Puppet::Error)
end
end
end
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")
+ provider_class.instances.should == []
+ 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(/ /, '')
+ systemu (1.2.0)
+ vagrant (0.8.7, 0.6.9)
+ HEREDOC
+
+ provider_class.instances.map {|p| p.properties}.should == [
+ {:ensure => ["1.2.0"], :provider => :gem, :name => 'systemu'},
+ {:ensure => ["0.8.7", "0.6.9"], :provider => :gem, :name => 'vagrant'}
+ ]
+ end
+ end
end
diff --git a/spec/unit/type/package_spec.rb b/spec/unit/type/package_spec.rb
index fb7a0f1..921af54 100755
--- a/spec/unit/type/package_spec.rb
+++ b/spec/unit/type/package_spec.rb
@@ -230,6 +230,7 @@ def setprops(properties)
[:purged, :absent].each do |state|
it "should install if it is #{state.to_s}" do
@provider.stubs(:properties).returns(:ensure => state)
+ @package.property(:ensure).insync?(state).should be_false
@provider.expects(:install)
@catalog.apply
end
@@ -237,15 +238,39 @@ def setprops(properties)
it "should do nothing if the current version is equal to the desired version" do
@provider.stubs(:properties).returns(:ensure => "1.0")
+ @package.property(:ensure).insync?('1.0').should be_true
@provider.expects(:install).never
@catalog.apply
end
it "should install if the current version is not equal to the specified version" do
@provider.stubs(:properties).returns(:ensure => "2.0")
+ @package.property(:ensure).insync?('2.0').should be_false
@provider.expects(:install)
@catalog.apply
end
+
+ describe "when current value is an array" do
+ let(:installed_versions) { ["1.0", "2.0", "3.0"] }
+
+ before (:each) do
+ @provider.stubs(:properties).returns(:ensure => installed_versions)
+ end
+
+ it "should install if value not in the array" do
+ @package[:ensure] = "1.5"
+ @package.property(:ensure).insync?(installed_versions).should be_false
+ @provider.expects(:install)
+ @catalog.apply
+ end
+
+ it "should not install if value is in the array" do
+ @package[:ensure] = "2.0"
+ @package.property(:ensure).insync?(installed_versions).should be_true
+ @provider.expects(:install).never
+ @catalog.apply
+ end
+ end
end
end
end
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
