[PATCH/puppet 1/1] Fix #2681 Incorrectly duplicating resources
re...@reductivelabs.com
Ensure that resources whose refs are included in the catalog are
skipped to avoid duplication.
* Refactor to avoid early bailout on resources that cannot be ensured
absent.
* Remove check for managed? in generate
Checking if a resource is managed is unnecessary when checking for its
inclusion in the catalog.
* Add test coverage for Puppet::Type::Resources#generate
Signed-off-by: Rein Henrichs <re...@reinh.com>
---
lib/puppet/type/resources.rb | 46 +++++++++++++++---------------
spec/unit/type/resources.rb | 65 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 88 insertions(+), 23 deletions(-)
diff --git a/lib/puppet/type/resources.rb b/lib/puppet/type/resources.rb
index ab564a1..e1de557 100644
--- a/lib/puppet/type/resources.rb
+++ b/lib/puppet/type/resources.rb
@@ -85,33 +85,33 @@ Puppet::Type.newtype(:resources) do
end
end
+ def able_to_ensure_absent?(resource)
+ begin
+ resource[:ensure] = :absent
+ rescue ArgumentError, Puppet::Error => detail
+ err "The 'ensure' attribute on #{self[:name]} resources does not accept 'absent' as a value"
+ false
+ end
+ end
+
# Generate any new resources we need to manage. This is pretty hackish
# right now, because it only supports purging.
def generate
return [] unless self.purge?
- hascheck = false
- method =
- resource_type.instances.find_all do |resource|
- ! resource.managed?
- end.find_all do |resource|
- check(resource)
- end.each do |resource|
- begin
- resource[:ensure] = :absent
- rescue ArgumentError, Puppet::Error => detail
- err "The 'ensure' attribute on %s resources does not accept 'absent' as a value" %
- [self[:name]]
- return []
- end
- @parameters.each do |name, param|
- next unless param.metaparam?
- resource[name] = param.value
- end
-
- # Mark that we're purging, so transactions can handle relationships
- # correctly
- resource.purging
- end
+ resource_type.instances.
+ reject { |r| catalog.resources.include? r.ref }.
+ select { |r| check(r) }.
+ select { |r| r.class.validproperty?(:ensure) }.
+ select { |r| able_to_ensure_absent?(r) }.
+ each { |resource|
+ @parameters.each do |name, param|
+ resource[name] = param.value if param.metaparam?
+ end
+
+ # Mark that we're purging, so transactions can handle relationships
+ # correctly
+ resource.purging
+ }
end
def resource_type
diff --git a/spec/unit/type/resources.rb b/spec/unit/type/resources.rb
index 147f21d..a3faede 100644
--- a/spec/unit/type/resources.rb
+++ b/spec/unit/type/resources.rb
@@ -21,4 +21,69 @@ describe resources do
resources.new(:name => "file").resource_type.should == Puppet::Type.type(:file)
end
end
+
+ describe "#generate" do
+ before do
+ @host1 = Puppet::Type.type(:host).new(:name => 'localhost', :ip => '127.0.0.1')
+ @catalog = Puppet::Resource::Catalog.new
+ @context = Puppet::Transaction.new(@catalog)
+ end
+
+ describe "when dealing with non-purging resources" do
+ before do
+ @resources = Puppet::Type.type(:resources).new(:name => 'host')
+ end
+
+ it "should not generate any resource" do
+ @resources.generate.should be_empty
+ end
+ end
+
+ describe "when the catalog contains a purging resource" do
+ before do
+ @resources = Puppet::Type.type(:resources).new(:name => 'host', :purge => true)
+ @purgeable_resource = Puppet::Type.type(:host).new(:name => 'localhost', :ip => '127.0.0.1')
+ @catalog.add_resource @resources
+ end
+
+ it "should not generate a duplicate of that resource" do
+ Puppet::Type.type(:host).stubs(:instances).returns [@host1]
+ @catalog.add_resource @host1
+ @resources.generate.collect { |r| r.ref }.should_not include(@host1.ref)
+ end
+
+
+ describe "when generating a purgeable resource" do
+ it "should be included in the generated resources" do
+ Puppet::Type.type(:host).stubs(:instances).returns [@purgeable_resource]
+ @resources.generate.collect { |r| r.ref }.should include(@purgeable_resource.ref)
+ end
+ end
+
+ describe "when the instance's do not have an ensure property" do
+ it "should not be included in the generated resources" do
+ @no_ensure_resource = Puppet::Type.type(:exec).new(:name => '/usr/bin/env echo')
+ Puppet::Type.type(:host).stubs(:instances).returns [@no_ensure_resource]
+ @resources.generate.collect { |r| r.ref }.should_not include(@no_ensure_resource.ref)
+ end
+ end
+
+ describe "when the instance's ensure property does not accept absent" do
+ it "should not be included in the generated resources" do
+ @no_absent_resource = Puppet::Type.type(:service).new(:name => 'foobar')
+ Puppet::Type.type(:host).stubs(:instances).returns [@no_absent_resource]
+ @resources.generate.collect { |r| r.ref }.should_not include(@no_absent_resource.ref)
+ end
+ end
+
+ describe "when checking the instance fails" do
+ it "should not be included in the generated resources" do
+ @purgeable_resource = Puppet::Type.type(:host).new(:name => 'foobar')
+ Puppet::Type.type(:host).stubs(:instances).returns [@purgeable_resource]
+ @resources.expects(:check).with(@purgeable_resource).returns(false)
+ @resources.generate.collect { |r| r.ref }.should_not include(@purgeable_resource.ref)
+ end
+ end
+ end
+ end
end
--
1.6.4.2
Luke Kanies
It isn't necessary to have relatives in Kansas City in order to be
unhappy. -- Groucho Marx
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
Marc Fournier
> From: Markus Roberts <Mar...@reality.com>
>
> Ensure that resources whose refs are included in the catalog are
> skipped to avoid duplication.
>
> * Refactor to avoid early bailout on resources that cannot be ensured
> absent.
>
> * Remove check for managed? in generate
>
> Checking if a resource is managed is unnecessary when checking for
> its inclusion in the catalog.
>
> * Add test coverage for Puppet::Type::Resources#generate
I've just tried this new patch and the warnings go away.
I haven't bothered testing the tests though...
Thanks Markus !
Marc