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.

9 files changed, 79 insertions, 64 deletions

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/node.rb b/spec/unit/node.rb
index 06719c4cd..26aec4196 100755
--- a/spec/unit/node/node.rb
+++ b/spec/unit/node.rb
@@ -1,6 +1,6 @@
 #!/usr/bin/env ruby
 
-require File.dirname(__FILE__) + '/../../spec_helper'
+require File.dirname(__FILE__) + '/../spec_helper'
 
 describe Puppet::Node, " when initializing" do
     before do
@@ -44,11 +44,17 @@ describe Puppet::Node, " when initializing" do
         @node.classes.should == ["myclass"]
     end
 
-    it "should accept the environment during initialization" do
+    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"]
@@ -57,32 +63,30 @@ 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" => "myenv"}
-        @node.environment.should == "myenv"
+        @node.parameters = {"environment" => "two"}
+        @node.environment.should == "two"
     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
+    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 not use an environment fact that is an empty string" do
-        @node.parameters = {"environment" => ""}
-        @node.environment.should be_nil
+    it "should fail if the parameter environment is invalid" do
+        @node.parameters = {"environment" => "three"}
+        proc { @node.environment }.should raise_error(ArgumentError)
     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
+    it "should fail if the parameter environment is invalid" do
+        @node.parameters = {"environment" => "three"}
+        proc { @node.environment }.should raise_error(ArgumentError)
     end
 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/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'