(maint) Re-allow relative and absolute paths for moduledir & puppetfile

Previous to recent refactors an API user could provide either an
absolute or relative moduledir path, as well as an absolute
puppetfile_path or relative puppetfile_name, when resolving Puppetfiles.
The recent refactor required consumers to either pass absolute paths or
allow defaults rooted in the basedir, but disallowed passing relative
paths that would then be rooted in the basedir.

This commit returns that ability, as well as refactoring the internal
path joining and cleaning to be shared by all consumers of a file system
path.

This will change the the internal storage of basedir, moduledir, and
puppetfile to use a "cleaned" path stripped of extra slashes and dots.
However, this cleaned representation is what we would compare to
install_paths and display to users in error messages, so it should be a
change that does not affect any behavior.

lib/r10k/module_loader/puppetfile.rb

@@ -13,22 +13,24 @@ class Puppetfile
       # @param basedir [String] The path that contains the moduledir &
       #     Puppetfile by default. May be an environment, project, or
       #     simple directory.
-      # @param puppetfile [String] The full path to the Puppetfile
-      # @param moduledir [String] The full path to the moduledir
+      # @param puppetfile [String] The path to the Puppetfile, either an
+      #     absolute full path or a relative path with regards to the basedir.
+      # @param moduledir [String] The path to the moduledir, either an
+      #     absolute full path or a relative path with regards to the basedir.
       # @param forge [String] The url (without protocol) to the Forge
       # @param overrides [Hash] Configuration for loaded modules' behavior
       # @param environment [R10K::Environment] The environment loading may be
       #     taking place within
       def initialize(basedir:,
-                     moduledir: File.join(basedir, DEFAULT_MODULEDIR),
-                     puppetfile: File.join(basedir, DEFAULT_PUPPETFILE_NAME),
+                     moduledir: DEFAULT_MODULEDIR,
+                     puppetfile: DEFAULT_PUPPETFILE_NAME,
                      forge: DEFAULT_FORGE_API,
                      overrides: {},
                      environment: nil)
 
-        @basedir     = basedir
-        @moduledir   = moduledir
-        @puppetfile  = puppetfile
+        @basedir     = cleanpath(basedir)
+        @moduledir   = resolve_path(@basedir, moduledir)
+        @puppetfile  = resolve_path(@basedir, puppetfile)
         @forge       = forge
         @overrides   = overrides
         @environment = environment
@@ -76,11 +78,7 @@ def set_forge(forge)
 
       # @param [String] moduledir
       def set_moduledir(moduledir)
-        @moduledir = if Pathname.new(moduledir).absolute?
-          moduledir
-        else
-          File.join(@basedir, moduledir)
-        end
+        @moduledir = resolve_path(@basedir, moduledir)
       end
 
       # @param [String] name
@@ -97,7 +95,7 @@ def add_module(name, args)
         args[:overrides] = @overrides
 
         if install_path = args.delete(:install_path)
-          install_path = resolve_install_path(install_path)
+          install_path = resolve_path(@basedir, install_path)
           validate_install_path!(install_path, name)
         else
           install_path = @moduledir
@@ -136,29 +134,24 @@ def validate_no_duplicate_names!(modules)
         end
       end
 
-      def resolve_install_path(path)
-        pn = Pathname.new(path)
-
-        unless pn.absolute?
-          pn = Pathname.new(File.join(@basedir, path))
+      def resolve_path(base, path)
+        if Pathname.new(path).absolute?
+          cleanpath(path)
+        else
+          cleanpath(File.join(base, path))
         end
-
-        # .cleanpath is as good as we can do without touching the filesystem.
-        # The .realpath methods will also choke if some of the intermediate
-        # paths are missing, even though we will create them later as needed.
-        pn.cleanpath.to_s
       end
 
       def validate_install_path!(path, modname)
-        unless /^#{Regexp.escape(real_basedir)}.*/ =~ path
-          raise R10K::Error.new("Puppetfile cannot manage content '#{modname}' outside of containing environment: #{path} is not within #{real_basedir}")
+        unless /^#{Regexp.escape(@basedir)}.*/ =~ path
+          raise R10K::Error.new("Puppetfile cannot manage content '#{modname}' outside of containing environment: #{path} is not within #{@basedir}")
         end
 
         true
       end
 
       def determine_managed_directories(managed_content)
-        managed_content.keys.reject { |dir| dir == real_basedir }
+        managed_content.keys.reject { |dir| dir == @basedir }
       end
 
       # Returns an array of the full paths to all the content being managed.
@@ -177,8 +170,12 @@ def determine_purge_exclusions(managed_dirs)
         end
       end
 
