From 1ff99959eabb89509ec4e48df37bfaeea50f8558 Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Mon, 3 May 2021 15:02:46 -0700
Subject: [PATCH 01/11] (maint) Force Module.new to take a Hash of args

Prior to this a Puppetfile could declare a module with:
```
mod 'name'
mod 'name', :latest
mod 'name', 'version'
mod 'name', key_one: 'foo', key_two: ...
```

The optional second parameter(s) were then passed down through the
Puppetfile#add_module, Module.implements, and Module.new signatures
as the `args` parameter.

Being able to assume that the `args` parameter is a Hash simplifies the
code in several places - the exception being the Forge Module
implementation (which is the only module where anything but a Hash as the
args was ever valid) and the DSL methods that convert user input into
---
 lib/r10k/environment/with_modules.rb | 24 +++++++++---------
 lib/r10k/module.rb                   |  2 +-
 lib/r10k/module/forge.rb             | 27 +++++++++++---------
 lib/r10k/module/git.rb               |  2 +-
 lib/r10k/module/svn.rb               |  2 +-
 lib/r10k/puppetfile.rb               | 37 +++++++++++++++++++---------
 spec/unit/module/forge_spec.rb       | 26 +++++++++----------
 spec/unit/module_spec.rb             | 10 ++++----
 spec/unit/puppetfile_spec.rb         |  8 +++---
 9 files changed, 78 insertions(+), 60 deletions(-)

diff --git a/lib/r10k/environment/with_modules.rb b/lib/r10k/environment/with_modules.rb
index 419dfcd8e..3a55c4136 100644
--- a/lib/r10k/environment/with_modules.rb
+++ b/lib/r10k/environment/with_modules.rb
@@ -87,6 +87,10 @@ def accept(visitor)
 
   def load_modules(module_hash)
     module_hash.each do |name, args|
+      if !args.is_a?(Hash)
+        args = { version: args }
+      end
+
       add_module(name, args)
     end
   end
@@ -94,18 +98,16 @@ def load_modules(module_hash)
   # @param [String] name
   # @param [*Object] args
   def add_module(name, args)
-    if args.is_a?(Hash)
-      # symbolize keys in the args hash
-      args = args.inject({}) { |memo,(k,v)| memo[k.to_sym] = v; memo }
-      args[:overrides] = @overrides
-
-      if install_path = args.delete(:install_path)
-        install_path = resolve_install_path(install_path)
-        validate_install_path(install_path, name)
-      end
-    end
+    # symbolize keys in the args hash
+    args = args.inject({}) { |memo,(k,v)| memo[k.to_sym] = v; memo }
+    args[:overrides] = @overrides
 
-    install_path ||= @moduledir
+    if install_path = args.delete(:install_path)
+      install_path = resolve_install_path(install_path)
+      validate_install_path(install_path, name)
+    else
+      install_path = @moduledir
+    end
 
     # Keep track of all the content this environment is managing to enable purging.
     @managed_content[install_path] = Array.new unless @managed_content.has_key?(install_path)
diff --git a/lib/r10k/module.rb b/lib/r10k/module.rb
index e28a5ef74..79932af28 100644
--- a/lib/r10k/module.rb
+++ b/lib/r10k/module.rb
@@ -17,7 +17,7 @@ def self.register(klass)
   #
   # @param [String] name The unique name of the module
   # @param [String] basedir The root to install the module in
-  # @param [Object] args An arbitary value or set of values that specifies the implementation
+  # @param [Hash] args An arbitary Hash that specifies the implementation
   # @param [R10K::Environment] environment Optional environment that this module is a part of
   #
   # @return [Object < R10K::Module] A member of the implementing subclass
diff --git a/lib/r10k/module/forge.rb b/lib/r10k/module/forge.rb
index 3c1790701..d9d4251cd 100644
--- a/lib/r10k/module/forge.rb
+++ b/lib/r10k/module/forge.rb
@@ -13,7 +13,12 @@ class R10K::Module::Forge < R10K::Module::Base
   R10K::Module.register(self)
 
   def self.implement?(name, args)
-    (args.is_a?(Hash) && args[:type].to_s == 'forge') || (!!(name.match %r[\w+[/-]\w+]) && valid_version?(args))
+    args[:type].to_s == 'forge' ||
+      (!!
+       ((args.keys & %i{git svn type}).empty? &&
+        args.has_key?(:version) &&
+        name.match(%r[\w+[/-]\w+]) &&
+        valid_version?(args[:version])))
   end
 
   def self.valid_version?(expected_version)
@@ -40,17 +45,15 @@ def initialize(title, dirname, opts, environment=nil)
     @metadata_file = R10K::Module::MetadataFile.new(path + 'metadata.json')
     @metadata = @metadata_file.read
 
-    if opts.is_a?(Hash)
-      setopts(opts, {
-        # Standard option interface
-        :version   => :expected_version,
-        :source    => ::R10K::Util::Setopts::Ignore,
-        :type      => ::R10K::Util::Setopts::Ignore,
-        :overrides => :self,
-      })
-    else
-      @expected_version = opts || current_version || :latest
-    end
+    setopts(opts, {
+      # Standard option interface
+      :version   => :expected_version,
+      :source    => ::R10K::Util::Setopts::Ignore,
+      :type      => ::R10K::Util::Setopts::Ignore,
+      :overrides => :self,
+    })
+
+    @expected_version ||= current_version || :latest
 
     @v3_module = PuppetForge::V3::Module.new(:slug => @title)
   end
diff --git a/lib/r10k/module/git.rb b/lib/r10k/module/git.rb
index 957439b52..3c97b78e8 100644
--- a/lib/r10k/module/git.rb
+++ b/lib/r10k/module/git.rb
@@ -8,7 +8,7 @@ class R10K::Module::Git < R10K::Module::Base
   R10K::Module.register(self)
 
   def self.implement?(name, args)
-    args.is_a?(Hash) && (args.has_key?(:git) || args[:type].to_s == 'git')
+    args.has_key?(:git) || args[:type].to_s == 'git'
   rescue
     false
   end
diff --git a/lib/r10k/module/svn.rb b/lib/r10k/module/svn.rb
index b20b2dcb4..35ecb2cd8 100644
--- a/lib/r10k/module/svn.rb
+++ b/lib/r10k/module/svn.rb
@@ -7,7 +7,7 @@ class R10K::Module::SVN < R10K::Module::Base
   R10K::Module.register(self)
 
   def self.implement?(name, args)
-    args.is_a?(Hash) && (args.has_key?(:svn) || args[:type].to_s == 'svn')
+    args.has_key?(:svn) || args[:type].to_s == 'svn'
   end
 
   # @!attribute [r] expected_revision
diff --git a/lib/r10k/puppetfile.rb b/lib/r10k/puppetfile.rb
index 3d0758bb5..56968790d 100644
--- a/lib/r10k/puppetfile.rb
+++ b/lib/r10k/puppetfile.rb
@@ -133,22 +133,29 @@ def set_moduledir(moduledir)
   end
 
   # @param [String] name
-  # @param [*Object] args
+  # @param [Hash, String, Symbol] args Calling with anything but a Hash is
+  #   deprecated. The DSL will now convert String and Symbol versions to
+  #   Hashes of the shape
+  #     { version: <String or Symbol> }
+  #
   def add_module(name, args)
-    if args.is_a?(Hash)
-      args[:overrides] = @overrides
+    if !args.is_a?(Hash)
+      args = { version: args }
+    end
 
-      if install_path = args.delete(:install_path)
-        install_path = resolve_install_path(install_path)
-        validate_install_path(install_path, name)
-      end
+    args[:overrides] = @overrides
 
-      if @default_branch_override != nil
-        args[:default_branch_override] = @default_branch_override
-      end
+    if install_path = args.delete(:install_path)
+      install_path = resolve_install_path(install_path)
+      validate_install_path(install_path, name)
+    else
+      install_path = @moduledir
+    end
+
+    if @default_branch_override != nil
+      args[:default_branch_override] = @default_branch_override
     end
 
-    install_path ||= @moduledir
 
     mod = R10K::Module.new(name, install_path, args, @environment)
     mod.origin = :puppetfile
@@ -248,7 +255,13 @@ def initialize(librarian)
     end
 
     def mod(name, args = nil)
-      @librarian.add_module(name, args)
+      if args.is_a?(Hash)
+        opts = args
+      else
+        opts = { version: args }
+      end
+
+      @librarian.add_module(name, opts)
     end
 
     def forge(location)
diff --git a/spec/unit/module/forge_spec.rb b/spec/unit/module/forge_spec.rb
index cc9fb1253..ab7a8b612 100644
--- a/spec/unit/module/forge_spec.rb
+++ b/spec/unit/module/forge_spec.rb
@@ -11,15 +11,15 @@
 
   describe "implementing the Puppetfile spec" do
     it "should implement 'branan/eight_hundred', '8.0.0'" do
-      expect(described_class).to be_implement('branan/eight_hundred', '8.0.0')
+      expect(described_class).to be_implement('branan/eight_hundred', { version: '8.0.0' })
     end
 
     it "should implement 'branan-eight_hundred', '8.0.0'" do
