Merge branch 'pr/3146' into stable

* pr/3146:
  (maint) Modify other uses of get() to get!()
  (PUP-3244) Add Puppet::Environment::Directories#get! method
  (PUP-3244) Validate Environment Before Setting
  (PUP-3244) Acceptance Tests for Nonexistent Environments
  (PUP-3244) ENC ignores missing directory environments

Closes GH-3146

10 files changed, 116 insertions, 12 deletions

diff --git a/acceptance/tests/environment/enc_nonexistent_directory_enviornment.rb b/acceptance/tests/environment/enc_nonexistent_directory_enviornment.rb
new file mode 100644
index 000000000..f25916a8f
--- /dev/null
+++ b/acceptance/tests/environment/enc_nonexistent_directory_enviornment.rb
@@ -0,0 +1,43 @@
+test_name "Master should produce error if enc specifies a nonexistent environment"
+testdir = create_tmpdir_for_user master, 'nonexistent_env'
+
+apply_manifest_on master, <<-MANIFEST
+file {
+  [  "#{testdir}/environments", "#{testdir}/environments/production" ]:
+  ensure => directory,
+}
+
+file { "#{testdir}/environments/production/environment.conf":
+    ensure  => file,
+    content => '
+      manifest=./production.pp
+    ',
+}
+
+file { "#{testdir}/environments/production/production.pp":
+  ensure  => file,
+  content => 'notify { "In the production environment": }',
+}
+
+file { "#{testdir}/enc.rb":
+  ensure  => file,
+  mode    => '0775',
+  content => 'echo "environment: doesnotexist"',
+}
+MANIFEST
+
+master_opts = {
+  'main' => {
+    'node_terminus' => 'exec',
+    'external_nodes' => "#{testdir}/enc.rb",
+    'environmentpath' => "#{testdir}/environments",
+  }
+}
+
+with_puppet_running_on master, master_opts, testdir do
+  agents.each do |agent|
+    on agent, "puppet agent --no-daemonize --onetime --server #{master} --verbose", :acceptable_exit_codes => [0] do
+      assert_match(/Could not find a directory environment named 'doesnotexist'/, stderr, "Errors when nonexistant environment is specified")
+    end
+  end
+end
diff --git a/lib/puppet/application.rb b/lib/puppet/application.rb
index 1d9611093..0e98d8adb 100644
--- a/lib/puppet/application.rb
+++ b/lib/puppet/application.rb
@@ -360,10 +360,7 @@ class Application
     #
     configured_environment_name = Puppet[:environment]
     if self.class.run_mode.name != :agent
-      configured_environment = Puppet.lookup(:environments).get(configured_environment_name)
-      if configured_environment.nil?
-        fail(Puppet::Environments::EnvironmentNotFound, configured_environment_name)
-      end
+      configured_environment = Puppet.lookup(:environments).get!(configured_environment_name)
     else
       configured_environment = Puppet::Node::Environment.remote(configured_environment_name)
     end
diff --git a/lib/puppet/environments.rb b/lib/puppet/environments.rb
index 9f7ac5c31..6af5622de 100644
--- a/lib/puppet/environments.rb
+++ b/lib/puppet/environments.rb
@@ -48,6 +48,14 @@ module Puppet::Environments
   #     we are looking up
   #   @return [Puppet::Setting::EnvironmentConf, nil] the configuration for the
   #     requested environment, or nil if not found or no configuration is available
+  #
+  # @!macro [new] loader_get_or_fail
+  #   Find a named environment or raise
+  #   Puppet::Environments::EnvironmentNotFound when the named environment is
+  #   does not exist.
+  #
+  #   @param name [String,Symbol] The name of environment to find
+  #   @return [Puppet::Node::Environment] the requested environment
 
   # A source of pre-defined environments.
   #
@@ -76,6 +84,14 @@ module Puppet::Environments
       end
     end
 
+    # @!macro loader_get_or_fail
+    def get!(name)
+      if !environment = get(name)
+        raise EnvironmentNotFound, name
+      end
+      environment
+    end
+
     # Returns a basic environment configuration object tied to the environment's
     # implementation values.  Will not interpolate.
     #
@@ -144,6 +160,14 @@ module Puppet::Environments
       Puppet::Node::Environment.new(name)
     end
 
+    # @note Because the Legacy system cannot list out all of its environments,
+    #   this method will never fail and is only calling get directly.
+    #
+    # @!macro loader_get_or_fail
+    def get!(name)
+      get(name)
+    end
+
     # @note we could return something here, but since legacy environments
     #   are deprecated, there is no point.
     #
@@ -206,6 +230,14 @@ module Puppet::Environments
       list.find { |env| env.name == name.intern }
     end
 
+    # @!macro loader_get_or_fail
+    def get!(name)
+      if !environment = get(name)
+        raise EnvironmentNotFound, name
+      end
+      environment
+    end
+
     # @!macro loader_get_conf
     def get_conf(name)
       valid_directories.each do |envdir|
@@ -259,6 +291,14 @@ module Puppet::Environments
       nil
     end
 
+    # @!macro loader_get_or_fail
+    def get!(name)
+      if !environment = get(name)
+        raise EnvironmentNotFound, name
+      end
+      environment
+    end
+
     # @!macro loader_get_conf
     def get_conf(name)
       @loaders.each do |loader|
@@ -289,6 +329,14 @@ module Puppet::Environments
       end
     end
 
+    # @!macro loader_get_or_fail
+    def get!(name)
+      if !environment = get(name)
+        raise EnvironmentNotFound, name
+      end
+      environment
+    end
+
     # Clears the cache of the environment with the given name.
     # (The intention is that this could be used from a MANUAL cache eviction command (TBD)
     def clear(name)
