It looks like most of the patch is the same, but a comment on the integration tests below.
On Jul 16, 2009, at 2:30 PM, Brice Figureau wrote: > > Hi, > > Here's a new version of this patch, including 2 indirection tests > (one for the filtering at transaction evaluation time, the other > for filtering after compilation). This patch also fixes the unit > tests which were broken. > > Please review and comment accordingly (especially the two integration > tests). > > Thanks, > Brice > > Original commit msg: > The issue is that when we convert Puppet::Parser::Resource catalog > to a Puppet::Resource catalog before storing it to the database, > we don't allow virtual resource to be converted. > Unfortunately exported resources are virtual by design, and as > such aren't converted, and we lose them, so it isn't possible > to store them in the database. > Unfortunately, the client will get the exported resources too. > > The fix is dual-fold: > * we make sure exported resource are skipped when the transaction is > applied as a last safeguard > * we filter-out the catalog through the catalog compiler terminus > before > the catalog is returned to the client > > Signed-off-by: Brice Figureau <[email protected]> > --- > lib/puppet/indirector/catalog/compiler.rb | 6 +++ > lib/puppet/indirector/indirection.rb | 2 +- > lib/puppet/resource/catalog.rb | 14 +++++++- > lib/puppet/transaction.rb | 2 + > lib/puppet/type.rb | 9 ++++- > spec/integration/indirector/catalog/compiler.rb | 34 ++++++++++++++ > +++++ > spec/integration/transaction.rb | 13 +++++++ > spec/unit/indirector/catalog/compiler.rb | 34 ++++++++++++++ > +++++ > spec/unit/indirector/indirection.rb | 8 ++++ > spec/unit/resource/catalog.rb | 41 ++++++++++++++ > +++++++++ > spec/unit/transaction.rb | 28 +++++++++++++++ > spec/unit/type.rb | 6 +++- > 12 files changed, 193 insertions(+), 4 deletions(-) > create mode 100755 spec/integration/indirector/catalog/compiler.rb > > diff --git a/lib/puppet/indirector/catalog/compiler.rb b/lib/puppet/ > indirector/catalog/compiler.rb > index c9a216d..12da4de 100644 > --- a/lib/puppet/indirector/catalog/compiler.rb > +++ b/lib/puppet/indirector/catalog/compiler.rb > @@ -41,6 +41,12 @@ class Puppet::Resource::Catalog::Compiler < > Puppet::Indirector::Code > end > end > > + # filter-out a catalog to remove exported resources > + def filter(catalog) > + return catalog.filter { |r| r.exported? } if > catalog.respond_to?(:filter) > + catalog > + end > + > def initialize > set_server_facts > end > diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/ > indirector/indirection.rb > index f16a9b5..dc7e58f 100644 > --- a/lib/puppet/indirector/indirection.rb > +++ b/lib/puppet/indirector/indirection.rb > @@ -202,7 +202,7 @@ class Puppet::Indirector::Indirection > cache.save request(:save, result, *args) > end > > - return result > + return terminus.respond_to?(:filter) ? > terminus.filter(result) : result > end > > return nil > diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/ > catalog.rb > index 42e92f4..bb82906 100644 > --- a/lib/puppet/resource/catalog.rb > +++ b/lib/puppet/resource/catalog.rb > @@ -468,6 +468,13 @@ class Puppet::Resource::Catalog < > Puppet::SimpleGraph > to_catalog :to_resource > end > > + # filter out the catalog, applying +block+ to each resource. > + # If the block result is false, the resource will > + # be kept otherwise it will be skipped > + def filter(&block) > + to_catalog :to_resource, &block > + end > + > # Store the classes in the classfile. > def write_class_file > begin > @@ -534,7 +541,8 @@ class Puppet::Resource::Catalog < > Puppet::SimpleGraph > > map = {} > vertices.each do |resource| > - next if resource.respond_to?(:virtual?) and > resource.virtual? > + next if virtual_not_exported?(resource) > + next if block_given? and yield resource > > #This is hackity hack for 1094 > #Aliases aren't working in the ral catalog because the > current instance of the resource > @@ -589,4 +597,8 @@ class Puppet::Resource::Catalog < > Puppet::SimpleGraph > > return result > end > + > + def virtual_not_exported?(resource) > + resource.respond_to?(:virtual?) and resource.virtual? and > (resource.respond_to?(:exported?) and not resource.exported?) > + end > end > diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb > index f09ca80..ea283d8 100644 > --- a/lib/puppet/transaction.rb > +++ b/lib/puppet/transaction.rb > @@ -592,6 +592,8 @@ class Transaction > resource.debug "Not scheduled" > elsif failed_dependencies?(resource) > resource.warning "Skipping because of failed dependencies" > + elsif resource.exported? > + resource.debug "Skipping because exported" > else > return false > end > diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb > index d300338..e97cb57 100644 > --- a/lib/puppet/type.rb > +++ b/lib/puppet/type.rb > @@ -1839,6 +1839,9 @@ class Type > # The catalog that this resource is stored in. > attr_accessor :catalog > > + # is the resource exported > + attr_accessor :exported > + > # create a log at specified level > def log(msg) > Puppet::Util::Log.create( > @@ -1872,7 +1875,7 @@ class Type > self.title = resource.ref > end > > - [:file, :line, :catalog].each do |getter| > + [:file, :line, :catalog, :exported].each do |getter| > setter = getter.to_s + "=" > if val = resource.send(getter) > self.send(setter, val) > @@ -2053,6 +2056,10 @@ class Type > return trans > end > > + def exported? > + exported > + end > + > end # Puppet::Type > end > > diff --git a/spec/integration/indirector/catalog/compiler.rb b/spec/ > integration/indirector/catalog/compiler.rb > new file mode 100755 > index 0000000..74dd2c4 > --- /dev/null > +++ b/spec/integration/indirector/catalog/compiler.rb > @@ -0,0 +1,34 @@ > +#!/usr/bin/env ruby > + > +Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist? > (f) ? require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/ > spec_helper.rb") } > + > +require 'puppet/resource/catalog' > + > +Puppet::Resource::Catalog.indirection.terminus(:compiler) > + > +describe Puppet::Resource::Catalog::Compiler do > + before do > + @catalog = Puppet::Resource::Catalog.new > + > + @one = Puppet::Resource.new(:file, "/one") > + @one.exported = true > + > + @two = Puppet::Resource.new(:file, "/two") > + @catalog.add_resource(@one, @two) > + end > + > + after { Puppet.settings.clear } > + > + it "should remove exported resources when filtering" do > + > Puppet > ::Resource::Catalog.indirection.terminus.filter(@catalog).resources > == [ @two ] I think you need a '.should' here, right? > > + end > + > + it "should filter out exported resources when finding a > catalog" do > + request = stub 'request' > + > Puppet > ::Resource > ::Catalog.indirection.terminus.stubs(:extract_facts_from_request) > + > Puppet > ::Resource::Catalog.indirection.terminus.stubs(:node_from_request) > + > Puppet > ::Resource > ::Catalog.indirection.terminus.stubs(:compile).returns(@catalog) > + > + > Puppet > ::Resource::Catalog.indirection.terminus.find(request).resources == > [ @two ] Same here. > > + end > +end > diff --git a/spec/integration/transaction.rb b/spec/integration/ > transaction.rb > index c06a43d..2b8a5d9 100755 > --- a/spec/integration/transaction.rb > +++ b/spec/integration/transaction.rb > @@ -22,4 +22,17 @@ describe Puppet::Transaction do > > transaction.evaluate > end > + > + it "should not apply exported resources" do > + catalog = Puppet::Resource::Catalog.new > + resource = Puppet::Type.type(:file).new :path => "/foo/ > bar", :backup => false > + resource.exported = true > + catalog.add_resource resource > + > + transaction = Puppet::Transaction.new(catalog) > + > + resource.expects(:evaluate).never > + > + transaction.evaluate > + end > end > diff --git a/spec/unit/indirector/catalog/compiler.rb b/spec/unit/ > indirector/catalog/compiler.rb > index 6e49a65..78a8028 100755 > --- a/spec/unit/indirector/catalog/compiler.rb > +++ b/spec/unit/indirector/catalog/compiler.rb > @@ -223,4 +223,38 @@ describe Puppet::Resource::Catalog::Compiler do > @compiler.find(@request) > end > end > + > + describe "when filtering resources" do > + before :each do > + @compiler = Puppet::Resource::Catalog::Compiler.new > + @catalog = stub_everything 'catalog' > + @catalog.stubs(:respond_to?).with(:filter).returns(true) > + end > + > + it "should delegate to the catalog instance filtering" do > + @catalog.expects(:filter) > + @compiler.filter(@catalog) > + end > + > + it "should filter out exported resources" do > + resource = mock 'resource', :exported? => true > + @catalog.stubs(:filter).yields(resource) > + > + @compiler.filter(@catalog) > + end > + > + it "should return the same catalog if it doesn't support > filtering" do > + @catalog.stubs(:respond_to?).with(:filter).returns(false) > + > + @compiler.filter(@catalog).should == @catalog > + end > + > + it "should return the filtered catalog" do > + catalog = stub 'filtered catalog' > + @catalog.stubs(:filter).returns(catalog) > + > + @compiler.filter(@catalog).should == catalog > + end > + > + end > end > diff --git a/spec/unit/indirector/indirection.rb b/spec/unit/ > indirector/indirection.rb > index 5d9efd2..220aa24 100755 > --- a/spec/unit/indirector/indirection.rb > +++ b/spec/unit/indirector/indirection.rb > @@ -243,6 +243,14 @@ describe Puppet::Indirector::Indirection do > @indirection.find("/my/key") > end > > + it "should filter the result instance if the terminus > supports it" do > + @terminus.stubs(:find).returns(@instance) > + > @terminus.stubs(:respond_to?).with(:filter).returns(true) > + > + @terminus.expects(:filter).with(@instance) > + > + @indirection.find("/my/key") > + end > describe "when caching is enabled" do > before do > @indirection.cache_class = :cache_terminus > diff --git a/spec/unit/resource/catalog.rb b/spec/unit/resource/ > catalog.rb > index 2f4476a..97b6ad7 100755 > --- a/spec/unit/resource/catalog.rb > +++ b/spec/unit/resource/catalog.rb > @@ -295,6 +295,47 @@ describe Puppet::Resource::Catalog, "when > compiling" do > end > end > > + describe "when filtering" do > + before :each do > + @original = Puppet::Resource::Catalog.new("mynode") > + @original.tag(*%w{one two three}) > + @original.add_class *%w{four five six} > + > + @r1 = stub_everything 'r1', :ref => "File[/a]" > + @r1.stubs(:respond_to?).with(:ref).returns(true) > + @r1.stubs(:dup).returns(@r1) > + @r1.stubs(:is_a?).returns(Puppet::Resource).returns(true) > + > + @r2 = stub_everything 'r2', :ref => "File[/b]" > + @r2.stubs(:respond_to?).with(:ref).returns(true) > + @r2.stubs(:dup).returns(@r2) > + @r2.stubs(:is_a?).returns(Puppet::Resource).returns(true) > + > + @resources = [...@r1,@r2] > + > + @original.add_resource(@r1,@r2) > + end > + > + it "should transform the catalog to a resource catalog" do > + @original.expects(:to_catalog).with { |h,b| h > == :to_resource } > + > + @original.filter > + end > + > + it "should scan each catalog resource in turn and apply > filtering block" do > + @resources.each { |r| r.expects(:exported?) } > + @original.filter do |r| > + r.exported? > + end > + end > + > + it "should filter out resources which produce true when the > filter block is evaluated" do > + @original.filter do |r| > + r == @r1 > + end.resource("File[/a]").should be_nil > + end > + end > + > describe "when functioning as a resource container" do > before do > @catalog = Puppet::Resource::Catalog.new("host") > diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb > index 37870f1..0e36747 100755 > --- a/spec/unit/transaction.rb > +++ b/spec/unit/transaction.rb > @@ -52,6 +52,34 @@ describe Puppet::Transaction do > > @transaction > .generate_additional_resources(generator, :generate).should be_empty > end > end > + > + describe "when skipping a resource" do > + before :each do > + @resource = stub_everything 'res' > + @catalog = Puppet::Resource::Catalog.new > + @transaction = Puppet::Transaction.new(@catalog) > + end > + > + it "should skip resource with missing tags" do > + @transaction.stubs(:missing_tags?).returns(true) > + @transaction.skip?(@resource).should be_true > + end > + > + it "should skip not scheduled resources" do > + @transaction.stubs(:scheduled?).returns(false) > + @transaction.skip?(@resource).should be_true > + end > + > + it "should skip resources with failed dependencies" do > + @transaction.stubs(:failed_dependencies?).returns(false) > + @transaction.skip?(@resource).should be_true > + end > + > + it "should skip exported resource" do > + @resource.stubs(:exported?).returns true > + @transaction.skip?(@resource).should be_true > + end > + end > end > > describe Puppet::Transaction, " when determining tags" do > diff --git a/spec/unit/type.rb b/spec/unit/type.rb > index a9e48a2..a55009d 100755 > --- a/spec/unit/type.rb > +++ b/spec/unit/type.rb > @@ -68,6 +68,10 @@ describe Puppet::Type do > resource.tags = [:tag1,:tag2] > end > > + it "should have a method to know if the resource is exported" do > + Puppet::Type.type(:mount).new(:name => "foo").should > respond_to(:exported?) > + end > + > describe "when initializing" do > describe "and passed a TransObject" do > it "should fail" do > @@ -87,7 +91,7 @@ describe Puppet::Type do > Puppet::Type.type(:mount).new(resource).title.should > == "User[foo]" > end > > - [:line, :file, :catalog].each do |param| > + [:line, :file, :catalog, :exported].each do |param| > it "should copy '#{param}' from the resource if > present" do > resource = Puppet::Resource.new(:mount, "/foo") > resource.send(param.to_s + "=", "foo") > -- > 1.6.0.2 > > > > -- No one who cannot rejoice in the discovery of his own mistakes deserves to be called a scholar. --Donald Foster --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