-      expect(described_class).to be_implement('branan-eight_hundred', '8.0.0')
+      expect(described_class).to be_implement('branan-eight_hundred', { version: '8.0.0' })
     end
 
     it "should fail with an invalid title" do
-      expect(described_class).to_not be_implement('branan!eight_hundred', '8.0.0')
+      expect(described_class).to_not be_implement('branan!eight_hundred', { version: '8.0.0' })
     end
   end
 
@@ -30,7 +30,7 @@
   end
 
   describe "setting attributes" do
-    subject { described_class.new('branan/eight_hundred', '/moduledir', '8.0.0') }
+    subject { described_class.new('branan/eight_hundred', '/moduledir', { version: '8.0.0' }) }
 
     it "sets the name" do
       expect(subject.name).to eq 'eight_hundred'
@@ -50,7 +50,7 @@
   end
 
   describe "properties" do
-    subject { described_class.new('branan/eight_hundred', fixture_modulepath, '8.0.0') }
+    subject { described_class.new('branan/eight_hundred', fixture_modulepath, { version: '8.0.0' }) }
 
     it "sets the module type to :forge" do
       expect(subject.properties).to include(:type => :forge)
@@ -67,7 +67,7 @@
   end
 
   context "when a module is deprecated" do
-    subject { described_class.new('puppetlabs/corosync', fixture_modulepath, :latest) }
+    subject { described_class.new('puppetlabs/corosync', fixture_modulepath, { version: :latest }) }
 
     it "warns on sync if module is not already insync" do
       allow(subject).to receive(:status).and_return(:absent)
@@ -96,12 +96,12 @@
 
   describe '#expected_version' do
     it "returns an explicitly given expected version" do
-      subject = described_class.new('branan/eight_hundred', fixture_modulepath, '8.0.0')
+      subject = described_class.new('branan/eight_hundred', fixture_modulepath, { version: '8.0.0' })
       expect(subject.expected_version).to eq '8.0.0'
     end
 
     it "uses the latest version from the forge when the version is :latest" do
-      subject = described_class.new('branan/eight_hundred', fixture_modulepath, :latest)
+      subject = described_class.new('branan/eight_hundred', fixture_modulepath, { version: :latest })
       expect(subject.v3_module).to receive_message_chain(:current_release, :version).and_return('8.8.8')
       expect(subject.expected_version).to eq '8.8.8'
     end
@@ -109,7 +109,7 @@
 
   describe "determining the status" do
 
-    subject { described_class.new('branan/eight_hundred', fixture_modulepath, '8.0.0') }
+    subject { described_class.new('branan/eight_hundred', fixture_modulepath, { version: '8.0.0' }) }
 
     it "is :absent if the module directory is absent" do
       allow(subject).to receive(:exist?).and_return false
@@ -154,7 +154,7 @@
   end
 
   describe "#sync" do
-    subject { described_class.new('branan/eight_hundred', fixture_modulepath, '8.0.0') }
+    subject { described_class.new('branan/eight_hundred', fixture_modulepath, { version: '8.0.0' }) }
 
     it 'does nothing when the module is in sync' do
       allow(subject).to receive(:status).and_return :insync
@@ -186,7 +186,7 @@
 
   describe '#install' do
     it 'installs the module from the forge' do
-      subject = described_class.new('branan/eight_hundred', fixture_modulepath, '8.0.0')
+      subject = described_class.new('branan/eight_hundred', fixture_modulepath, { version: '8.0.0' })
       release = instance_double('R10K::Forge::ModuleRelease')
       expect(R10K::Forge::ModuleRelease).to receive(:new).with('branan-eight_hundred', '8.0.0').and_return(release)
       expect(release).to receive(:install).with(subject.path)
@@ -196,7 +196,7 @@
 
   describe '#uninstall' do
     it 'removes the module path' do
-      subject = described_class.new('branan/eight_hundred', fixture_modulepath, '8.0.0')
+      subject = described_class.new('branan/eight_hundred', fixture_modulepath, { version: '8.0.0' })
       expect(FileUtils).to receive(:rm_rf).with(subject.path.to_s)
       subject.uninstall
     end
@@ -204,7 +204,7 @@
 
   describe '#reinstall' do
     it 'uninstalls and then installs the module' do
-      subject = described_class.new('branan/eight_hundred', fixture_modulepath, '8.0.0')
+      subject = described_class.new('branan/eight_hundred', fixture_modulepath, { version: '8.0.0' })
       expect(subject).to receive(:uninstall)
       expect(subject).to receive(:install)
       subject.reinstall
diff --git a/spec/unit/module_spec.rb b/spec/unit/module_spec.rb
index 6f9ccad37..ec5022ed3 100644
--- a/spec/unit/module_spec.rb
+++ b/spec/unit/module_spec.rb
@@ -30,12 +30,12 @@
     [ 'bar/quux',
       'bar-quux',
     ].each do |scenario|
-      it "accepts a name matching #{scenario} and args nil" do
-        obj = R10K::Module.new(scenario, '/modulepath', nil)
+      it "accepts a name matching #{scenario} and version nil" do
+        obj = R10K::Module.new(scenario, '/modulepath', { version: nil })
         expect(obj).to be_a_kind_of(R10K::Module::Forge)
       end
     end
-    [ '8.0.0',
+    [ {version: '8.0.0'},
       {type: 'forge', version: '8.0.0'},
     ].each do |scenario|
       it "accepts a name matching bar-quux and args #{scenario.inspect}" do
@@ -65,7 +65,7 @@
       end
 
       it 'sets the expected version to what is found in the metadata' do
-        obj = R10K::Module.new(@title, @dirname, nil)
+        obj = R10K::Module.new(@title, @dirname, {version: nil})
         expect(obj.send(:instance_variable_get, :'@expected_version')).to eq('1.2.0')
       end
     end
@@ -73,7 +73,7 @@
 
   it "raises an error if delegation fails" do
     expect {
-      R10K::Module.new('bar!quux', '/modulepath', ["NOPE NOPE NOPE NOPE!"])
+      R10K::Module.new('bar!quux', '/modulepath', {version: ["NOPE NOPE NOPE NOPE!"]})
     }.to raise_error RuntimeError, /doesn't have an implementation/
   end
 end
diff --git a/spec/unit/puppetfile_spec.rb b/spec/unit/puppetfile_spec.rb
index 653a0b4c4..8147c0a7d 100644
--- a/spec/unit/puppetfile_spec.rb
+++ b/spec/unit/puppetfile_spec.rb
@@ -68,15 +68,15 @@
   end
 
   describe "adding modules" do
-    it "should accept Forge modules with a string arg" do
-      allow(R10K::Module).to receive(:new).with('puppet/test_module', subject.moduledir, '1.2.3', anything).and_call_original
+    it "should transform Forge modules with a string arg to have a version key" do
+      allow(R10K::Module).to receive(:new).with('puppet/test_module', subject.moduledir, hash_including(version: '1.2.3'), anything).and_call_original
 
       expect { subject.add_module('puppet/test_module', '1.2.3') }.to change { subject.modules }
       expect(subject.modules.collect(&:name)).to include('test_module')
     end
 
     it "should not accept Forge modules with a version comparison" do
-      allow(R10K::Module).to receive(:new).with('puppet/test_module', subject.moduledir, '< 1.2.0', anything).and_call_original
+      allow(R10K::Module).to receive(:new).with('puppet/test_module', subject.moduledir, hash_including(version: '< 1.2.0'), anything).and_call_original
 
       expect {
         subject.add_module('puppet/test_module', '< 1.2.0')
@@ -181,7 +181,7 @@
 
   describe '#managed_directories' do
     it 'returns an array of paths that can be purged' do
-      allow(R10K::Module).to receive(:new).with('puppet/test_module', subject.moduledir, '1.2.3', anything).and_call_original
+      allow(R10K::Module).to receive(:new).with('puppet/test_module', subject.moduledir, hash_including(version: '1.2.3'), anything).and_call_original
 
       subject.add_module('puppet/test_module', '1.2.3')
       expect(subject.managed_directories).to match_array(["/some/nonexistent/basedir/modules"])

From 06271da7fdd84f82824b8ab649b5e237bddeccea Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Mon, 3 May 2021 15:22:54 -0700
Subject: [PATCH 02/11] (maint) Deprecate the usage of Module#sync options

The Git implementation was the only user of options to sync, and the
value passed is already passed into the Module's initialization.
---
 lib/r10k/action/deploy/environment.rb       | 2 +-
 lib/r10k/action/deploy/module.rb            | 2 +-
 lib/r10k/action/puppetfile/install.rb       | 5 ++---
 lib/r10k/module/base.rb                     | 1 +
 lib/r10k/module/forge.rb                    | 1 +
 lib/r10k/module/git.rb                      | 6 +++++-
 lib/r10k/module/local.rb                    | 1 +
 lib/r10k/module/svn.rb                      | 1 +
 spec/unit/action/puppetfile/install_spec.rb | 8 ++++----
 9 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/lib/r10k/action/deploy/environment.rb b/lib/r10k/action/deploy/environment.rb
index e81a2c667..c4dd9f3e3 100644
--- a/lib/r10k/action/deploy/environment.rb
+++ b/lib/r10k/action/deploy/environment.rb
@@ -184,7 +184,7 @@ def visit_puppetfile(puppetfile)
 
         def visit_module(mod)
           logger.info _("Deploying %{origin} content %{path}") % {origin: mod.origin, path: mod.path}
-          mod.sync(force: @settings.dig(:overrides, :modules, :force))
+          mod.sync
         end
 
         def write_environment_info!(environment, started_at, success)
diff --git a/lib/r10k/action/deploy/module.rb b/lib/r10k/action/deploy/module.rb
index ff3ad80a5..dfa24271d 100644
--- a/lib/r10k/action/deploy/module.rb
+++ b/lib/r10k/action/deploy/module.rb
@@ -80,7 +80,7 @@ def visit_module(mod)
           requested_mods = @settings.dig(:overrides, :modules, :requested_modules)
           if requested_mods.include?(mod.name)
             logger.info _("Deploying module %{mod_path}") % {mod_path: mod.path}
-            mod.sync(force: @settings.dig(:overrides, :modules, :force))
+            mod.sync
             if mod.environment && @settings.dig(:overrides, :environments, :generate_types)
               logger.debug("Generating puppet types for environment '#{mod.environment.dirname}'...")
               mod.environment.generate_types!
diff --git a/lib/r10k/action/puppetfile/install.rb b/lib/r10k/action/puppetfile/install.rb
index 8c75ac335..89c2e20d1 100644
--- a/lib/r10k/action/puppetfile/install.rb
+++ b/lib/r10k/action/puppetfile/install.rb
@@ -14,7 +14,7 @@ def call
           pf = R10K::Puppetfile.new(@root,
                                     {moduledir: @moduledir,
                                      puppetfile_path: @puppetfile,
-                                     force: @force})
+                                     force: @force || false})
           pf.accept(self)
           @visit_ok
         end