diff --git a/lib/puppet/indirector/request.rb b/lib/puppet/indirector/request.rb
index 04c55f9a3..a156f9de5 100644
--- a/lib/puppet/indirector/request.rb
+++ b/lib/puppet/indirector/request.rb
@@ -95,8 +95,7 @@ class Puppet::Indirector::Request
     elsif (current_environment = Puppet.lookup(:current_environment)).name == env
       current_environment
     else
-      Puppet.lookup(:environments).get(env) ||
-      raise(Puppet::Environments::EnvironmentNotFound, env)
+      Puppet.lookup(:environments).get!(env)
     end
   end
 
diff --git a/lib/puppet/module.rb b/lib/puppet/module.rb
index 3a3435c35..deecfd45b 100644
--- a/lib/puppet/module.rb
+++ b/lib/puppet/module.rb
@@ -29,7 +29,7 @@ class Puppet::Module
   def self.find(modname, environment = nil)
     return nil unless modname
     # Unless a specific environment is given, use the current environment
-    env = environment ? Puppet.lookup(:environments).get(environment) : Puppet.lookup(:current_environment)
+    env = environment ? Puppet.lookup(:environments).get!(environment) : Puppet.lookup(:current_environment)
     env.module(modname)
   end
 
diff --git a/lib/puppet/module_tool.rb b/lib/puppet/module_tool.rb
index 8f462f6d8..984947d33 100644
--- a/lib/puppet/module_tool.rb
+++ b/lib/puppet/module_tool.rb
@@ -151,7 +151,7 @@ module Puppet
       elsif options[:environment]
         # This use of looking up an environment is correct since it honours
         # a reguest to get a particular environment via environment name.
-        Puppet.lookup(:environments).get(options[:environment])
+        Puppet.lookup(:environments).get!(options[:environment])
       else
         Puppet.lookup(:current_environment)
       end
diff --git a/lib/puppet/node.rb b/lib/puppet/node.rb
index d61d49385..32f11168c 100644
--- a/lib/puppet/node.rb
+++ b/lib/puppet/node.rb
@@ -67,7 +67,7 @@ class Puppet::Node
         # for a node when it has not specified its environment
         # Tt will be used to establish what the current environment is.
         #
-        self.environment = Puppet.lookup(:environments).get(Puppet[:environment])
+        self.environment = Puppet.lookup(:environments).get!(Puppet[:environment])
       end
 
       @environment
@@ -76,7 +76,7 @@ class Puppet::Node
 
   def environment=(env)
     if env.is_a?(String) or env.is_a?(Symbol)
-      @environment = Puppet.lookup(:environments).get(env)
+      @environment = Puppet.lookup(:environments).get!(env)
     else
       @environment = env
     end
diff --git a/lib/puppet/parser/type_loader.rb b/lib/puppet/parser/type_loader.rb
index 1357f948d..2f6c77257 100644
--- a/lib/puppet/parser/type_loader.rb
+++ b/lib/puppet/parser/type_loader.rb
@@ -55,7 +55,7 @@ class Puppet::Parser::TypeLoader
 
   def environment=(env)
     if env.is_a?(String) or env.is_a?(Symbol)
-      @environment = Puppet.lookup(:environments).get(env)
+      @environment = Puppet.lookup(:environments).get!(env)
     else
       @environment = env
     end
diff --git a/lib/puppet/util/autoload.rb b/lib/puppet/util/autoload.rb
index 51ff94e1c..ddc62734c 100644
--- a/lib/puppet/util/autoload.rb
+++ b/lib/puppet/util/autoload.rb
@@ -128,7 +128,7 @@ class Puppet::Util::Autoload
       # "app_defaults_initialized?" method on the main puppet Settings object.
       # --cprice 2012-03-16
       if Puppet.settings.app_defaults_initialized? &&
-        env ||= Puppet.lookup(:environments).get(Puppet[:environment])
+        env ||= Puppet.lookup(:environments).get!(Puppet[:environment])
 
         # if the app defaults have been initialized then it should be safe to access the module path setting.
         $env_module_directories[env] ||= env.modulepath.collect do |dir|
diff --git a/spec/unit/environments_spec.rb b/spec/unit/environments_spec.rb
index 80a0bb2d5..917790f29 100644
--- a/spec/unit/environments_spec.rb
+++ b/spec/unit/environments_spec.rb
@@ -106,6 +106,17 @@ describe Puppet::Environments do
       end
     end
 
+    it "raises error if an environment can't be found" do
+      directory_tree = FS::MemoryFile.a_directory("envdir", [])
+
+      loader_from(:filesystem => [directory_tree],
+                  :directory => directory_tree) do |loader|
+        expect do
+          loader.get!("env_not_in_this_list")
+        end.to raise_error(Puppet::Environments::EnvironmentNotFound)
+      end
+    end
+
     context "with an environment.conf" do
       let(:envdir) do
         FS::MemoryFile.a_directory(File.expand_path("envdir"), [
@@ -315,6 +326,12 @@ config_version=$vardir/random/scripts
       expect(loader.get(:doesnotexist)).to be_nil
     end
 
+    it "raises error if environment is not found" do
+      expect do
+        loader.get!(:doesnotexist)
+      end.to raise_error(Puppet::Environments::EnvironmentNotFound)
+    end
+
     it "gets a basic conf" do
       conf = loader.get_conf(:static1)
       expect(conf.modulepath).to eq('')