(PUP-6992) Fix inconsistent autoload of environment puppet functions

This commit changes the autoload behavior for puppet functions and types
that reside under the 'functions' or 'types' directory in an environment
so that the naming becomes consistent with the ruby functions that
reside under 'lib/puppet/functions'.

In essence, the namespace 'environment' is moved into a subdirectory with
the same name that functions and types in the global namespace can reside
directly under their respective 'functions' and 'types' directory.

acceptance/tests/parser_functions/puppet_lookup_cmd.rb

@@ -37,6 +37,7 @@
   '#{@coderoot}/environments/production':;
   '#{@coderoot}/environments/production/data':;
   '#{@coderoot}/environments/production/functions':;
+  '#{@coderoot}/environments/production/functions/environment':;
   '#{@coderoot}/environments/production/lib':;
   '#{@coderoot}/environments/production/lib/puppet':;
   '#{@coderoot}/environments/production/lib/puppet/functions':;
@@ -88,6 +89,7 @@
   '#{@coderoot}/environments/env1':;
   '#{@coderoot}/environments/env1/data':;
   '#{@coderoot}/environments/env1/functions':;
+  '#{@coderoot}/environments/env1/functions/environment':;
   '#{@coderoot}/environments/env1/lib':;
   '#{@coderoot}/environments/env1/lib/puppet':;
   '#{@coderoot}/environments/env1/lib/puppet/functions':;
@@ -140,6 +142,7 @@
   '#{@coderoot}/environments/env2':;
   '#{@coderoot}/environments/env2/data':;
   '#{@coderoot}/environments/env2/functions':;
+  '#{@coderoot}/environments/env2/functions/environment':;
   '#{@coderoot}/environments/env2/lib':;
   '#{@coderoot}/environments/env2/lib/puppet':;
   '#{@coderoot}/environments/env2/lib/puppet/functions':;
@@ -192,6 +195,7 @@
   '#{@coderoot}/environments/env3':;
   '#{@coderoot}/environments/env3/data':;
   '#{@coderoot}/environments/env3/functions':;
+  '#{@coderoot}/environments/env3/functions/environment':;
   '#{@coderoot}/environments/env3/not-lib':;
   '#{@coderoot}/environments/env3/not-lib/puppet':;
   '#{@coderoot}/environments/env3/not-lib/puppet/functions':;
@@ -244,6 +248,7 @@
   '#{@coderoot}/environments/env4':;
   '#{@coderoot}/environments/env4/data':;
   '#{@coderoot}/environments/env4/functions':;
+  '#{@coderoot}/environments/env4/functions/environment':;
   '#{@coderoot}/environments/env4/lib':;
   '#{@coderoot}/environments/env4/lib/puppet':;
   '#{@coderoot}/environments/env4/lib/puppet/functions':;
@@ -523,7 +528,7 @@ def data()
 }
 
 # Environment puppet function data provider
