+1 (as a partial pairing partner) On Thu, Aug 12, 2010 at 2:10 PM, Paul Berry <[email protected]> wrote:
> This change is part of an ongoing effort to remove functionality from > TypeCollection that is not related to keeping track of a collection of > types. This reduces TypeCollection's linkage to the environment, > which is a step toward decoupling it from the type loading mechanism. > > Signed-off-by: Paul Berry <[email protected]> > --- > lib/puppet/node/environment.rb | 36 +++++++++++- > lib/puppet/parser/compiler.rb | 2 +- > lib/puppet/parser/parser_support.rb | 2 +- > lib/puppet/resource/type_collection.rb | 32 ---------- > spec/unit/application/apply_spec.rb | 11 ++-- > spec/unit/application/master_spec.rb | 9 +-- > spec/unit/node/environment_spec.rb | 87 > ++++++++++++++++++++++++++- > spec/unit/parser/compiler_spec.rb | 4 +- > spec/unit/parser/parser_spec.rb | 2 +- > spec/unit/resource/type_collection_spec.rb | 81 > -------------------------- > 10 files changed, 132 insertions(+), 134 deletions(-) > > diff --git a/lib/puppet/node/environment.rb > b/lib/puppet/node/environment.rb > index 3f67474..e43dca7 100644 > --- a/lib/puppet/node/environment.rb > +++ b/lib/puppet/node/environment.rb > @@ -81,7 +81,7 @@ class Puppet::Node::Environment > Thread.current[:known_resource_types] ||= synchronize { > if @known_resource_types.nil? or @known_resource_types.stale? > @known_resource_types = Puppet::Resource::TypeCollection.new(self) > - @known_resource_types.perform_initial_import > + perform_initial_import > end > @known_resource_types > } > @@ -143,5 +143,39 @@ class Puppet::Node::Environment > end > end > > + def version > + return @version if defined?(@version) > + > + if self[:config_version] == "" > + @version = Time.now.to_i > + return @version > + end > + > + @version = Puppet::Util.execute([self[:config_version]]).strip > + > + rescue Puppet::ExecutionFailure => e > + raise Puppet::ParseError, "Unable to set config_version: #{e.message}" > + end > + > + private > + > + def perform_initial_import > + return if Puppet.settings[:ignoreimport] > + parser = Puppet::Parser::Parser.new(self) > + if code = Puppet.settings.uninterpolated_value(:code, name.to_s) and > code != "" > + parser.string = code > + else > + file = Puppet.settings.value(:manifest, name.to_s) > + return unless File.exist?(file) > + parser.file = file > + end > + parser.parse > + rescue => detail > + msg = "Could not parse for environment #{self}: #{detail}" > + error = Puppet::Error.new(msg) > + error.set_backtrace(detail.backtrace) > + raise error > + end > + > @root = new(:'*root*') > end > diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb > index 61bb13c..21b6d84 100644 > --- a/lib/puppet/parser/compiler.rb > +++ b/lib/puppet/parser/compiler.rb > @@ -426,7 +426,7 @@ class Puppet::Parser::Compiler > > # For maintaining the relationship between scopes and their resources. > @catalog = Puppet::Resource::Catalog.new(@node.name) > - @catalog.version = known_resource_types.version > + @catalog.version = environment.version > > # Create our initial scope and a resource that will evaluate main. > @topscope = Puppet::Parser::Scope.new(:compiler => self) > diff --git a/lib/puppet/parser/parser_support.rb > b/lib/puppet/parser/parser_support.rb > index 97d985c..f3f98e8 100644 > --- a/lib/puppet/parser/parser_support.rb > +++ b/lib/puppet/parser/parser_support.rb > @@ -223,7 +223,7 @@ class Puppet::Parser::Parser > end > > def version > - known_resource_types.version > + environment.version > end > > # Add a new file to be checked when we're checking to see if we should be > diff --git a/lib/puppet/resource/type_collection.rb > b/lib/puppet/resource/type_collection.rb > index 3a327e2..1a013bc 100644 > --- a/lib/puppet/resource/type_collection.rb > +++ b/lib/puppet/resource/type_collection.rb > @@ -110,42 +110,10 @@ class Puppet::Resource::TypeCollection > end > end > > - def perform_initial_import > - return if Puppet.settings[:ignoreimport] > - parser = Puppet::Parser::Parser.new(environment) > - if code = Puppet.settings.uninterpolated_value(:code, > environment.to_s) and code != "" > - parser.string = code > - else > - file = Puppet.settings.value(:manifest, environment.to_s) > - return unless File.exist?(file) > - parser.file = file > - end > - parser.parse > - rescue => detail > - msg = "Could not parse for environment #{environment}: #{detail}" > - error = Puppet::Error.new(msg) > - error.set_backtrace(detail.backtrace) > - raise error > - end > - > def stale? > @watched_files.values.detect { |file| file.changed? } > end > > - def version > - return @version if defined?(@version) > - > - if environment[:config_version] == "" > - @version = Time.now.to_i > - return @version > - end > - > - @version = Puppet::Util.execute([environment[:config_version]]).strip > - > - rescue Puppet::ExecutionFailure => e > - raise Puppet::ParseError, "Unable to set config_version: #{e.message}" > - end > - > def watch_file(file) > @watched_files[file] = Puppet::Util::LoadedFile.new(file) > end > diff --git a/spec/unit/application/apply_spec.rb > b/spec/unit/application/apply_spec.rb > index 0b00d1a..a79ddba 100755 > --- a/spec/unit/application/apply_spec.rb > +++ b/spec/unit/application/apply_spec.rb > @@ -142,17 +142,16 @@ describe Puppet::Application::Apply do > > describe "the parseonly command" do > before :each do > - Puppet.stubs(:[]).with(:environment) > + @environment = Puppet::Node::Environment.new("env") > + Puppet.stubs(:[]).with(:environment).returns(@environment) > Puppet.stubs(:[]).with(:manifest).returns("site.pp") > Puppet.stubs(:err) > @apply.stubs(:exit) > @apply.options.stubs(:[]).with(:code).returns "some code" > - @collection = stub_everything > - Puppet::Resource::TypeCollection.stubs(:new).returns(@collection) > end > > - it "should use a Puppet Resource Type Collection to parse the file" > do > - @collection.expects(:perform_initial_import) > + it "should use the environment to parse the file" do > + @environment.stubs(:perform_initial_import) > @apply.parseonly > end > > @@ -162,7 +161,7 @@ describe Puppet::Application::Apply do > end > > it "should exit with exit code 1 if error" do > - > @collection.stubs(:perform_initial_import).raises(Puppet::ParseError) > + > @environment.stubs(:perform_initial_import).raises(Puppet::ParseError) > @apply.expects(:exit).with(1) > @apply.parseonly > end > diff --git a/spec/unit/application/master_spec.rb > b/spec/unit/application/master_spec.rb > index 0baa822..d230213 100644 > --- a/spec/unit/application/master_spec.rb > +++ b/spec/unit/application/master_spec.rb > @@ -257,16 +257,15 @@ describe Puppet::Application::Master do > > describe "the parseonly command" do > before :each do > - Puppet.stubs(:[]).with(:environment) > + @environment = Puppet::Node::Environment.new("env") > + Puppet.stubs(:[]).with(:environment).returns(@environment) > Puppet.stubs(:[]).with(:manifest).returns("site.pp") > Puppet.stubs(:err) > @master.stubs(:exit) > - @collection = stub_everything > - Puppet::Resource::TypeCollection.stubs(:new).returns(@collection) > end > > it "should use a Puppet Resource Type Collection to parse the file" > do > - @collection.expects(:perform_initial_import) > + @environment.expects(:perform_initial_import) > @master.parseonly > end > > @@ -276,7 +275,7 @@ describe Puppet::Application::Master do > end > > it "should exit with exit code 1 if error" do > - > @collection.stubs(:perform_initial_import).raises(Puppet::ParseError) > + > @environment.stubs(:perform_initial_import).raises(Puppet::ParseError) > @master.expects(:exit).with(1) > @master.parseonly > end > diff --git a/spec/unit/node/environment_spec.rb > b/spec/unit/node/environment_spec.rb > index 6edcce5..1bc0221 100755 > --- a/spec/unit/node/environment_spec.rb > +++ b/spec/unit/node/environment_spec.rb > @@ -52,7 +52,7 @@ describe Puppet::Node::Environment do > before do > @env = Puppet::Node::Environment.new("dev") > @collection = Puppet::Resource::TypeCollection.new(@env) > - @collection.stubs(:perform_initial_import) > + @env.stubs(:perform_initial_import) > Thread.current[:known_resource_types] = nil > end > > @@ -66,9 +66,8 @@ describe Puppet::Node::Environment do > end > > it "should perform the initial import when creating a new collection" > do > - @collection.expects(:perform_initial_import) > - Puppet::Resource::TypeCollection.expects(:new).returns @collection > - > + @env = Puppet::Node::Environment.new("dev") > + @env.expects(:perform_initial_import) > @env.known_resource_types > end > > @@ -274,4 +273,84 @@ describe Puppet::Node::Environment do > @helper.environment.name.should == :foo > end > end > + > + describe "when performing initial import" do > + before do > + @parser = stub 'parser', :file= => nil, :string => nil, :parse => > nil > + Puppet::Parser::Parser.stubs(:new).returns @parser > + @env = Puppet::Node::Environment.new("env") > + end > + > + it "should create a new parser instance" do > + Puppet::Parser::Parser.expects(:new).returns @parser > + @env.instance_eval { perform_initial_import } > + end > + > + it "should set the parser's string to the 'code' setting and parse if > code is available" do > + Puppet.settings[:code] = "my code" > + @parser.expects(:string=).with "my code" > + @parser.expects(:parse) > + @env.instance_eval { perform_initial_import } > + end > + > + it "should set the parser's file to the 'manifest' setting and parse > if no code is available and the manifest is available" do > + File.stubs(:expand_path).with("/my/file").returns "/my/file" > + File.expects(:exist?).with("/my/file").returns true > + Puppet.settings[:manifest] = "/my/file" > + @parser.expects(:file=).with "/my/file" > + @parser.expects(:parse) > + @env.instance_eval { perform_initial_import } > + end > + > + it "should not attempt to load a manifest if none is present" do > + File.stubs(:expand_path).with("/my/file").returns "/my/file" > + File.expects(:exist?).with("/my/file").returns false > + Puppet.settings[:manifest] = "/my/file" > + @parser.expects(:file=).never > + @parser.expects(:parse).never > + @env.instance_eval { perform_initial_import } > + end > + > + it "should fail helpfully if there is an error importing" do > + File.stubs(:exist?).returns true > + @parser.expects(:parse).raises ArgumentError > + lambda { @env.instance_eval { perform_initial_import } }.should > raise_error(Puppet::Error) > + end > + > + it "should not do anything if the ignore_import settings is set" do > + Puppet.settings[:ignoreimport] = true > + @parser.expects(:string=).never > + @parser.expects(:file=).never > + @parser.expects(:parse).never > + @env.instance_eval { perform_initial_import } > + end > + end > + > + describe "when determining the configuration version" do > + before do > + @env = Puppet::Node::Environment.new("env") > + end > + > + it "should default to the current time" do > + time = Time.now > + > + Time.stubs(:now).returns time > + @env.version.should == time.to_i > + end > + > + it "should use the output of the environment's config_version setting > if one is provided" do > + @env.stubs(:[]).with(:config_version).returns("/my/foo") > + > + Puppet::Util.expects(:execute).with(["/my/foo"]).returns "output\n" > + @env.version.should == "output" > + end > + > + it "should raise a puppet parser error if executing config_version > fails" do > + @env.stubs(:[]).with(:config_version).returns("test") > + > Puppet::Util.expects(:execute).raises(Puppet::ExecutionFailure.new("msg")) > + > + lambda { @env.version }.should raise_error(Puppet::ParseError) > + end > + > + end > end > diff --git a/spec/unit/parser/compiler_spec.rb > b/spec/unit/parser/compiler_spec.rb > index e8c06dd..8befccc 100755 > --- a/spec/unit/parser/compiler_spec.rb > +++ b/spec/unit/parser/compiler_spec.rb > @@ -96,8 +96,8 @@ describe Puppet::Parser::Compiler do > @compiler.ast_nodes?.should be_true > end > > - it "should copy the known_resource_types version to the catalog" do > - @compiler.catalog.version.should == @known_resource_types.version > + it "should copy the environment version to the catalog" do > + @compiler.catalog.version.should == @compiler.environment.version > end > > it "should copy any node classes into the class list" do > diff --git a/spec/unit/parser/parser_spec.rb > b/spec/unit/parser/parser_spec.rb > index 0657ab3..7d0428f 100755 > --- a/spec/unit/parser/parser_spec.rb > +++ b/spec/unit/parser/parser_spec.rb > @@ -334,7 +334,7 @@ describe Puppet::Parser do > > describe "when determining the configuration version" do > it "should determine it from the resource type collection" do > - @parser.known_resource_types.expects(:version).returns "foo" > + @parser.environment.expects(:version).returns "foo" > @parser.version.should == "foo" > end > end > diff --git a/spec/unit/resource/type_collection_spec.rb > b/spec/unit/resource/type_collection_spec.rb > index 4bad291..9115d90 100644 > --- a/spec/unit/resource/type_collection_spec.rb > +++ b/spec/unit/resource/type_collection_spec.rb > @@ -415,85 +415,4 @@ describe Puppet::Resource::TypeCollection do > @loader.should_not be_stale > end > end > - > - describe "when performing initial import" do > - before do > - @parser = stub 'parser', :file= => nil, :string => nil, :parse => > nil > - Puppet::Parser::Parser.stubs(:new).returns @parser > - @code = Puppet::Resource::TypeCollection.new("env") > - end > - > - it "should create a new parser instance" do > - Puppet::Parser::Parser.expects(:new).returns @parser > - @code.perform_initial_import > - end > - > - it "should set the parser's string to the 'code' setting and parse if > code is available" do > - Puppet.settings[:code] = "my code" > - @parser.expects(:string=).with "my code" > - @parser.expects(:parse) > - @code.perform_initial_import > - end > - > - it "should set the parser's file to the 'manifest' setting and parse > if no code is available and the manifest is available" do > - File.stubs(:expand_path).with("/my/file").returns "/my/file" > - File.expects(:exist?).with("/my/file").returns true > - Puppet.settings[:manifest] = "/my/file" > - @parser.expects(:file=).with "/my/file" > - @parser.expects(:parse) > - @code.perform_initial_import > - end > - > - it "should not attempt to load a manifest if none is present" do > - File.stubs(:expand_path).with("/my/file").returns "/my/file" > - File.expects(:exist?).with("/my/file").returns false > - Puppet.settings[:manifest] = "/my/file" > - @parser.expects(:file=).never > - @parser.expects(:parse).never > - @code.perform_initial_import > - end > - > - it "should fail helpfully if there is an error importing" do > - File.stubs(:exist?).returns true > - @parser.expects(:parse).raises ArgumentError > - lambda { @code.perform_initial_import }.should > raise_error(Puppet::Error) > - end > - > - it "should not do anything if the ignore_import settings is set" do > - Puppet.settings[:ignoreimport] = true > - @parser.expects(:string=).never > - @parser.expects(:file=).never > - @parser.expects(:parse).never > - @code.perform_initial_import > - end > - end > - > - describe "when determining the configuration version" do > - before do > - @code = Puppet::Resource::TypeCollection.new("env") > - end > - > - it "should default to the current time" do > - time = Time.now > - > - Time.stubs(:now).returns time > - @code.version.should == time.to_i > - end > - > - it "should use the output of the environment's config_version setting > if one is provided" do > - > @code.environment.stubs(:[]).with(:config_version).returns("/my/foo") > - > - Puppet::Util.expects(:execute).with(["/my/foo"]).returns "output\n" > - @code.version.should == "output" > - end > - > - it "should raise a puppet parser error if executing config_version > fails" do > - @code.environment.stubs(:[]).with(:config_version).returns("test") > - > Puppet::Util.expects(:execute).raises(Puppet::ExecutionFailure.new("msg")) > - > - lambda { @code.version }.should raise_error(Puppet::ParseError) > - end > - > - end > - > end > -- > 1.7.2 > > -- > 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]<puppet-dev%[email protected]> > . > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > -- 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.
