Implement less restrictive env module merge

CHANGELOG.mkd

@@ -5,6 +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)
 
 3.7.0
 -----

doc/dynamic-environments/configuration.mkd

@@ -673,6 +673,31 @@ modules:
     ref: 62d07f2
 ```
 
+#### Puppetfile module conflicts
+
+When a module is defined in an environment and also in a Puppetfile, the default behavior is for the environment definition of the module to take precedence, a warning to be logged, and the Puppetfile definition to be ignored. The behavior is configurable to optionally skip the warning, or allow a hard failure instead. Use the `module_conflicts` option in an environment definition to control this.
+
+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.
+* `error`: an error will be raised alerting the user to the conflict. The environment will not be deployed.
+
+```yaml
+# production.yaml
+---
+type: git
+remote: git@github.com:puppetlabs/control-repo.git
+ref: 8820892
+module_conflicts: override_puppetfile_and_warn
+modules:
+  puppetlabs-stdlib: 6.0.0
+  puppetlabs-concat: 6.1.0
+  reidmv-xampl:
+    git: https://github.com/reidmv/reidmv-xampl.git
+    ref: 62d07f2
+```
+
 ### Bare Environment Type
 
 A "control repository" typically contains a hiera.yaml, an environment.conf, a manifests/site.pp file, and a few other things. However, none of these are strictly necessary for an environment to be functional if modules can be deployed to it.

lib/r10k/environment/with_modules.rb

@@ -49,7 +49,9 @@ def modules
     return @modules if @puppetfile.nil?
 
     @puppetfile.load unless @puppetfile.loaded?
-    @modules + @puppetfile.modules
+
+    env_mod_names = @modules.map(&:name)
+    @modules + @puppetfile.modules.select { |mod| !env_mod_names.include?(mod.name) }
   end
 
   def accept(visitor)
@@ -101,10 +103,21 @@ def validate_no_module_conflicts
                 .select { |_, v| v.size > 1 }
                 .map(&:first)
     unless conflicts.empty?
-      msg = _('Puppetfile cannot contain module names defined by environment %{name}') % {name: self.name}
-      msg += ' '
-      msg += _("Remove the conflicting definitions of the following modules: %{conflicts}" % { conflicts: conflicts.join(' ') })
-      raise R10K::Error.new(msg)
+      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(', ') })
+      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(', ') })
+      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(', ') })
+      else
+        raise R10K::Error, _('Unexpected value for `module_conflicts` setting in %{env} ' \
+                             'environment: %{val}' % {env: 'name', val: conflict_opt})
+      end
     end
   end
 

spec/unit/environment/git_spec.rb

@@ -62,9 +62,10 @@
 
   describe "enumerating modules" do
     it "loads the Puppetfile and returns modules in that puppetfile" do
+      mod = double('A module', :name => 'dbl')
       expect(subject.puppetfile).to receive(:load)
-      expect(subject.puppetfile).to receive(:modules).and_return [:modules]
-      expect(subject.modules).to eq([:modules])
+      expect(subject.puppetfile).to receive(:modules).and_return [mod]
+      expect(subject.modules).to eq([mod])
     end
   end
 

spec/unit/environment/with_modules_spec.rb

@@ -0,0 +1,88 @@
+require 'spec_helper'
+require 'r10k/environment'
+
+describe R10K::Environment::WithModules do
+  subject do
+    described_class.new(
+      'release42',
+      '/some/nonexistent/environmentdir',
+      'prefix_release42',
+      {
+        :type             => 'bare',
+        :modules          => {
+          'puppetlabs-stdlib' => '6.0.0',
+          'puppetlabs-concat' => '6.1.0',
+          'puppetlabs-exec'   => '0.5.0',
+        }
+      }.merge(subject_params)
+    )
+  end
+
+  # Default no additional params
+  let(:subject_params) { {} }
+
+  describe "dealing with Puppetfile module conflicts" do
+    context "with no module conflicts" do
+      it "validates when there are no conflicts" do
+        mod = double('module', :name => 'nonconflict')
+        expect(subject.puppetfile).to receive(:load)
+        expect(subject.puppetfile).to receive(:modules).and_return [mod]
+        expect { subject.validate_no_module_conflicts }.not_to raise_error
+      end
+    end
+
+    context "with module conflicts and default behavior" do
+      it "does not raise an error" do
+        mod = double('duplicate-stdlib', :name => 'stdlib')
+        expect(subject.puppetfile).to receive(:load)
+        expect(subject.puppetfile).to receive(:modules).and_return [mod]
+        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
+        mod = double('duplicate-stdlib', :name => 'stdlib')
+        expect(subject.puppetfile).to receive(:load)
+        expect(subject.puppetfile).to receive(:modules).and_return [mod]
+        expect { subject.validate_no_module_conflicts }.to raise_error(R10K::Error, /Puppetfile.*defined.*conflict/i)
+      end
+    end
+
+    context "with module conflicts and 'override_puppetfile' behavior" do
+      let(:subject_params) {{ :module_conflicts => 'override_puppetfile' }}
+      it "does not raise an error" do
+        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(:debug).with(/Puppetfile.*both define.*ignored/i)
+        expect { subject.validate_no_module_conflicts }.not_to raise_error
+      end
+    end
+
+    context "with module conflicts and invalid configuration" do
+      let(:subject_params) {{ :module_conflicts => 'batman' }}
+      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]
+        expect { subject.validate_no_module_conflicts }.to raise_error(R10K::Error, /Unexpected value.*module_conflicts/i)
+      end
+    end
+  end
+
+  describe "modules method" do
+    it "overrides duplicates, choosing the environment version" do
+      mod = double('duplicate-stdlib', :name => 'stdlib', :tag => :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)
+    end
+  end
+end