-file { '#{@coderoot}/environments/production/functions/data.pp':
+file { '#{@coderoot}/environments/production/functions/environment/data.pp':
   ensure => file,
   mode => "0755",
   content => 'function environment::data() {
@@ -535,7 +540,7 @@ def data()
 ',
 }
 
-file { '#{@coderoot}/environments/env1/functions/data.pp':
+file { '#{@coderoot}/environments/env1/functions/environment/data.pp':
   ensure => file,
   mode => "0755",
   content => 'function environment::data() {
@@ -547,7 +552,7 @@ def data()
 ',
 }
 
-file { '#{@coderoot}/environments/env2/functions/data.pp':
+file { '#{@coderoot}/environments/env2/functions/environment/data.pp':
   ensure => file,
   mode => "0755",
   content => 'function environment::data() {
@@ -559,7 +564,7 @@ def data()
 ',
 }
 
-file { '#{@coderoot}/environments/env3/functions/data.pp':
+file { '#{@coderoot}/environments/env3/functions/environment/data.pp':
   ensure => file,
   mode => "0755",
   content => 'function environment::data() {
@@ -571,7 +576,7 @@ def data()
 ',
 }
 
-file { '#{@coderoot}/environments/env4/functions/data.pp':
+file { '#{@coderoot}/environments/env4/functions/environment/data.pp':
   ensure => file,
   mode => "0755",
   content => 'function environment::data() {

lib/puppet/pops/loader/loader_paths.rb

@@ -109,7 +109,9 @@ def root_path
     def effective_path(typed_name, start_index_in_name)
       # Puppet name to path always skips the name-space as that is part of the generic path
       # i.e. <module>/mymodule/functions/foo.pp is the function mymodule::foo
-      "#{File.join(generic_path, typed_name.name_parts[ 1..-1 ])}.pp"
+      parts = typed_name.name_parts
+      parts = parts[start_index_in_name..-1] if parts.size > 1
+      "#{File.join(generic_path, parts)}.pp"
     end
   end
 

lib/puppet/pops/loader/module_loaders.rb

@@ -21,6 +21,8 @@ module Loader
 # @api private
 #
 module ModuleLoaders
+  ENVIRONMENT = 'environment'.freeze
+
   def self.system_loader_from(parent_loader, loaders)
     # Puppet system may be installed in a fixed location via RPM, installed as a Gem, via source etc.
     # The only way to find this across the different ways puppet can be installed is
@@ -37,6 +39,15 @@ def self.system_loader_from(parent_loader, loaders)
                                                        )
   end
 
+  def self.environment_loader_from(parent_loader, loaders, env_path)
+    ModuleLoaders::FileBased.new(parent_loader,
+      loaders,
+      ENVIRONMENT,
+      File.join(env_path, 'lib'),
+      ENVIRONMENT
+    )
+  end
+
   def self.module_loader_from(parent_loader, loaders, module_name, module_path)
     ModuleLoaders::FileBased.new(parent_loader,
                                                        loaders,
@@ -107,7 +118,6 @@ def find(typed_name)
       return nil unless typed_name.name_authority == Pcore::RUNTIME_NAME_AUTHORITY
 
       # Assume it is a global name, and that all parts of the name should be used when looking up
-      name_part_index = 0
       name_parts = typed_name.name_parts
 
       # Certain types and names can be disqualified up front
@@ -119,10 +129,6 @@ def find(typed_name)
         # ok since such a "module" cannot have namespaced content).
         #
         return nil unless name_parts[0] == module_name
-
-        # Skip the first part of the name when computing the path since the path already contains the name of the
-        # module
-        name_part_index = 1
       else
         # The name is in the global name space.
 
@@ -144,7 +150,7 @@ def find(typed_name)
       # Find the file to instantiate, and instantiate the entity if file is found
       origin = nil
       if (smart_path = smart_paths.effective_paths(typed_name.type).find do |sp|
-          origin = sp.effective_path(typed_name, name_part_index)
+          origin = sp.effective_path(typed_name, global? ? 0 : 1)
           existing_path(origin)
         end)
         value = smart_path.instantiator.create(self, typed_name, origin, get_contents(origin))
@@ -200,12 +206,20 @@ def get_source_ref(relative_path)
       raise NotImplementedError.new
     end
 
+    # Answers the question if this loader represents a global component (true for resource type loader and environment loader)
+    #
+    # @return [Boolean] `true` if this loader represents a global component
+    #
+    def global?
+      module_name.nil? || module_name == ENVIRONMENT
+    end
+
     # Produces the private loader for the module. If this module is not already resolved, this will trigger resolution
     #
     def private_loader
       # The system loader has a nil module_name and it does not have a private_loader as there are no functions
       # that can only by called by puppet runtime - if so, it acts as the private loader directly.
-      @private_loader ||= (module_name.nil? || module_name == 'environment' ? self : @loaders.private_loader_for_module(module_name))
+      @private_loader ||= (global? ? self : @loaders.private_loader_for_module(module_name))
     end
   end
 

lib/puppet/pops/loaders.rb

@@ -241,7 +241,7 @@ def create_environment_loader(environment)
       loader = add_loader_by_name(Loader::SimpleEnvironmentLoader.new(@runtime3_type_loader, loader_name))
     else
       # View the environment as a module to allow loading from it - this module is always called 'environment'
-      loader = Loader::ModuleLoaders.module_loader_from(@runtime3_type_loader, self, 'environment', env_path)
+      loader = Loader::ModuleLoaders.environment_loader_from(@runtime3_type_loader, self, env_path)
     end
 
     # An environment has a module path even if it has a null loader

spec/unit/functions/lookup_spec.rb

@@ -196,11 +196,13 @@ def explain(key, options = {})
           {
             env_name => {
               'environment.conf' => "environment_data_provider=function\n",
-              'functions' => { 'data.pp' => <<-PUPPET.unindent }
+              'functions' => {
+                'environment' => { 'data.pp' => <<-PUPPET.unindent }
                     function environment::data() {
                       { 'a' => 'value a' }
                     }
-              PUPPET
+                    PUPPET
+              }
             }
           }
         end

spec/unit/pops/loaders/environment_loader_spec.rb

@@ -0,0 +1,172 @@
+require 'spec_helper'
+require 'puppet_spec/files'
+require 'puppet/pops'
+require 'puppet/loaders'
+
+describe 'Environment loader' do
+  include PuppetSpec::Files
+
+  let(:env_name) { 'spec' }
+  let(:code_dir) { Puppet[:environmentpath] }
+  let(:env_dir) { File.join(code_dir, env_name) }
+  let(:env) { Puppet::Node::Environment.create(env_name.to_sym, [File.join(populated_code_dir, env_name, 'modules')]) }
+  let(:populated_code_dir) do
+    dir_contained_in(code_dir, env_name => env_content)
+    PuppetSpec::Files.record_tmp(env_dir)
+    code_dir
+  end
+
+  let(:env_content) {
+    {
+      'lib' => {
+        'puppet' => {
+          'functions' => {
+            'ruby_foo.rb' => <<-RUBY.unindent,
+              Puppet::Functions.create_function(:ruby_foo) do
+                def ruby_foo()
+                  'ruby_foo'
+                end
+              end
+              RUBY
+            'environment' => {
+              'ruby_foo.rb' => <<-RUBY.unindent
+                Puppet::Functions.create_function(:'environment::ruby_foo') do
+                  def ruby_foo()
+                    'environment::ruby_foo'
+                  end
+                end
+                RUBY
+            },
+            'someother' => {
+              'ruby_foo.rb' => <<-RUBY.unindent
+                Puppet::Functions.create_function(:'someother::ruby_foo') do
+                  def ruby_foo()
+                    'someother::ruby_foo'
+                  end
+                end
+                RUBY
+            },
+          }
+        }
+      },
+      'functions' => {
+        'puppet_foo.pp' => <<-PUPPET.unindent,
+          function puppet_foo() {
+            'puppet_foo'
+          }
+          PUPPET
+        'environment' => {
+          'puppet_foo.pp' => <<-PUPPET.unindent,
+            function environment::puppet_foo() {
+              'environment::puppet_foo'
+            }
+            PUPPET
+        },
+        'someother' => {
+          'puppet_foo.pp' => <<-PUPPET.unindent,
+            function somether::puppet_foo() {
+              'someother::puppet_foo'
+            }
+            PUPPET
+        }
+      },
+      'types' => {
+        'footype.pp' => <<-PUPPET.unindent,
+          type FooType = Enum['foo', 'bar', 'baz']
+          PUPPET
+        'environment' => {
+          'footype.pp' => <<-PUPPET.unindent,
+            type Environment::FooType = Integer[0,9]
+            PUPPET
+        },
+        'someother' => {
+          'footype.pp' => <<-PUPPET.unindent,
+            type SomeOther::FooType = Float[0.0,9.0]
+            PUPPET
+        }
+      }
+    }
+  }
+
+  before(:each) do
+    Puppet.push_context(:loaders => Puppet::Pops::Loaders.new(env))
+  end
+
+  after(:each) do
+    Puppet.pop_context
+  end
+
+  def load_or_nil(type, name)
+    found = Puppet::Pops::Loaders.find_loader(nil).load_typed(Puppet::Pops::Loader::TypedName.new(type, name))
+    found.nil? ? nil : found.value
+  end
+
+  context 'loading a Ruby function' do
+    it 'loads from global name space' do
+      function = load_or_nil(:function, 'ruby_foo')
+      expect(function).not_to be_nil
+
+      expect(function.class.name).to eq('ruby_foo')
+      expect(function).to be_a(Puppet::Functions::Function)
+    end
+
+    it 'loads from environment name space' do
+      function = load_or_nil(:function, 'environment::ruby_foo')
+      expect(function).not_to be_nil
+
+      expect(function.class.name).to eq('environment::ruby_foo')
+      expect(function).to be_a(Puppet::Functions::Function)
+    end
+
+    it 'fails to load from namespaces other than global or environment' do
+      function = load_or_nil(:function, 'someother::ruby_foo')
+      expect(function).to be_nil
+    end
+  end
+
+  context 'loading a Puppet function' do
+    it 'loads from global name space' do
+      function = load_or_nil(:function, 'puppet_foo')
+      expect(function).not_to be_nil
+
+      expect(function.class.name).to eq('puppet_foo')
+      expect(function).to be_a(Puppet::Functions::PuppetFunction)
+    end
+
+    it 'loads from environment name space' do
+      function = load_or_nil(:function, 'environment::puppet_foo')
+      expect(function).not_to be_nil
+
+      expect(function.class.name).to eq('environment::puppet_foo')
+      expect(function).to be_a(Puppet::Functions::PuppetFunction)
+    end
+
+    it 'fails to load from namespaces other than global or environment' do
+      function = load_or_nil(:function, 'someother::puppet_foo')
+      expect(function).to be_nil
+    end
+  end
+
+  context 'loading a Puppet type' do
+    it 'loads from global name space' do
+      type = load_or_nil(:type, 'footype')
+      expect(type).not_to be_nil
+
+      expect(type).to be_a(Puppet::Pops::Types::PTypeAliasType)
+      expect(type.name).to eq('FooType')
+    end
+
+    it 'loads from environment name space' do
+      type = load_or_nil(:type, 'environment::footype')
+      expect(type).not_to be_nil
+
+      expect(type).to be_a(Puppet::Pops::Types::PTypeAliasType)
+      expect(type.name).to eq('Environment::FooType')
+    end
+
+    it 'fails to load from namespaces other than global or environment' do
+      type = load_or_nil(:type, 'someother::footype')
+      expect(type).to be_nil
+    end
+  end
+end