From 8c097feea0fdcd35afd5f27881b3cf9a499bdedd Mon Sep 17 00:00:00 2001 From: Reid Vandewiele Date: Fri, 4 Dec 2020 15:58:42 -0800 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Molly Waggett --- CHANGELOG.mkd | 2 +- doc/dynamic-environments/configuration.mkd | 4 ++-- lib/r10k/environment/with_modules.rb | 15 ++++++++------- spec/unit/environment/with_modules_spec.rb | 7 ++++--- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.mkd b/CHANGELOG.mkd index dfd750ffd..ae00799fd 100644 --- a/CHANGELOG.mkd +++ b/CHANGELOG.mkd @@ -5,7 +5,7 @@ Unreleased ---------- - Fetch modules from the repo's default branch, rather than defaulting to 'master' [#1096](https://github.com/puppetlabs/r10k/issues/1096) -- Enable configurable behavior of environment module / Puppetfile module conflicts (experimental features) +- Experimental feature change: conflicts between environment-defined modules and Puppetfile-defined modules now default to logging a warning and deploying the environment module version, overriding the Puppetfile. Previously, conflicts would result in an error. The behavior is now configurable via the `module_conflicts` environment setting [#1107](https://github.com/puppetlabs/r10k/pull/1107) 3.7.0 ----- diff --git a/doc/dynamic-environments/configuration.mkd b/doc/dynamic-environments/configuration.mkd index 1916e0c55..6066e1f7f 100644 --- a/doc/dynamic-environments/configuration.mkd +++ b/doc/dynamic-environments/configuration.mkd @@ -679,8 +679,8 @@ When a module is defined in an environment and also in a Puppetfile, the default Available `module_conflicts` options: -* `override_puppetfile_and_warn` (default): the version of the module defined by the enviornment will be used, and the version defined in the Puppetfile will be ignored. A warning will be printed. -* `override_puppetfile`: the version of the module defined by the enviornment will be used, and the version defined in the Puppetfile will be ignored. +* `override_puppetfile_and_warn` (default): the version of the module defined by the environment will be used, and the version defined in the Puppetfile will be ignored. A warning will be printed. +* `override_puppetfile`: the version of the module defined by the environment will be used, and the version defined in the Puppetfile will be ignored. * `error`: an error will be raised alerting the user to the conflict. The environment will not be deployed. ```yaml diff --git a/lib/r10k/environment/with_modules.rb b/lib/r10k/environment/with_modules.rb index d483294aa..be9b504ba 100644 --- a/lib/r10k/environment/with_modules.rb +++ b/lib/r10k/environment/with_modules.rb @@ -103,20 +103,21 @@ def validate_no_module_conflicts .select { |_, v| v.size > 1 } .map(&:first) unless conflicts.empty? + conflicts_str = conflicts.join(', ') + log_msg = _('Environment and Puppetfile both define the following modules, Puppetfile ' \ + 'definition will be ignored: %{mods}' % { mods: conflicts_str }) case conflict_opt = @options[:module_conflicts] when 'override_puppetfile' - logger.debug _('Environment and Puppetfile both define the following modules, Puppetfile ' \ - 'definition will be ignored: %{mods}' % { mods: conflicts.join(', ') }) + logger.debug log_msg when 'override_puppetfile_and_warn', nil - logger.warn _('Environment and Puppetfile both define the following modules, Puppetfile ' \ - 'definition will be ignored: %{mods}' % { mods: conflicts.join(', ') }) + logger.warn log_msg when 'error' raise R10K::Error, _('Puppetfile cannot contain module names defined by environment ' \ - '%{name}; Remove the conflicting definitions of the following modules: ' \ - '%{mods}' % { name: self.name, mods: conflicts.join(', ') }) + '%{env}; Remove the conflicting definitions of the following modules: ' \ + '%{mods}' % { env: self.name, mods: conflicts_str }) else raise R10K::Error, _('Unexpected value for `module_conflicts` setting in %{env} ' \ - 'environment: %{val}' % {env: 'name', val: conflict_opt}) + 'environment: %{val}' % {env: self.name, val: conflict_opt}) end end end diff --git a/spec/unit/environment/with_modules_spec.rb b/spec/unit/environment/with_modules_spec.rb index 43f66f7b5..7fe3a3ee6 100644 --- a/spec/unit/environment/with_modules_spec.rb +++ b/spec/unit/environment/with_modules_spec.rb @@ -36,13 +36,14 @@ mod = double('duplicate-stdlib', :name => 'stdlib') expect(subject.puppetfile).to receive(:load) expect(subject.puppetfile).to receive(:modules).and_return [mod] + expect(subject.logger).to receive(:warn).with(/Puppetfile.*both define.*ignored/i) expect { subject.validate_no_module_conflicts }.not_to raise_error end end context "with module conflicts and 'error' behavior" do let(:subject_params) {{ :module_conflicts => 'error' }} - it "does not raise an error" do + it "raises an error" do mod = double('duplicate-stdlib', :name => 'stdlib') expect(subject.puppetfile).to receive(:load) expect(subject.puppetfile).to receive(:modules).and_return [mod] @@ -74,7 +75,7 @@ describe "modules method" do it "overrides duplicates, choosing the environment version" do - mod = double('duplicate-stdlib', :name => 'stdlib', :tag => :double) + mod = double('duplicate-stdlib', :name => 'stdlib', :giveaway => :'i-am-a-double') expect(subject.puppetfile).to receive(:load) expect(subject.puppetfile).to receive(:modules).and_return [mod] returned = subject.modules @@ -82,7 +83,7 @@ # Make sure the module that was picked was the environment one, not the Puppetfile one stdlib = returned.find { |m| m.name == 'stdlib' } - expect(stdlib.respond_to?(:tag)).to eq(false) + expect(stdlib.respond_to?(:giveaway)).to eq(false) end end end