On 20/07/09 19:05, Luke Kanies wrote: > It looks like most of the patch is the same, but a comment on the > integration tests below.
Yes, you're right. /me should open eyes when coding... Hmm, there's an issue, though, James seems to have merged this patch. I'll queue a new fixing patch ASAP. > 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 >> >> > > -- Brice Figureau My Blog: http://www.masterzen.fr/ --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
