Apply suggestions from code review

Co-authored-by: Molly Waggett <mwaggett@alumni.reed.edu>

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
 -----

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

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

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,15 +75,15 @@
 
   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
       expect(returned.map(&:name).sort).to eq(%w[concat exec stdlib])
 
       # 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