@@ -34,14 +34,13 @@ def visit_puppetfile(pf)
         end
 
         def visit_module(mod)
-          @force ||= false
           logger.info _("Updating module %{mod_path}") % {mod_path: mod.path}
 
           if mod.respond_to?(:desired_ref) && mod.desired_ref == :control_branch
             logger.warn _("Cannot track control repo branch for content '%{name}' when not part of a 'deploy' action, will use default if available." % {name: mod.name})
           end
 
-          mod.sync(force: @force)
+          mod.sync
         end
 
         def allowed_initialize_opts
diff --git a/lib/r10k/module/base.rb b/lib/r10k/module/base.rb
index 1b0a3ed2d..bdba932cb 100644
--- a/lib/r10k/module/base.rb
+++ b/lib/r10k/module/base.rb
@@ -61,6 +61,7 @@ def full_path
   end
 
   # Synchronize this module with the indicated state.
+  # @param [Hash] opts Deprecated
   # @abstract
   def sync(opts={})
     raise NotImplementedError
diff --git a/lib/r10k/module/forge.rb b/lib/r10k/module/forge.rb
index d9d4251cd..596008a3d 100644
--- a/lib/r10k/module/forge.rb
+++ b/lib/r10k/module/forge.rb
@@ -58,6 +58,7 @@ def initialize(title, dirname, opts, environment=nil)
     @v3_module = PuppetForge::V3::Module.new(:slug => @title)
   end
 
+  # @param [Hash] opts Deprecated
   def sync(opts={})
     case status
     when :absent
diff --git a/lib/r10k/module/git.rb b/lib/r10k/module/git.rb
index 3c97b78e8..66adb0c2f 100644
--- a/lib/r10k/module/git.rb
+++ b/lib/r10k/module/git.rb
@@ -54,6 +54,9 @@ def initialize(title, dirname, opts, environment=nil)
       :default_branch_override => :default_override_ref,
     })
 
+    force = @overrides && @overrides.dig(:modules, :force)
+    @force = force == false ? false : true
+
     @desired_ref ||= 'master'
 
     if @desired_ref == :control_branch && @environment && @environment.respond_to?(:ref)
@@ -75,8 +78,9 @@ def properties
     }
   end
 
+  # @param [Hash] opts Deprecated
   def sync(opts={})
-    force = opts && opts.fetch(:force, true)
+    force = opts[:force] || @force
     @repo.sync(version, force)
   end
 
diff --git a/lib/r10k/module/local.rb b/lib/r10k/module/local.rb
index 61add3e2d..3135a96aa 100644
--- a/lib/r10k/module/local.rb
+++ b/lib/r10k/module/local.rb
@@ -30,6 +30,7 @@ def status
     :insync
   end
 
+  # @param [Hash] opts Deprecated
   def sync(opts={})
     logger.debug1 _("Module %{title} is a local module, always indicating synced.") % {title: title}
   end
diff --git a/lib/r10k/module/svn.rb b/lib/r10k/module/svn.rb
index 35ecb2cd8..b49749ec5 100644
--- a/lib/r10k/module/svn.rb
+++ b/lib/r10k/module/svn.rb
@@ -70,6 +70,7 @@ def status
     end
   end
 
+  # @param [Hash] opts Deprecated
   def sync(opts={})
     case status
     when :absent
diff --git a/spec/unit/action/puppetfile/install_spec.rb b/spec/unit/action/puppetfile/install_spec.rb
index 6ba5148e1..953ee649a 100644
--- a/spec/unit/action/puppetfile/install_spec.rb
+++ b/spec/unit/action/puppetfile/install_spec.rb
@@ -5,7 +5,7 @@
   let(:default_opts) { { root: "/some/nonexistent/path" } }
   let(:puppetfile) {
     R10K::Puppetfile.new('/some/nonexistent/path',
-                         {:moduledir => nil, :puppetfile_path => nil, :force => nil})
+                         {:moduledir => nil, :puppetfile_path => nil, :force => false})
   }
 
   def installer(opts = {}, argv = [], settings = {})
@@ -17,7 +17,7 @@ def installer(opts = {}, argv = [], settings = {})
     allow(puppetfile).to receive(:load!).and_return(nil)
     allow(R10K::Puppetfile).to receive(:new).
       with("/some/nonexistent/path",
-           {:moduledir => nil, :puppetfile_path => nil, :force => nil}).
+           {:moduledir => nil, :puppetfile_path => nil, :force => false}).
       and_return(puppetfile)
   end
 
@@ -73,7 +73,7 @@ def installer(opts = {}, argv = [], settings = {})
     it "can use a custom puppetfile path" do
       expect(R10K::Puppetfile).to receive(:new).
         with("/some/nonexistent/path",
-             {:moduledir => nil, :force => nil, puppetfile_path: "/some/other/path/Puppetfile"}).
+             {:moduledir => nil, :force => false, puppetfile_path: "/some/other/path/Puppetfile"}).
         and_return(puppetfile)
 
       installer({puppetfile: "/some/other/path/Puppetfile"}).call
@@ -82,7 +82,7 @@ def installer(opts = {}, argv = [], settings = {})
     it "can use a custom moduledir path" do
       expect(R10K::Puppetfile).to receive(:new).
         with("/some/nonexistent/path",
-             {:puppetfile_path => nil, :force => nil, moduledir: "/some/other/path/site-modules"}).
+             {:puppetfile_path => nil, :force => false, moduledir: "/some/other/path/site-modules"}).
         and_return(puppetfile)
 
       installer({moduledir: "/some/other/path/site-modules"}).call

From 48cb15b888cab2d32f9d819b06818639c78b8233 Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Mon, 3 May 2021 15:39:31 -0700
Subject: [PATCH 03/11] (maint) Consolidate module sync logging

This consolidates the logging notifying users that a module is being
synchronized. Each caller provided slightly different messages so this
creates a new, generic message that should work for previous uses.
---
 lib/r10k/action/deploy/environment.rb | 1 -
 lib/r10k/action/deploy/module.rb      | 1 -
 lib/r10k/action/puppetfile/install.rb | 2 --
 lib/r10k/module/base.rb               | 3 +--
 lib/r10k/module/forge.rb              | 2 ++
 lib/r10k/module/git.rb                | 2 ++
 lib/r10k/module/svn.rb                | 2 ++
 spec/unit/module/forge_spec.rb        | 2 ++
 8 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/lib/r10k/action/deploy/environment.rb b/lib/r10k/action/deploy/environment.rb
index c4dd9f3e3..e1e26eaf2 100644
--- a/lib/r10k/action/deploy/environment.rb
+++ b/lib/r10k/action/deploy/environment.rb
@@ -183,7 +183,6 @@ def visit_puppetfile(puppetfile)
         end
 
         def visit_module(mod)
-          logger.info _("Deploying %{origin} content %{path}") % {origin: mod.origin, path: mod.path}
           mod.sync
         end
 
diff --git a/lib/r10k/action/deploy/module.rb b/lib/r10k/action/deploy/module.rb
index dfa24271d..4eb73b890 100644
--- a/lib/r10k/action/deploy/module.rb
+++ b/lib/r10k/action/deploy/module.rb
@@ -79,7 +79,6 @@ def visit_puppetfile(puppetfile)
         def visit_module(mod)
           requested_mods = @settings.dig(:overrides, :modules, :requested_modules)
           if requested_mods.include?(mod.name)