-      def real_basedir
-        Pathname.new(@basedir).cleanpath.to_s
+      # .cleanpath is as good as we can do without touching the filesystem.
+      # The .realpath methods will choke if some of the intermediate paths
+      # are missing, even though in some cases we will create them later as
+      # needed.
+      def cleanpath(path)
+        Pathname.new(path).cleanpath.to_s
       end
 
       # For testing purposes only

spec/unit/module_loader/puppetfile_spec.rb

@@ -4,25 +4,47 @@
 describe R10K::ModuleLoader::Puppetfile do
   describe 'initial parameters' do
     describe 'honor' do
-      subject do
-        R10K::ModuleLoader::Puppetfile.new(basedir: '/test/basedir/env',
-                                           moduledir: '/test/basedir/env/dist-modules',
-                                           puppetfile: '/test/basedir/env/puppetfile.prod',
-                                           forge: 'localforge.internal.corp',
-                                           overrides: { modules: { deploy_modules: true } },
-                                           environment: R10K::Environment::Git.new('env',
-                                                                                   '/test/basedir/',
-                                                                                   'env',
-                                                                                   { remote: 'git://foo/remote',
-                                                                                     ref: 'env' }))
+      let(:options) do
+        {
+          basedir: '/test/basedir/env',
+          forge: 'localforge.internal.corp',
+          overrides: { modules: { deploy_modules: true } },
+          environment: R10K::Environment::Git.new('env',
+                                                  '/test/basedir/',
+                                                  'env',
+                                                  { remote: 'git://foo/remote',
+                                                    ref: 'env' })
+        }
       end
 
-      it 'the moduledir' do
-        expect(subject.instance_variable_get(:@moduledir)).to eq('/test/basedir/env/dist-modules')
+      subject { R10K::ModuleLoader::Puppetfile.new(**options) }
+
+      describe 'the moduledir' do
+        it 'respects absolute paths' do
+          absolute_options = options.merge({moduledir: '/opt/puppetlabs/special/modules'})
+          puppetfile = R10K::ModuleLoader::Puppetfile.new(**absolute_options)
+          expect(puppetfile.instance_variable_get(:@moduledir)).to eq('/opt/puppetlabs/special/modules')
+        end
+
+        it 'roots the moduledir in the basepath if a relative path is specified' do
+          relative_options = options.merge({moduledir: 'my/special/modules'})
+          puppetfile = R10K::ModuleLoader::Puppetfile.new(**relative_options)
+          expect(puppetfile.instance_variable_get(:@moduledir)).to eq('/test/basedir/env/my/special/modules')
+        end
       end
 
-      it 'the Puppetfile' do
-        expect(subject.instance_variable_get(:@puppetfile)).to eq('/test/basedir/env/puppetfile.prod')
+      describe 'the Puppetfile' do
+        it 'respects absolute paths' do
+          absolute_options = options.merge({puppetfile: '/opt/puppetlabs/special/Puppetfile'})
+          puppetfile = R10K::ModuleLoader::Puppetfile.new(**absolute_options)
+          expect(puppetfile.instance_variable_get(:@puppetfile)).to eq('/opt/puppetlabs/special/Puppetfile')
+        end
+
+        it 'roots the Puppetfile in the basepath if a relative path is specified' do
+          relative_options = options.merge({puppetfile: 'Puppetfile.global'})
+          puppetfile = R10K::ModuleLoader::Puppetfile.new(**relative_options)
+          expect(puppetfile.instance_variable_get(:@puppetfile)).to eq('/test/basedir/env/Puppetfile.global')
+        end
       end
 
       it 'the forge' do
@@ -269,6 +291,22 @@ def expect_wrapped_error(orig, pf_path, wrapped_error)
       expect { subject.load! }.not_to raise_error
     end
 
+    describe 'setting a custom moduledir' do
+      it 'allows setting an absolute moduledir' do
+        @path = '/fake/basedir'
+        allow(subject).to receive(:puppetfile_content).and_return('moduledir "/fake/moduledir"')
+        subject.load!
+        expect(subject.instance_variable_get(:@moduledir)).to eq('/fake/moduledir')
+      end
+
+      it 'roots relative moduledirs in the basedir' do
+        @path = '/fake/basedir'
+        allow(subject).to receive(:puppetfile_content).and_return('moduledir "my/moduledir"')
+        subject.load!
+        expect(subject.instance_variable_get(:@moduledir)).to eq(File.join(@path, 'my/moduledir'))
+      end
+    end
+
     it 'accepts a forge module without a version' do
       @path = File.join(PROJECT_ROOT, 'spec', 'fixtures', 'unit', 'puppetfile', 'valid-forge-without-version')
       pf_path = File.join(@path, 'Puppetfile')