From a153c84dfba737fa5732f8440a8e1ec1ed796f28 Mon Sep 17 00:00:00 2001 From: Justin Stoller Date: Tue, 29 Jun 2021 14:45:06 -0700 Subject: [PATCH] (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 | 53 ++++++++--------- spec/unit/module_loader/puppetfile_spec.rb | 68 +++++++++++++++++----- 2 files changed, 78 insertions(+), 43 deletions(-) diff --git a/lib/r10k/module_loader/puppetfile.rb b/lib/r10k/module_loader/puppetfile.rb index b0a52220d..6e4170f6a 100644 --- a/lib/r10k/module_loader/puppetfile.rb +++ b/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 diff --git a/spec/unit/module_loader/puppetfile_spec.rb b/spec/unit/module_loader/puppetfile_spec.rb index a75678358..25f7b51f6 100644 --- a/spec/unit/module_loader/puppetfile_spec.rb +++ b/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')