-            logger.info _("Deploying module %{mod_path}") % {mod_path: mod.path}
             mod.sync
             if mod.environment && @settings.dig(:overrides, :environments, :generate_types)
               logger.debug("Generating puppet types for environment '#{mod.environment.dirname}'...")
diff --git a/lib/r10k/action/puppetfile/install.rb b/lib/r10k/action/puppetfile/install.rb
index 89c2e20d1..4af3fed95 100644
--- a/lib/r10k/action/puppetfile/install.rb
+++ b/lib/r10k/action/puppetfile/install.rb
@@ -34,8 +34,6 @@ def visit_puppetfile(pf)
         end
 
         def visit_module(mod)
-          logger.info _("Updating module %{mod_path}") % {mod_path: mod.path}
-
           if mod.respond_to?(:desired_ref) && mod.desired_ref == :control_branch
             logger.warn _("Cannot track control repo branch for content '%{name}' when not part of a 'deploy' action, will use default if available." % {name: mod.name})
           end
diff --git a/lib/r10k/module/base.rb b/lib/r10k/module/base.rb
index bdba932cb..6dc0b1cf8 100644
--- a/lib/r10k/module/base.rb
+++ b/lib/r10k/module/base.rb
@@ -62,9 +62,8 @@ def full_path
 
   # Synchronize this module with the indicated state.
   # @param [Hash] opts Deprecated
-  # @abstract
   def sync(opts={})
-    raise NotImplementedError
+    logger.info _("Checking module %{path}") % {path: path}
   end
 
   # Return the desired version of this module
diff --git a/lib/r10k/module/forge.rb b/lib/r10k/module/forge.rb
index 596008a3d..9d8e53f86 100644
--- a/lib/r10k/module/forge.rb
+++ b/lib/r10k/module/forge.rb
@@ -60,6 +60,8 @@ def initialize(title, dirname, opts, environment=nil)
 
   # @param [Hash] opts Deprecated
   def sync(opts={})
+    super
+
     case status
     when :absent
       install
diff --git a/lib/r10k/module/git.rb b/lib/r10k/module/git.rb
index 66adb0c2f..cae6cd567 100644
--- a/lib/r10k/module/git.rb
+++ b/lib/r10k/module/git.rb
@@ -80,6 +80,8 @@ def properties
 
   # @param [Hash] opts Deprecated
   def sync(opts={})
+    super
+
     force = opts[:force] || @force
     @repo.sync(version, force)
   end
diff --git a/lib/r10k/module/svn.rb b/lib/r10k/module/svn.rb
index b49749ec5..f245bc0ed 100644
--- a/lib/r10k/module/svn.rb
+++ b/lib/r10k/module/svn.rb
@@ -72,6 +72,8 @@ def status
 
   # @param [Hash] opts Deprecated
   def sync(opts={})
+    super
+
     case status
     when :absent
       install
diff --git a/spec/unit/module/forge_spec.rb b/spec/unit/module/forge_spec.rb
index ab7a8b612..a98abd465 100644
--- a/spec/unit/module/forge_spec.rb
+++ b/spec/unit/module/forge_spec.rb
@@ -77,6 +77,7 @@
       logger_dbl = double(Log4r::Logger)
       allow_any_instance_of(described_class).to receive(:logger).and_return(logger_dbl)
 
+      allow(logger_dbl).to receive(:info).with(/Checking module.*/)
       expect(logger_dbl).to receive(:warn).with(/puppet forge module.*puppetlabs-corosync.*has been deprecated/i)
 
       subject.sync
@@ -88,6 +89,7 @@
       logger_dbl = double(Log4r::Logger)
       allow_any_instance_of(described_class).to receive(:logger).and_return(logger_dbl)
 
+      allow(logger_dbl).to receive(:info).with(/Checking module.*/)
       expect(logger_dbl).to_not receive(:warn).with(/puppet forge module.*puppetlabs-corosync.*has been deprecated/i)
 
       subject.sync

From dd7f8ffb0c8160ad2b84d0f3b5b9afc9d86e224a Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Tue, 4 May 2021 13:51:32 -0700
Subject: [PATCH 04/11] (maint) Move control branch warning to git module init

Prior to this when running `puppetfile install` users would receive a
warning whenever processing a git module with a control_branch setting
of :control_repo. This is because the puppetfile subcommands have no
concept of a containing environment that they operate within.

However, this warning should also be applicable when deploying git
modules with non-git backed environments (eg SVN or "bare"
environments). This warning is also only applicable to git backed
modules.

This patch moves the warning from the `puppetfile install` command to
the git module initialization and changes the message to be more general.
---
 lib/r10k/action/puppetfile/install.rb | 4 ----
 lib/r10k/module/git.rb                | 8 ++++++--
 spec/unit/module/git_spec.rb          | 8 ++++++++
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/lib/r10k/action/puppetfile/install.rb b/lib/r10k/action/puppetfile/install.rb
index 4af3fed95..e25755258 100644
--- a/lib/r10k/action/puppetfile/install.rb
+++ b/lib/r10k/action/puppetfile/install.rb
@@ -34,10 +34,6 @@ def visit_puppetfile(pf)
         end
 
         def visit_module(mod)
-          if mod.respond_to?(:desired_ref) && mod.desired_ref == :control_branch
-            logger.warn _("Cannot track control repo branch for content '%{name}' when not part of a 'deploy' action, will use default if available." % {name: mod.name})
-          end
-
           mod.sync
         end
 
diff --git a/lib/r10k/module/git.rb b/lib/r10k/module/git.rb
index cae6cd567..9623ed95d 100644
--- a/lib/r10k/module/git.rb
+++ b/lib/r10k/module/git.rb
@@ -59,8 +59,12 @@ def initialize(title, dirname, opts, environment=nil)
 
     @desired_ref ||= 'master'
 
-    if @desired_ref == :control_branch && @environment && @environment.respond_to?(:ref)
-      @desired_ref = @environment.ref
+    if @desired_ref == :control_branch
+      if @environment && @environment.respond_to?(:ref)
+        @desired_ref = @environment.ref
+      else
+        logger.warn _("Cannot track control repo branch for content '%{name}' when not part of a git-backed environment, will use default if available." % {name: name})
+      end
     end
 
     @repo = R10K::Git::StatefulRepository.new(@remote, @dirname, @name)
diff --git a/spec/unit/module/git_spec.rb b/spec/unit/module/git_spec.rb
index 231d3738b..3b1666658 100644
--- a/spec/unit/module/git_spec.rb
+++ b/spec/unit/module/git_spec.rb
@@ -210,6 +210,14 @@ def test_module(extra_opts, env=nil)
             expect(mod.desired_ref).to eq(:control_branch)
           end
 
+          it "warns control branch may be unresolvable" do
+            logger = double("logger")
+            allow_any_instance_of(described_class).to receive(:logger).and_return(logger)
+            expect(logger).to receive(:warn).with(/Cannot track control repo branch.*boolean.*/)
+
+            test_module(branch: :control_branch)
+          end
+
           context "when default ref is provided and resolvable" do
             it "uses default ref" do
               expect(mock_repo).to receive(:resolve).with('default').and_return('abc123')

From 1901284b05164f920c882a3281f062ecb6dafac2 Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Wed, 5 May 2021 11:16:38 -0700
Subject: [PATCH 05/11] (maint) Allow modules to decide if they should sync

Previously, the deploy module action would determine in the visit_module
method if the visited module should be synchronized.

This patch moves that logic into the Module::Base class and makes it
available to subclasses via the `will_sync?` method. It also updates
each module implementation's sync method to only act if `will_sync?`
returns true.
---
 lib/r10k/action/deploy/module.rb            | 17 +++----
 lib/r10k/environment/base.rb                |  6 +--
 lib/r10k/module/base.rb                     | 17 ++++++-
 lib/r10k/module/forge.rb                    | 25 +++++-----
 lib/r10k/module/git.rb                      | 24 +++++----
 lib/r10k/module/svn.rb                      | 35 +++++++------
 spec/unit/action/deploy/module_spec.rb      | 55 ++++++++++++++++++++-
 spec/unit/action/puppetfile/install_spec.rb |  2 +-
 8 files changed, 121 insertions(+), 60 deletions(-)

diff --git a/lib/r10k/action/deploy/module.rb b/lib/r10k/action/deploy/module.rb
index 4eb73b890..0cedc96ac 100644
--- a/lib/r10k/action/deploy/module.rb
+++ b/lib/r10k/action/deploy/module.rb
@@ -28,7 +28,7 @@ def initialize(opts, argv, settings)
                 generate_types: @generate_types
               },
               modules: {
-                requested_modules: @argv,
+                requested_modules: @argv.map.to_a,
                 # force here is used to make it easier to reason about
                 force: !@no_force
               },
@@ -77,15 +77,12 @@ def visit_puppetfile(puppetfile)
         end
 
         def visit_module(mod)
