From 8cc07adda20b4e63bbad5b2759303d00d215341c Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Tue, 20 Nov 2007 01:08:34 -0600 Subject: Using the Environment class to determine the default environment, rather than plenty of different places having the logic of how to determine the default environment. --- lib/puppet/indirector/module_files.rb | 4 +- lib/puppet/node.rb | 19 ++-- lib/puppet/node/environment.rb | 1 + spec/integration/indirector/module_files.rb | 6 +- spec/unit/indirector/configuration/compiler.rb | 5 +- spec/unit/indirector/module_files.rb | 63 ++++++----- spec/unit/node.rb | 140 +++++++++++++++++++++++++ spec/unit/node/environment.rb | 4 + spec/unit/node/node.rb | 136 ------------------------ spec/unit/ral/exec.rb | 1 - 10 files changed, 197 insertions(+), 182 deletions(-) create mode 100755 spec/unit/node.rb delete mode 100755 spec/unit/node/node.rb diff --git a/lib/puppet/indirector/module_files.rb b/lib/puppet/indirector/module_files.rb index c79fae57b..84286d8a5 100644 --- a/lib/puppet/indirector/module_files.rb +++ b/lib/puppet/indirector/module_files.rb @@ -57,10 +57,8 @@ class Puppet::Indirector::ModuleFiles < Puppet::Indirector::Terminus def environment(node_name) if node_name and node = Puppet::Node.find(node_name) node.environment - elsif env = Puppet.settings[:environment] and env != "" - env else - nil + Puppet::Node::Environment.new.name end end diff --git a/lib/puppet/node.rb b/lib/puppet/node.rb index 9758c895c..2d87b6515 100644 --- a/lib/puppet/node.rb +++ b/lib/puppet/node.rb @@ -3,6 +3,7 @@ require 'puppet/indirector' # A simplistic class for managing the node information itself. class Puppet::Node require 'puppet/node/facts' + require 'puppet/node/environment' # Set up indirection, so that nodes can be looked for in # the node sources. @@ -19,19 +20,23 @@ class Puppet::Node attr_accessor :name, :classes, :parameters, :source, :ipaddress, :names attr_reader :time - attr_writer :environment + + # Set the environment, making sure that it's valid. + def environment=(value) + raise(ArgumentError, "Invalid environment %s" % value) unless Puppet::Node::Environment.valid?(value) + @environment = value + end # Do not return environments that are the empty string, and use # explicitly set environments, then facts, then a central env # value. def environment - unless @environment and @environment != "" - if env = parameters["environment"] and env != "" - @environment = env - elsif env = Puppet[:environment] and env != "" + unless @environment + if env = parameters["environment"] + raise(ArgumentError, "Invalid environment %s from parameters" % env) unless Puppet::Node::Environment.valid?(env) @environment = env else - @environment = nil + @environment = Puppet::Node::Environment.new.name.to_s end end @environment @@ -66,7 +71,7 @@ class Puppet::Node @parameters = options[:parameters] || {} - @environment = options[:environment] + self.environment = options[:environment] if options[:environment] @time = Time.now end diff --git a/lib/puppet/node/environment.rb b/lib/puppet/node/environment.rb index ada7f4eea..2a314803f 100644 --- a/lib/puppet/node/environment.rb +++ b/lib/puppet/node/environment.rb @@ -9,6 +9,7 @@ class Puppet::Node::Environment # Is the provided environment valid? def self.valid?(name) + return false if name.to_s == "" valid.include?(name.to_sym) end diff --git a/spec/integration/indirector/module_files.rb b/spec/integration/indirector/module_files.rb index 3f49ec7fa..1831bffbc 100755 --- a/spec/integration/indirector/module_files.rb +++ b/spec/integration/indirector/module_files.rb @@ -10,10 +10,11 @@ require 'puppet/indirector/module_files' describe Puppet::Indirector::ModuleFiles, " when interacting with Puppet::Module and FileServing::Content" do it "should look for files in the module's 'files' directory" do + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) # We just test a subclass, since it's close enough. @terminus = Puppet::Indirector::FileContent::Modules.new @module = Puppet::Module.new("mymod", "/some/path/mymod") - Puppet::Module.expects(:find).with("mymod", nil).returns(@module) + Puppet::Module.expects(:find).with("mymod", "myenv").returns(@module) filepath = "/some/path/mymod/files/myfile" @@ -25,9 +26,10 @@ end describe Puppet::Indirector::ModuleFiles, " when interacting with FileServing::Fileset and FileServing::Content" do it "should return an instance for every file in the fileset" do + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) @terminus = Puppet::Indirector::FileContent::Modules.new @module = Puppet::Module.new("mymod", "/some/path/mymod") - Puppet::Module.expects(:find).with("mymod", nil).returns(@module) + Puppet::Module.expects(:find).with("mymod", "myenv").returns(@module) filepath = "/some/path/mymod/files/myfile" FileTest.stubs(:exists?).with(filepath).returns(true) diff --git a/spec/unit/indirector/configuration/compiler.rb b/spec/unit/indirector/configuration/compiler.rb index cb9dc7162..c024fe995 100755 --- a/spec/unit/indirector/configuration/compiler.rb +++ b/spec/unit/indirector/configuration/compiler.rb @@ -118,9 +118,12 @@ end describe Puppet::Node::Configuration::Compiler, " when creating configurations" do before do Facter.stubs(:value).returns("whatever") + env = stub 'environment', :name => "yay" + Puppet::Node::Environment.stubs(:new).returns(env) + @compiler = Puppet::Node::Configuration::Compiler.new @name = "me" - @node = Puppet::Node.new @name, :environment => "yay" + @node = Puppet::Node.new @name @node.stubs(:merge) Puppet::Node.stubs(:search).with(@name).returns(@node) end diff --git a/spec/unit/indirector/module_files.rb b/spec/unit/indirector/module_files.rb index 9cb683c3c..9011530dd 100755 --- a/spec/unit/indirector/module_files.rb +++ b/spec/unit/indirector/module_files.rb @@ -9,6 +9,7 @@ require 'puppet/indirector/module_files' module ModuleFilesTerminusTesting def setup + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) Puppet::Indirector::Terminus.stubs(:register_terminus_class) @model = mock 'model' @indirection = stub 'indirection', :name => :mystuff, :register_terminus_type => nil, :model => @model @@ -31,39 +32,39 @@ describe Puppet::Indirector::ModuleFiles, " when finding files" do include ModuleFilesTerminusTesting it "should strip off the leading '/modules' mount name" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Module.expects(:find).with('my', "myenv").returns @module @module_files.find(@uri) end it "should not strip off leading terms that start with '/modules' but are longer words" do - Puppet::Module.expects(:find).with('modulestart', nil).returns nil + Puppet::Module.expects(:find).with('modulestart', "myenv").returns nil @module_files.find("puppetmounts://host/modulestart/my/local/file") end it "should search for a module whose name is the first term in the remaining file path" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Module.expects(:find).with('my', "myenv").returns @module @module_files.find(@uri) end it "should search for a file relative to the module's files directory" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Module.expects(:find).with('my', "myenv").returns @module FileTest.expects(:exists?).with("/module/path/files/local/file") @module_files.find(@uri) end it "should return nil if the module does not exist" do - Puppet::Module.expects(:find).with('my', nil).returns nil + Puppet::Module.expects(:find).with('my', "myenv").returns nil @module_files.find(@uri).should be_nil end it "should return nil if the module exists but the file does not" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Module.expects(:find).with('my', "myenv").returns @module FileTest.expects(:exists?).with("/module/path/files/local/file").returns(false) @module_files.find(@uri).should be_nil end it "should return an instance of the model if a module is found and the file exists" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Module.expects(:find).with('my', "myenv").returns @module FileTest.expects(:exists?).with("/module/path/files/local/file").returns(true) @model.expects(:new).returns(:myinstance) @module_files.find(@uri).should == :myinstance @@ -76,24 +77,19 @@ describe Puppet::Indirector::ModuleFiles, " when finding files" do @module_files.find(@uri, :node => "mynode") end - it "should use the local environment setting to look up the module if the node name is not provided and the environment is not set to ''" do - Puppet.settings.stubs(:value).with(:environment).returns("testing") + it "should use the default environment setting to look up the module if the node name is not provided" do + env = stub "environment", :name => "testing" + Puppet::Node::Environment.stubs(:new).returns(env) Puppet::Module.expects(:find).with('my', "testing") @module_files.find(@uri) end - - it "should not use an environment when looking up the module if the node name is not provided and the environment is set to ''" do - Puppet.settings.stubs(:value).with(:environment).returns("") - Puppet::Module.expects(:find).with('my', nil) - @module_files.find(@uri) - end end describe Puppet::Indirector::ModuleFiles, " when returning instances" do include ModuleFilesTerminusTesting before do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Module.expects(:find).with('my', "myenv").returns @module FileTest.expects(:exists?).with("/module/path/files/local/file").returns(true) @instance = mock 'instance' end @@ -180,33 +176,39 @@ describe Puppet::Indirector::ModuleFiles, " when searching for files" do include ModuleFilesTerminusTesting it "should strip off the leading '/modules' mount name" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) + Puppet::Module.expects(:find).with('my', "myenv").returns @module @module_files.search(@uri) end it "should not strip off leading terms that start with '/modules' but are longer words" do - Puppet::Module.expects(:find).with('modulestart', nil).returns nil + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) + Puppet::Module.expects(:find).with('modulestart', "myenv").returns nil @module_files.search("puppetmounts://host/modulestart/my/local/file") end it "should search for a module whose name is the first term in the remaining file path" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) + Puppet::Module.expects(:find).with('my', "myenv").returns @module @module_files.search(@uri) end it "should search for a file relative to the module's files directory" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) + Puppet::Module.expects(:find).with('my', "myenv").returns @module FileTest.expects(:exists?).with("/module/path/files/local/file") @module_files.search(@uri) end it "should return nil if the module does not exist" do - Puppet::Module.expects(:find).with('my', nil).returns nil + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) + Puppet::Module.expects(:find).with('my', "myenv").returns nil @module_files.search(@uri).should be_nil end it "should return nil if the module exists but the file does not" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) + Puppet::Module.expects(:find).with('my', "myenv").returns @module FileTest.expects(:exists?).with("/module/path/files/local/file").returns(false) @module_files.search(@uri).should be_nil end @@ -218,27 +220,24 @@ describe Puppet::Indirector::ModuleFiles, " when searching for files" do @module_files.search(@uri, :node => "mynode") end - it "should use the local environment setting to look up the module if the node name is not provided and the environment is not set to ''" do - Puppet.settings.stubs(:value).with(:environment).returns("testing") + it "should use the default environment setting to look up the module if the node name is not provided and the environment is not set to ''" do + env = stub 'env', :name => "testing" + Puppet::Node::Environment.stubs(:new).returns(env) Puppet::Module.expects(:find).with('my', "testing") @module_files.search(@uri) end - it "should not use an environment when looking up the module if the node name is not provided and the environment is set to ''" do - Puppet.settings.stubs(:value).with(:environment).returns("") - Puppet::Module.expects(:find).with('my', nil) - @module_files.search(@uri) - end - it "should use :path2instances from the terminus_helper to return instances if a module is found and the file exists" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) + Puppet::Module.expects(:find).with('my', "myenv").returns @module FileTest.expects(:exists?).with("/module/path/files/local/file").returns(true) @module_files.expects(:path2instances).with(@uri, "/module/path/files/local/file", {}) @module_files.search(@uri) end it "should pass any options on to :path2instances" do - Puppet::Module.expects(:find).with('my', nil).returns @module + Puppet::Node::Environment.stubs(:new).returns(stub('env', :name => "myenv")) + Puppet::Module.expects(:find).with('my', "myenv").returns @module FileTest.expects(:exists?).with("/module/path/files/local/file").returns(true) @module_files.expects(:path2instances).with(@uri, "/module/path/files/local/file", :testing => :one, :other => :two) @module_files.search(@uri, :testing => :one, :other => :two) diff --git a/spec/unit/node.rb b/spec/unit/node.rb new file mode 100755 index 000000000..26aec4196 --- /dev/null +++ b/spec/unit/node.rb @@ -0,0 +1,140 @@ +#!/usr/bin/env ruby + +require File.dirname(__FILE__) + '/../spec_helper' + +describe Puppet::Node, " when initializing" do + before do + @node = Puppet::Node.new("testnode") + end + + it "should set the node name" do + @node.name.should == "testnode" + end + + it "should not allow nil node names" do + proc { Puppet::Node.new(nil) }.should raise_error(ArgumentError) + end + + it "should default to an empty parameter hash" do + @node.parameters.should == {} + end + + it "should default to an empty class array" do + @node.classes.should == [] + end + + it "should note its creation time" do + @node.time.should be_instance_of(Time) + end + + it "should accept parameters passed in during initialization" do + params = {"a" => "b"} + @node = Puppet::Node.new("testing", :parameters => params) + @node.parameters.should == params + end + + it "should accept classes passed in during initialization" do + classes = %w{one two} + @node = Puppet::Node.new("testing", :classes => classes) + @node.classes.should == classes + end + + it "should always return classes as an array" do + @node = Puppet::Node.new("testing", :classes => "myclass") + @node.classes.should == ["myclass"] + end + + it "should accept an environment value" do + Puppet.settings.stubs(:value).with(:environments).returns("myenv") + @node = Puppet::Node.new("testing", :environment => "myenv") + @node.environment.should == "myenv" + end + + it "should validate the environment" do + Puppet.settings.stubs(:value).with(:environments).returns("myenv") + proc { Puppet::Node.new("testing", :environment => "other") }.should raise_error(ArgumentError) + end + + it "should accept names passed in" do + @node = Puppet::Node.new("testing", :names => ["myenv"]) + @node.names.should == ["myenv"] + end +end + +describe Puppet::Node, " when returning the environment" do + before do + Puppet.settings.stubs(:value).with(:environments).returns("one,two") + Puppet.settings.stubs(:value).with(:environment).returns("one") + @node = Puppet::Node.new("testnode") + end + + it "should return the 'environment' fact if present and there is no explicit environment" do + @node.parameters = {"environment" => "two"} + @node.environment.should == "two" + end + + it "should use the default environment if there is no environment fact nor explicit environment" do + env = mock 'environment', :name => :myenv + Puppet::Node::Environment.expects(:new).returns(env) + @node.environment.should == "myenv" + end + + it "should fail if the parameter environment is invalid" do + @node.parameters = {"environment" => "three"} + proc { @node.environment }.should raise_error(ArgumentError) + end + + it "should fail if the parameter environment is invalid" do + @node.parameters = {"environment" => "three"} + proc { @node.environment }.should raise_error(ArgumentError) + end +end + +describe Puppet::Node, " when merging facts" do + before do + @node = Puppet::Node.new("testnode") + Puppet::Node::Facts.stubs(:find).with(@node.name).returns(Puppet::Node::Facts.new(@node.name, "one" => "c", "two" => "b")) + end + + it "should prefer parameters already set on the node over facts from the node" do + @node.parameters = {"one" => "a"} + @node.fact_merge + @node.parameters["one"].should == "a" + end + + it "should add passed parameters to the parameter list" do + @node.parameters = {"one" => "a"} + @node.fact_merge + @node.parameters["two"].should == "b" + end + + it "should accept arbitrary parameters to merge into its parameters" do + @node.parameters = {"one" => "a"} + @node.merge "two" => "three" + @node.parameters["two"].should == "three" + end +end + +describe Puppet::Node, " when indirecting" do + it "should redirect to the indirection" do + @indirection = mock 'indirection' + Puppet::Node.stubs(:indirection).returns(@indirection) + @indirection.expects(:find).with(:my_node.to_s) + Puppet::Node.find(:my_node.to_s) + end + + it "should default to the 'null' node terminus" do + Puppet::Node.indirection.terminus_class.should == :null + end + + after do + Puppet::Indirector::Indirection.clear_cache + end +end + +describe Puppet::Node do + # LAK:NOTE This is used to keep track of when a given node has connected, + # so we can report on nodes that do not appear to connecting to the + # central server. + it "should provide a method for noting that the node has connected" +end diff --git a/spec/unit/node/environment.rb b/spec/unit/node/environment.rb index 69d7acb47..8433a9877 100755 --- a/spec/unit/node/environment.rb +++ b/spec/unit/node/environment.rb @@ -55,6 +55,10 @@ describe Puppet::Node::Environment do proc { Puppet::Node::Environment.new("three") }.should raise_error(ArgumentError) end + it "should consider environments that are empty strings invalid" do + Puppet::Node::Environment.valid?("").should be_false + end + it "should fail if a no-longer-valid environment instance is asked for" do Puppet.settings.expects(:value).with(:environments).returns("one") Puppet::Node::Environment.new("one") diff --git a/spec/unit/node/node.rb b/spec/unit/node/node.rb deleted file mode 100755 index 06719c4cd..000000000 --- a/spec/unit/node/node.rb +++ /dev/null @@ -1,136 +0,0 @@ -#!/usr/bin/env ruby - -require File.dirname(__FILE__) + '/../../spec_helper' - -describe Puppet::Node, " when initializing" do - before do - @node = Puppet::Node.new("testnode") - end - - it "should set the node name" do - @node.name.should == "testnode" - end - - it "should not allow nil node names" do - proc { Puppet::Node.new(nil) }.should raise_error(ArgumentError) - end - - it "should default to an empty parameter hash" do - @node.parameters.should == {} - end - - it "should default to an empty class array" do - @node.classes.should == [] - end - - it "should note its creation time" do - @node.time.should be_instance_of(Time) - end - - it "should accept parameters passed in during initialization" do - params = {"a" => "b"} - @node = Puppet::Node.new("testing", :parameters => params) - @node.parameters.should == params - end - - it "should accept classes passed in during initialization" do - classes = %w{one two} - @node = Puppet::Node.new("testing", :classes => classes) - @node.classes.should == classes - end - - it "should always return classes as an array" do - @node = Puppet::Node.new("testing", :classes => "myclass") - @node.classes.should == ["myclass"] - end - - it "should accept the environment during initialization" do - @node = Puppet::Node.new("testing", :environment => "myenv") - @node.environment.should == "myenv" - end - - it "should accept names passed in" do - @node = Puppet::Node.new("testing", :names => ["myenv"]) - @node.names.should == ["myenv"] - end -end - -describe Puppet::Node, " when returning the environment" do - before do - @node = Puppet::Node.new("testnode") - end - - it "should return the 'environment' fact if present and there is no explicit environment" do - @node.parameters = {"environment" => "myenv"} - @node.environment.should == "myenv" - end - - it "should return the central environment if there is no environment fact nor explicit environment" do - Puppet.settings.expects(:[]).with(:environment).returns(:centralenv) - @node.environment.should == :centralenv - end - - it "should not use an explicit environment that is an empty string" do - @node.environment == "" - @node.environment.should be_nil - end - - it "should not use an environment fact that is an empty string" do - @node.parameters = {"environment" => ""} - @node.environment.should be_nil - end - - it "should not use an explicit environment that is an empty string" do - Puppet.settings.expects(:[]).with(:environment).returns(nil) - @node.environment.should be_nil - end -end - -describe Puppet::Node, " when merging facts" do - before do - @node = Puppet::Node.new("testnode") - Puppet::Node::Facts.stubs(:find).with(@node.name).returns(Puppet::Node::Facts.new(@node.name, "one" => "c", "two" => "b")) - end - - it "should prefer parameters already set on the node over facts from the node" do - @node.parameters = {"one" => "a"} - @node.fact_merge - @node.parameters["one"].should == "a" - end - - it "should add passed parameters to the parameter list" do - @node.parameters = {"one" => "a"} - @node.fact_merge - @node.parameters["two"].should == "b" - end - - it "should accept arbitrary parameters to merge into its parameters" do - @node.parameters = {"one" => "a"} - @node.merge "two" => "three" - @node.parameters["two"].should == "three" - end -end - -describe Puppet::Node, " when indirecting" do - it "should redirect to the indirection" do - @indirection = mock 'indirection' - Puppet::Node.stubs(:indirection).returns(@indirection) - @indirection.expects(:find).with(:my_node.to_s) - Puppet::Node.find(:my_node.to_s) - end - - it "should default to the 'null' node terminus" do - Puppet::Node.indirection.terminus_class.should == :null - end - - after do - Puppet::Indirector::Indirection.clear_cache - end -end - -describe Puppet::Node do - # LAK:NOTE This is used to keep track of when a given node has connected, - # so we can report on nodes that do not appear to connecting to the - # central server. - it "should provide a method for noting that the node has connected" -end diff --git a/spec/unit/ral/exec.rb b/spec/unit/ral/exec.rb index b465123f8..8edf6535d 100755 --- a/spec/unit/ral/exec.rb +++ b/spec/unit/ral/exec.rb @@ -5,7 +5,6 @@ require File.dirname(__FILE__) + '/../../spec_helper' require 'puppet/type/exec' module ExecModuleTesting - def create_resource(command, output, exitstatus) @user_name = 'some_user_name' @group_name = 'some_group_name' -- cgit