-          requested_mods = @settings.dig(:overrides, :modules, :requested_modules)
-          if requested_mods.include?(mod.name)
-            mod.sync
-            if mod.environment && @settings.dig(:overrides, :environments, :generate_types)
-              logger.debug("Generating puppet types for environment '#{mod.environment.dirname}'...")
-              mod.environment.generate_types!
-            end
-          else
-            logger.debug1(_("Only updating modules %{modules}, skipping module %{mod_name}") % {modules: requested_mods.inspect, mod_name: mod.name})
+          mod.sync
+
+          requested_mods = @settings.dig(:overrides, :modules, :requested_modules) || []
+          if requested_mods.include?(mod.name) && mod.environment && @settings.dig(:overrides, :environments, :generate_types)
+            logger.debug("Generating puppet types for environment '#{mod.environment.dirname}'...")
+            mod.environment.generate_types!
           end
         end
 
diff --git a/lib/r10k/environment/base.rb b/lib/r10k/environment/base.rb
index d02a70eaa..ac972f885 100644
--- a/lib/r10k/environment/base.rb
+++ b/lib/r10k/environment/base.rb
@@ -44,14 +44,14 @@ def initialize(name, basedir, dirname, options = {})
     @dirname = dirname
     @options = options
     @puppetfile_name = options.delete(:puppetfile_name)
-    @overrides = options.delete(:overrides)
+    @overrides = options.delete(:overrides) || {}
 
     @full_path = File.join(@basedir, @dirname)
     @path = Pathname.new(File.join(@basedir, @dirname))
 
     @puppetfile  = R10K::Puppetfile.new(@full_path,
-                                        {overrides: @options[:overrides],
-                                         force: @options.dig(:overrides, :modules, :force),
+                                        {overrides: @overrides,
+                                         force: @overrides.dig(:modules, :force),
                                          puppetfile_name: @puppetfile_name})
     @puppetfile.environment = self
   end
diff --git a/lib/r10k/module/base.rb b/lib/r10k/module/base.rb
index 6dc0b1cf8..e2a5d9d99 100644
--- a/lib/r10k/module/base.rb
+++ b/lib/r10k/module/base.rb
@@ -51,7 +51,11 @@ def initialize(title, dirname, args, environment=nil)
     @owner, @name = parse_title(@title)
     @path = Pathname.new(File.join(@dirname, @name))
     @environment = environment
+    @overrides = args.delete(:overrides) || {}
     @origin = 'external' # Expect Puppetfile or R10k::Environment to set this to a specific value
+
+    @requested_modules = @overrides.dig(:modules, :requested_modules) || []
+    @to_be_synced = (@requested_modules.empty? || @requested_modules.include?(@name))
   end
 
   # @deprecated
@@ -63,9 +67,20 @@ def full_path
   # Synchronize this module with the indicated state.
   # @param [Hash] opts Deprecated
   def sync(opts={})
-    logger.info _("Checking module %{path}") % {path: path}
+    raise NotImplementedError
   end
 
+  def will_sync?
+    if @to_be_synced
+      logger.info _("Checking module %{path}") % {path: path}
+      true
+    else
+      logger.debug1(_("Only updating modules %{modules}, skipping module %{name}") % {modules: @requested_modules.inspect, name: name})
+      false
+    end
+  end
+
+
   # Return the desired version of this module
   # @abstract
   def version
diff --git a/lib/r10k/module/forge.rb b/lib/r10k/module/forge.rb
index 9d8e53f86..42769beb6 100644
--- a/lib/r10k/module/forge.rb
+++ b/lib/r10k/module/forge.rb
@@ -47,10 +47,9 @@ def initialize(title, dirname, opts, environment=nil)
 
     setopts(opts, {
       # Standard option interface
-      :version   => :expected_version,
-      :source    => ::R10K::Util::Setopts::Ignore,
-      :type      => ::R10K::Util::Setopts::Ignore,
-      :overrides => :self,
+      :version => :expected_version,
+      :source  => ::R10K::Util::Setopts::Ignore,
+      :type    => ::R10K::Util::Setopts::Ignore,
     })
 
     @expected_version ||= current_version || :latest
@@ -60,15 +59,15 @@ def initialize(title, dirname, opts, environment=nil)
 
   # @param [Hash] opts Deprecated
   def sync(opts={})
-    super
-
-    case status
-    when :absent
-      install
-    when :outdated
-      upgrade
-    when :mismatched
-      reinstall
+    if will_sync?
+      case status
+      when :absent
+        install
+      when :outdated
+        upgrade
+      when :mismatched
+        reinstall
+      end
     end
   end
 
diff --git a/lib/r10k/module/git.rb b/lib/r10k/module/git.rb
index 9623ed95d..da69beb3e 100644
--- a/lib/r10k/module/git.rb
+++ b/lib/r10k/module/git.rb
@@ -36,25 +36,25 @@ def self.implement?(name, args)
   include R10K::Util::Setopts
 
   def initialize(title, dirname, opts, environment=nil)
+
     super
     setopts(opts, {
       # Standard option interface
-      :version   => :desired_ref,
-      :source    => :remote,
-      :type      => ::R10K::Util::Setopts::Ignore,
-      :overrides => :self,
+      :version                 => :desired_ref,
+      :source                  => :remote,
+      :type                    => ::R10K::Util::Setopts::Ignore,
 
       # Type-specific options
-      :branch    => :desired_ref,
-      :tag       => :desired_ref,
-      :commit    => :desired_ref,
-      :ref       => :desired_ref,
-      :git       => :remote,
+      :branch                  => :desired_ref,
+      :tag                     => :desired_ref,
+      :commit                  => :desired_ref,
+      :ref                     => :desired_ref,
+      :git                     => :remote,
       :default_branch          => :default_ref,
       :default_branch_override => :default_override_ref,
     })
 
-    force = @overrides && @overrides.dig(:modules, :force)
+    force = @overrides.dig(:modules, :force)
     @force = force == false ? false : true
 
     @desired_ref ||= 'master'
@@ -84,10 +84,8 @@ def properties
 
   # @param [Hash] opts Deprecated
   def sync(opts={})
-    super
-
     force = opts[:force] || @force
-    @repo.sync(version, force)
+    @repo.sync(version, force) if will_sync?
   end
 
   def status
diff --git a/lib/r10k/module/svn.rb b/lib/r10k/module/svn.rb
index f245bc0ed..d8aafb8b4 100644
--- a/lib/r10k/module/svn.rb
+++ b/lib/r10k/module/svn.rb
@@ -40,17 +40,16 @@ def initialize(name, dirname, opts, environment=nil)
     super
     setopts(opts, {
       # Standard option interface
-      :source    => :url,
-      :version   => :expected_revision,
-      :type      => ::R10K::Util::Setopts::Ignore,
-      :overrides => :self,
+      :source   => :url,
+      :version  => :expected_revision,
+      :type     => ::R10K::Util::Setopts::Ignore,
 
       # Type-specific options
-      :svn       => :url,
-      :rev       => :expected_revision,
-      :revision  => :expected_revision,
-      :username  => :self,
-      :password  => :self
+      :svn      => :url,
+      :rev      => :expected_revision,
+      :revision => :expected_revision,
+      :username => :self,
+      :password => :self
     })
 
     @working_dir = R10K::SVN::WorkingDir.new(@path, :username => @username, :password => @password)
@@ -72,15 +71,15 @@ def status
 
   # @param [Hash] opts Deprecated
   def sync(opts={})
-    super
-
-    case status
-    when :absent
-      install
-    when :mismatched
-      reinstall
-    when :outdated
-      update
+    if will_sync?
+      case status
+      when :absent
+        install
+      when :mismatched
+        reinstall
+      when :outdated
+        update
+      end
     end
   end
 
diff --git a/spec/unit/action/deploy/module_spec.rb b/spec/unit/action/deploy/module_spec.rb
index 2c7b722e6..d043a7751 100644
--- a/spec/unit/action/deploy/module_spec.rb
+++ b/spec/unit/action/deploy/module_spec.rb
@@ -84,7 +84,7 @@
       before do
         allow(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
           expect(environment.puppetfile).to receive(:modules).and_return(
-            [R10K::Module::Local.new(environment.name, '/fakedir', [], environment)]
+            [R10K::Module::Local.new(environment.name, '/fakedir', {}, environment)]
           )
           original.call(environment, &block)
         end
@@ -177,4 +177,57 @@
       expect(subject.instance_variable_get(:@oauth_token)).to eq('/nonexistent')
     end
   end
+
+  describe 'with modules' do
+
+    subject { described_class.new({ config: '/some/nonexistent/path' }, ['mod1', 'mod2'], {}) }
+
+    let(:cache) { instance_double("R10K::Git::Cache", 'sanitized_dirname' => 'foo', 'cached?' => true, 'sync' => true) }
+    let(:repo) { instance_double("R10K::Git::StatefulRepository", cache: cache, resolve: 'main') }
+
+    it 'does not sync modules not given' do
+      allow(R10K::Deployment).to receive(:new).and_wrap_original do |original, settings, &block|
+        original.call(settings.merge({
+          sources: {
+            main: {
+              remote: 'git://not/a/remote',
+              basedir: '/not/a/basedir',
+              type: 'git'
+            }
+          }
+        }))
+      end
+
+      allow(R10K::Git::StatefulRepository).to receive(:new).and_return(repo)
+      allow(R10K::Git).to receive_message_chain(:cache, :generate).and_return(cache)
+      allow_any_instance_of(R10K::Source::Git).to receive(:branch_names).and_return([R10K::Environment::Name.new('first', {})])
+
+      expect(subject).to receive(:visit_puppetfile).and_wrap_original do |original, puppetfile, &block|
+        expect(puppetfile).to receive(:load) do
+          puppetfile.add_module('mod1', { git: 'git://remote' })
+          puppetfile.add_module('mod2', { git: 'git://remote' })
+          puppetfile.add_module('mod3', { git: 'git://remote' })
+        end
+
+        original.call(puppetfile, &block)
+      end
+
+      expect(subject).to receive(:visit_module).and_wrap_original do |original, mod, &block|
+        if ['mod1', 'mod2'].include?(mod.name)
+          expect(mod.will_sync?).to be(true)
+        else
+          expect(mod.will_sync?).to be(false)
+        end
+
+        expect(mod).to receive(:sync).and_call_original
+
+        original.call(mod)
+      end.exactly(3).times
+
+      expect(repo).to receive(:sync).twice
+
+      subject.call
+    end
+  end
 end
+
diff --git a/spec/unit/action/puppetfile/install_spec.rb b/spec/unit/action/puppetfile/install_spec.rb
index 953ee649a..081751c0b 100644
--- a/spec/unit/action/puppetfile/install_spec.rb
+++ b/spec/unit/action/puppetfile/install_spec.rb
@@ -26,7 +26,7 @@ def installer(opts = {}, argv = [], settings = {})
   describe "installing modules" do
     let(:modules) do
       (1..4).map do |idx|
-        R10K::Module::Base.new("author/modname#{idx}", "/some/nonexistent/path/modname#{idx}", nil)
+        R10K::Module::Base.new("author/modname#{idx}", "/some/nonexistent/path/modname#{idx}", {})
       end
     end
 

From 654f5c2911773b89afdf37c7d02c001003dafd08 Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Fri, 7 May 2021 08:25:57 -0700
Subject: [PATCH 06/11] (maint) Move type generation for `depoy module` to
 #visit_environment

For every module deployed via `deploy module`, if the `generate-types`
flag was given, we would attempt to generate types for the environments
containing that module. This could lead to type generation happening
multiple times for each environment, though in practice users typically
use this action to update a single module at a time.

This moves the logic regarding type generation from the visit_module
method on the deploy modules action to the visit_environment method
where it makes more sense to do environment wide updates.
---
 lib/r10k/action/deploy/module.rb       | 15 +++++++++------
 spec/unit/action/deploy/module_spec.rb |  2 +-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/lib/r10k/action/deploy/module.rb b/lib/r10k/action/deploy/module.rb
index 0cedc96ac..2cfa65823 100644
--- a/lib/r10k/action/deploy/module.rb
+++ b/lib/r10k/action/deploy/module.rb
@@ -67,8 +67,17 @@ def visit_environment(environment)
             logger.debug1(_("Only updating modules in environment(s) %{opt_env} skipping environment %{env_path}") % {opt_env: requested_envs.inspect, env_path: environment.path})
           else
             logger.debug1(_("Updating modules %{modules} in environment %{env_path}") % {modules: @settings.dig(:overrides, :modules, :requested_modules).inspect, env_path: environment.path})
+
             yield
+
+            requested_mods = @settings.dig(:overrides, :modules, :requested_modules) || []
+            generate_types = @settings.dig(:overrides, :environments, :generate_types)
+            if generate_types && !((environment.modules.map(&:name) & requested_mods).empty?)
+              logger.debug("Generating puppet types for environment '#{environment.dirname}'...")
+              environment.generate_types!
+            end
           end
+
         end
 
         def visit_puppetfile(puppetfile)
@@ -78,12 +87,6 @@ def visit_puppetfile(puppetfile)
 
         def visit_module(mod)
           mod.sync
-
-          requested_mods = @settings.dig(:overrides, :modules, :requested_modules) || []
-          if requested_mods.include?(mod.name) && mod.environment && @settings.dig(:overrides, :environments, :generate_types)
-            logger.debug("Generating puppet types for environment '#{mod.environment.dirname}'...")
-            mod.environment.generate_types!
-          end
         end
 
         def allowed_initialize_opts
diff --git a/spec/unit/action/deploy/module_spec.rb b/spec/unit/action/deploy/module_spec.rb
index d043a7751..2f0ac503a 100644
--- a/spec/unit/action/deploy/module_spec.rb
+++ b/spec/unit/action/deploy/module_spec.rb
@@ -85,7 +85,7 @@
         allow(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
           expect(environment.puppetfile).to receive(:modules).and_return(
             [R10K::Module::Local.new(environment.name, '/fakedir', {}, environment)]
-          )
+          ).twice
           original.call(environment, &block)
         end
       end

From f575902a5ba2d2ec48dbe9f6fda36ef84f080d0f Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Fri, 7 May 2021 09:16:03 -0700
Subject: [PATCH 07/11] (maint) Remove visit_module usage

Previously, the module.sync method would be called within a visit_module
method on each action controlling the sync.

This removes the visit_module methods from actions and has the locations
where `module.accept(visitor)` as called before call `module.sync`
instead.

The visitor interface would wrap every visit_* method in a rescue block
and set the `visit_ok` instance variable to false if there was an issue.
To keep this general behavior for each action we've add this behavior to
the `call` method of the action.
---
 lib/r10k/action/deploy/environment.rb  | 18 ++++++-----
 lib/r10k/action/deploy/module.rb       | 20 ++++++------
 lib/r10k/action/puppetfile/install.rb  | 20 ++++++------
 lib/r10k/content_synchronizer.rb       |  8 ++---
 lib/r10k/environment/with_modules.rb   |  2 +-
 lib/r10k/module/base.rb                |  1 +
 spec/unit/action/deploy/module_spec.rb | 42 ++++++++++++--------------
 spec/unit/puppetfile_spec.rb           | 10 +++---
 8 files changed, 61 insertions(+), 60 deletions(-)

diff --git a/lib/r10k/action/deploy/environment.rb b/lib/r10k/action/deploy/environment.rb
index e1e26eaf2..fb2592a5a 100644
--- a/lib/r10k/action/deploy/environment.rb
+++ b/lib/r10k/action/deploy/environment.rb
@@ -51,11 +51,17 @@ def initialize(opts, argv, settings)
         def call
           @visit_ok = true
 
-          expect_config!
-          deployment = R10K::Deployment.new(@settings)
-          check_write_lock!(@settings)
+          begin
+            expect_config!
+            deployment = R10K::Deployment.new(@settings)
+            check_write_lock!(@settings)
+
+            deployment.accept(self)
+          rescue => e
+            @visit_ok = false
+            logger.error R10K::Errors::Formatting.format_exception(e, @trace)
+          end
 
-          deployment.accept(self)
           @visit_ok
         end
 
@@ -182,10 +188,6 @@ def visit_puppetfile(puppetfile)
           end
         end
 
-        def visit_module(mod)
-          mod.sync
-        end
-
         def write_environment_info!(environment, started_at, success)
           module_deploys =
             begin
diff --git a/lib/r10k/action/deploy/module.rb b/lib/r10k/action/deploy/module.rb
index 2cfa65823..6fca95d7b 100644
--- a/lib/r10k/action/deploy/module.rb
+++ b/lib/r10k/action/deploy/module.rb
@@ -40,12 +40,17 @@ def initialize(opts, argv, settings)
 
         def call
           @visit_ok = true
+          begin
+            expect_config!
+            deployment = R10K::Deployment.new(@settings)
+            check_write_lock!(@settings)
+
+            deployment.accept(self)
+          rescue => e
+            @visit_ok = false
+            logger.error R10K::Errors::Formatting.format_exception(e, @trace)
+          end
 
-          expect_config!
-          deployment = R10K::Deployment.new(@settings)
-          check_write_lock!(@settings)
-
-          deployment.accept(self)
           @visit_ok
         end
 
@@ -77,7 +82,6 @@ def visit_environment(environment)
               environment.generate_types!
             end
           end
-
         end
 
         def visit_puppetfile(puppetfile)
@@ -85,10 +89,6 @@ def visit_puppetfile(puppetfile)
           yield
         end
 
-        def visit_module(mod)
-          mod.sync
-        end
-
         def allowed_initialize_opts
           super.merge(environment: true,
                       cachedir: :self,
diff --git a/lib/r10k/action/puppetfile/install.rb b/lib/r10k/action/puppetfile/install.rb
index e25755258..159764cf4 100644
--- a/lib/r10k/action/puppetfile/install.rb
+++ b/lib/r10k/action/puppetfile/install.rb
@@ -11,11 +11,17 @@ class Install < R10K::Action::Base
 
         def call
           @visit_ok = true
-          pf = R10K::Puppetfile.new(@root,
-                                    {moduledir: @moduledir,
-                                     puppetfile_path: @puppetfile,
-                                     force: @force || false})
-          pf.accept(self)
+          begin
+            pf = R10K::Puppetfile.new(@root,
+                                      {moduledir: @moduledir,
+                                       puppetfile_path: @puppetfile,
+                                       force: @force || false})
+            pf.accept(self)
+          rescue => e
+            @visit_ok = false
+            logger.error R10K::Errors::Formatting.format_exception(e, @trace)
+          end
+
           @visit_ok
         end
 
@@ -33,10 +39,6 @@ def visit_puppetfile(pf)
                                   pf.purge_exclusions).purge!
         end
 
-        def visit_module(mod)
-          mod.sync
-        end
-
         def allowed_initialize_opts
           super.merge(root: :self, puppetfile: :self, moduledir: :self, force: :self )
         end
diff --git a/lib/r10k/content_synchronizer.rb b/lib/r10k/content_synchronizer.rb
index 40942df93..d313f5fee 100644
--- a/lib/r10k/content_synchronizer.rb
+++ b/lib/r10k/content_synchronizer.rb
@@ -4,7 +4,7 @@ module ContentSynchronizer
     def self.serial_accept(modules, visitor, loader)
       visitor.visit(:puppetfile, loader) do
         modules.each do |mod|
-          mod.accept(visitor)
+          mod.sync
         end
       end
     end
@@ -12,7 +12,7 @@ def self.serial_accept(modules, visitor, loader)
     def self.concurrent_accept(modules, visitor, loader, pool_size, logger)
       logger.debug _("Updating modules with %{pool_size} threads") % {pool_size: pool_size}
       mods_queue = modules_queue(modules, visitor, loader)
-      thread_pool = pool_size.times.map { visitor_thread(visitor, mods_queue, logger) }
+      thread_pool = pool_size.times.map { sync_thread(mods_queue, logger) }
       thread_exception = nil
 
       # If any threads raise an exception the deployment is considered a failure.
@@ -42,11 +42,11 @@ def self.modules_queue(modules, visitor, loader)
       end
     end
 
-    def self.visitor_thread(visitor, mods_queue, logger)
+    def self.sync_thread(mods_queue, logger)
       Thread.new do
         begin
           while mods = mods_queue.pop(true) do
-            mods.each {|mod| mod.accept(visitor) }
+            mods.each { |mod| mod.sync }
           end
         rescue ThreadError => e
           logger.debug _("Module thread %{id} exiting: %{message}") % {message: e.message, id: Thread.current.object_id}
diff --git a/lib/r10k/environment/with_modules.rb b/lib/r10k/environment/with_modules.rb
index 3a55c4136..f3a4035f8 100644
--- a/lib/r10k/environment/with_modules.rb
+++ b/lib/r10k/environment/with_modules.rb
@@ -78,7 +78,7 @@ def module_conflicts?(mod_b)
   def accept(visitor)
     visitor.visit(:environment, self) do
       @modules.each do |mod|
-        mod.accept(visitor)
+        mod.sync
       end
 
       puppetfile.accept(visitor)
diff --git a/lib/r10k/module/base.rb b/lib/r10k/module/base.rb
index e2a5d9d99..ea7924d14 100644
--- a/lib/r10k/module/base.rb
+++ b/lib/r10k/module/base.rb
@@ -102,6 +102,7 @@ def status
     raise NotImplementedError
   end
 
+  # Deprecated
   def accept(visitor)
     visitor.visit(:module, self)
   end
diff --git a/spec/unit/action/deploy/module_spec.rb b/spec/unit/action/deploy/module_spec.rb
index 2f0ac503a..b8fda1b42 100644
--- a/spec/unit/action/deploy/module_spec.rb
+++ b/spec/unit/action/deploy/module_spec.rb
@@ -82,10 +82,16 @@
       end
 
       before do
+        @modules = []
         allow(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
-          expect(environment.puppetfile).to receive(:modules).and_return(
-            [R10K::Module::Local.new(environment.name, '/fakedir', {}, environment)]
-          ).twice
+          mod = R10K::Module::Local.new(environment.name, '/fakedir', {}, environment)
+          if mod.name == 'first'
+            expect(environment).to receive(:generate_types!)
+          else
+            expect(environment).not_to receive(:generate_types!)
+          end
+          @modules << mod
+          expect(environment.puppetfile).to receive(:modules).and_return([mod]).twice
           original.call(environment, &block)
         end
       end
@@ -95,15 +101,8 @@
       end
 
       it 'only calls puppet generate types on environments with specified module' do
-        expect(subject).to receive(:visit_module).and_wrap_original do |original, mod, &block|
-          if mod.name == 'first'
-            expect(mod.environment).to receive(:generate_types!)
-          else
-            expect(mod.environment).not_to receive(:generate_types!)
-          end
-          original.call(mod, &block)
-        end.twice
         subject.call
+        expect(@modules.length).to be(2)
       end
     end
 
@@ -208,21 +207,18 @@
           puppetfile.add_module('mod2', { git: 'git://remote' })
           puppetfile.add_module('mod3', { git: 'git://remote' })
         end
+        puppetfile.modules.each do |mod|
+          if ['mod1', 'mod2'].include?(mod.name)
+            expect(mod.will_sync?).to be(true)
+          else
+            expect(mod.will_sync?).to be(false)
+          end
 
-        original.call(puppetfile, &block)
-      end
-
-      expect(subject).to receive(:visit_module).and_wrap_original do |original, mod, &block|
-        if ['mod1', 'mod2'].include?(mod.name)
-          expect(mod.will_sync?).to be(true)
-        else
-          expect(mod.will_sync?).to be(false)
+          expect(mod).to receive(:sync).and_call_original
         end
 
-        expect(mod).to receive(:sync).and_call_original
-
-        original.call(mod)
-      end.exactly(3).times
+        original.call(puppetfile, &block)
+      end
 
       expect(repo).to receive(:sync).twice
 
diff --git a/spec/unit/puppetfile_spec.rb b/spec/unit/puppetfile_spec.rb
index 8147c0a7d..ede28077e 100644
--- a/spec/unit/puppetfile_spec.rb
+++ b/spec/unit/puppetfile_spec.rb
@@ -301,7 +301,7 @@ def expect_wrapped_error(orig, pf_path, wrapped_error)
       subject.accept(visitor)
     end
 
-    it "passes the visitor to each module if the visitor yields" do
+    it "synchronizes each module if the visitor yields" do
       visitor = spy('visitor')
       expect(visitor).to receive(:visit) do |type, other, &block|
         expect(type).to eq :puppetfile
@@ -311,8 +311,8 @@ def expect_wrapped_error(orig, pf_path, wrapped_error)
 
       mod1 = instance_double('R10K::Module::Base', :cachedir => :none)
       mod2 = instance_double('R10K::Module::Base', :cachedir => :none)
-      expect(mod1).to receive(:accept).with(visitor)
-      expect(mod2).to receive(:accept).with(visitor)
+      expect(mod1).to receive(:sync)
+      expect(mod2).to receive(:sync)
       expect(subject).to receive(:modules).and_return([mod1, mod2])
 
       subject.accept(visitor)
@@ -332,8 +332,8 @@ def expect_wrapped_error(orig, pf_path, wrapped_error)
 
       mod1 = instance_double('R10K::Module::Base', :cachedir => :none)
       mod2 = instance_double('R10K::Module::Base', :cachedir => :none)
-      expect(mod1).to receive(:accept).with(visitor)
-      expect(mod2).to receive(:accept).with(visitor)
+      expect(mod1).to receive(:sync)
+      expect(mod2).to receive(:sync)
       expect(subject).to receive(:modules).and_return([mod1, mod2])
 
       expect(Thread).to receive(:new).exactly(pool_size).and_call_original

From c32abfdaf1380519d44154ff05613ea2467f08dd Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Fri, 7 May 2021 15:51:27 -0700
Subject: [PATCH 08/11] (maint) Document Action#initialize parameters

---
 lib/r10k/action/base.rb               | 7 +++++++
 lib/r10k/action/deploy/display.rb     | 7 +++++++
 lib/r10k/action/deploy/environment.rb | 7 +++++++
 lib/r10k/action/deploy/module.rb      | 8 +++++++-
 4 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/lib/r10k/action/base.rb b/lib/r10k/action/base.rb
index 056215b23..13742a76b 100644
--- a/lib/r10k/action/base.rb
+++ b/lib/r10k/action/base.rb
@@ -10,6 +10,13 @@ class Base
 
       attr_accessor :settings
 
+      # @param opts [Hash] A hash of options defined in #allowed_initialized_opts
+      #   and managed by the SetOps mixin within the Action::Base class.
+      #   Corresponds to the CLI flags and options.
+      # @param argv [CRI::ArgumentList] A list-like collection of the remaining
+      #   arguments to the CLI invocation (after removing flags and options).
+      # @param settings [Hash] A hash of configuration loaded from the relevant
+      #   config (r10k.yaml).
       def initialize(opts, argv, settings)
         @opts = opts
         @argv = argv
diff --git a/lib/r10k/action/deploy/display.rb b/lib/r10k/action/deploy/display.rb
index 0991bbdcb..2a627abcc 100644
--- a/lib/r10k/action/deploy/display.rb
+++ b/lib/r10k/action/deploy/display.rb
@@ -9,6 +9,13 @@ class Display < R10K::Action::Base
 
         include R10K::Action::Deploy::DeployHelpers
 
+        # @param opts [Hash] A hash of options defined in #allowed_initialized_opts
+        #   and managed by the SetOps mixin within the Action::Base class.
+        #   Corresponds to the CLI flags and options.
+        # @param argv [CRI::ArgumentList] A list-like collection of the remaining
+        #   arguments to the CLI invocation (after removing flags and options).
+        # @param settings [Hash] A hash of configuration loaded from the relevant
+        #   config (r10k.yaml).
         def initialize(opts, argv, settings)
           super
 
diff --git a/lib/r10k/action/deploy/environment.rb b/lib/r10k/action/deploy/environment.rb
index fb2592a5a..c91c7621a 100644
--- a/lib/r10k/action/deploy/environment.rb
+++ b/lib/r10k/action/deploy/environment.rb
@@ -19,6 +19,13 @@ class Environment < R10K::Action::Base
 
         attr_reader :settings
 
+        # @param opts [Hash] A hash of options defined in #allowed_initialized_opts
+        #   and managed by the SetOps mixin within the Action::Base class.
+        #   Corresponds to the CLI flags and options.
+        # @param argv [CRI::ArgumentList] A list-like collection of the remaining
+        #   arguments to the CLI invocation (after removing flags and options).
+        # @param settings [Hash] A hash of configuration loaded from the relevant
+        #   config (r10k.yaml).
         def initialize(opts, argv, settings)
           super
 
diff --git a/lib/r10k/action/deploy/module.rb b/lib/r10k/action/deploy/module.rb
index 6fca95d7b..41db0d7d2 100644
--- a/lib/r10k/action/deploy/module.rb
+++ b/lib/r10k/action/deploy/module.rb
@@ -15,8 +15,14 @@ class Module < R10K::Action::Base
 
         attr_reader :settings
 
+        # @param opts [Hash] A hash of options defined in #allowed_initialized_opts
+        #   and managed by the SetOps mixin within the Action::Base class.
+        #   Corresponds to the CLI flags and options.
+        # @param argv [CRI::ArgumentList] A list-like collection of the remaining
+        #   arguments to the CLI invocation (after removing flags and options).
+        # @param settings [Hash] A hash of configuration loaded from the relevant
+        #   config (r10k.yaml).
         def initialize(opts, argv, settings)
-
           super
 
           requested_env = @opts[:environment] ? [@opts[:environment].gsub(/\W/, '_')] : []

From cae6022c6a71b0995389f2718f0872199d8c7172 Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Mon, 10 May 2021 11:53:33 -0700
Subject: [PATCH 09/11] (maint) will_sync -> should_sync

---
 lib/r10k/module/base.rb                | 6 +++---
 lib/r10k/module/forge.rb               | 2 +-
 lib/r10k/module/git.rb                 | 2 +-
 lib/r10k/module/svn.rb                 | 2 +-
 spec/unit/action/deploy/module_spec.rb | 4 ++--
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/r10k/module/base.rb b/lib/r10k/module/base.rb
index ea7924d14..92c12c359 100644
--- a/lib/r10k/module/base.rb
+++ b/lib/r10k/module/base.rb
@@ -55,7 +55,7 @@ def initialize(title, dirname, args, environment=nil)
     @origin = 'external' # Expect Puppetfile or R10k::Environment to set this to a specific value
 
     @requested_modules = @overrides.dig(:modules, :requested_modules) || []
-    @to_be_synced = (@requested_modules.empty? || @requested_modules.include?(@name))
+    @should_sync = (@requested_modules.empty? || @requested_modules.include?(@name))
   end
 
   # @deprecated
@@ -70,8 +70,8 @@ def sync(opts={})
     raise NotImplementedError
   end
 
-  def will_sync?
-    if @to_be_synced
+  def should_sync?
+    if @should_sync
       logger.info _("Checking module %{path}") % {path: path}
       true
     else
diff --git a/lib/r10k/module/forge.rb b/lib/r10k/module/forge.rb
index 42769beb6..19b2faa5c 100644
--- a/lib/r10k/module/forge.rb
+++ b/lib/r10k/module/forge.rb
@@ -59,7 +59,7 @@ def initialize(title, dirname, opts, environment=nil)
 
   # @param [Hash] opts Deprecated
   def sync(opts={})
-    if will_sync?
+    if should_sync?
       case status
       when :absent
         install
diff --git a/lib/r10k/module/git.rb b/lib/r10k/module/git.rb
index da69beb3e..d580aef3c 100644
--- a/lib/r10k/module/git.rb
+++ b/lib/r10k/module/git.rb
@@ -85,7 +85,7 @@ def properties
   # @param [Hash] opts Deprecated
   def sync(opts={})
     force = opts[:force] || @force
-    @repo.sync(version, force) if will_sync?
+    @repo.sync(version, force) if should_sync?
   end
 
   def status
diff --git a/lib/r10k/module/svn.rb b/lib/r10k/module/svn.rb
index d8aafb8b4..16ab316ba 100644
--- a/lib/r10k/module/svn.rb
+++ b/lib/r10k/module/svn.rb
@@ -71,7 +71,7 @@ def status
 
   # @param [Hash] opts Deprecated
   def sync(opts={})
-    if will_sync?
+    if should_sync?
       case status
       when :absent
         install
diff --git a/spec/unit/action/deploy/module_spec.rb b/spec/unit/action/deploy/module_spec.rb
index b8fda1b42..1bc58075e 100644
--- a/spec/unit/action/deploy/module_spec.rb
+++ b/spec/unit/action/deploy/module_spec.rb
@@ -209,9 +209,9 @@
         end
         puppetfile.modules.each do |mod|
           if ['mod1', 'mod2'].include?(mod.name)
-            expect(mod.will_sync?).to be(true)
+            expect(mod.should_sync?).to be(true)
           else
-            expect(mod.will_sync?).to be(false)
+            expect(mod.should_sync?).to be(false)
           end
 
           expect(mod).to receive(:sync).and_call_original

From 609ea04854ca5282f55e88927c75a2c724ec0d1f Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Mon, 10 May 2021 11:58:23 -0700
Subject: [PATCH 10/11] (maint) Update Environment::WithModules#add_modules
 docs

---
 lib/r10k/environment/with_modules.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/r10k/environment/with_modules.rb b/lib/r10k/environment/with_modules.rb
index f3a4035f8..ad7a972d2 100644
--- a/lib/r10k/environment/with_modules.rb
+++ b/lib/r10k/environment/with_modules.rb
@@ -96,7 +96,7 @@ def load_modules(module_hash)
   end
 
   # @param [String] name
-  # @param [*Object] args
+  # @param [Hash] args
   def add_module(name, args)
     # symbolize keys in the args hash
     args = args.inject({}) { |memo,(k,v)| memo[k.to_sym] = v; memo }

From dbc12429aa0272fbe672b87194fcbad580a2a057 Mon Sep 17 00:00:00 2001
From: Justin Stoller <justin.stoller@gmail.com>
Date: Mon, 10 May 2021 14:24:40 -0700
Subject: [PATCH 11/11] (maint) Use "Deploying" in module sync logging

---
 lib/r10k/module/base.rb        | 2 +-
 spec/unit/module/forge_spec.rb | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/r10k/module/base.rb b/lib/r10k/module/base.rb
index 92c12c359..61e99e859 100644
--- a/lib/r10k/module/base.rb
+++ b/lib/r10k/module/base.rb
@@ -72,7 +72,7 @@ def sync(opts={})
 
   def should_sync?
     if @should_sync
-      logger.info _("Checking module %{path}") % {path: path}
+      logger.info _("Deploying module to %{path}") % {path: path}
       true
     else
       logger.debug1(_("Only updating modules %{modules}, skipping module %{name}") % {modules: @requested_modules.inspect, name: name})
diff --git a/spec/unit/module/forge_spec.rb b/spec/unit/module/forge_spec.rb
index a98abd465..b82964beb 100644
--- a/spec/unit/module/forge_spec.rb
+++ b/spec/unit/module/forge_spec.rb
@@ -77,7 +77,7 @@
       logger_dbl = double(Log4r::Logger)
       allow_any_instance_of(described_class).to receive(:logger).and_return(logger_dbl)
 
-      allow(logger_dbl).to receive(:info).with(/Checking module.*/)
+      allow(logger_dbl).to receive(:info).with(/Deploying module to.*/)
       expect(logger_dbl).to receive(:warn).with(/puppet forge module.*puppetlabs-corosync.*has been deprecated/i)
 
       subject.sync
@@ -89,7 +89,7 @@
       logger_dbl = double(Log4r::Logger)
       allow_any_instance_of(described_class).to receive(:logger).and_return(logger_dbl)
 
-      allow(logger_dbl).to receive(:info).with(/Checking module.*/)
+      allow(logger_dbl).to receive(:info).with(/Deploying module to.*/)
       expect(logger_dbl).to_not receive(:warn).with(/puppet forge module.*puppetlabs-corosync.*has been deprecated/i)
 
       subject.sync