[#1215] Ensure module postrun command only runs for modified envs

Previously, when a postrun command with interpolated environments was
specified, the `deploy module` command with no environments specified
would cause the command to be run for all environments that contained
the module, whether or not the env was actually updated by the
deploy.

Similarly, `puppet generate types` would be run for any environment that
contained the module, not just those for which it was updated. Together,
these two things could result in a huge amount of unnecessary work.

This commit makes modules return the result of their syncing, true for
"updated" and false for "not updated". The environment's `deploy`
function collects this information, recording the names of each module
that was updated as part of the deploy. The `module deploy` action then
uses this to determine which environments actually had modules modified,
and uses that to populate the `$modifiedenvs` in the postrun command. So
now, the command will only be run for environments that actually updated
a module.

This also updates the logic for running `puppet generate types` to
follow the same pattern: it is only run for environments that actually
had an updated module.

Note this does not affect environment deploys.

lib/r10k/action/deploy/environment.rb

@@ -135,6 +135,7 @@ def visit_deployment(deployment)
               envs.reject! { |e| !requested_envs.include?(e) } if requested_envs.any?
               postcmd = postcmd.map { |e| e.gsub('$modifiedenvs', envs.join(' ')) }
             end
+            logger.debug _("Executing postrun command.")
             subproc = R10K::Util::Subprocess.new(postcmd)
             subproc.logger = logger
             subproc.execute

lib/r10k/action/deploy/module.rb

@@ -78,16 +78,18 @@ def visit_deployment(deployment)
         ensure
           if (postcmd = @settings[:postrun])
             if @modified_envs.any?
+              envs_to_run = @modified_envs.join(' ')
+              logger.debug _("Running postrun command for environments: %{envs_to_run}.") % { envs_to_run: envs_to_run }
+
               if postcmd.grep('$modifiedenvs').any?
-                envs_to_run = @modified_envs.join(' ')
-                logger.debug "Running postrun command for environments #{envs_to_run}."
                 postcmd = postcmd.map { |e| e.gsub('$modifiedenvs', envs_to_run) }
               end
+
               subproc = R10K::Util::Subprocess.new(postcmd)
               subproc.logger = logger
               subproc.execute
             else
-              logger.debug "No environments were modified, not executing postrun command."
+              logger.debug _("No environments were modified, not executing postrun command.")
             end
           end
         end
@@ -103,11 +105,10 @@ def visit_environment(environment)
           else
             logger.debug1(_("Updating modules %{modules} in environment %{env_path}") % {modules: @settings.dig(:overrides, :modules, :requested_modules).inspect, env_path: environment.path})
 
-            environment.deploy
+            updated_modules = environment.deploy
 
-            requested_mods = @settings.dig(:overrides, :modules, :requested_modules) || []
             # We actually synced a module in this env
-            if !((environment.modules.map(&:name) & requested_mods).empty?)
+            if !(updated_modules.nil? || updated_modules.empty?)
               # Record modified environment for postrun command
               @modified_envs << environment.dirname
 

lib/r10k/content_synchronizer.rb

@@ -8,31 +8,40 @@ def self.serial_accept(modules, visitor, loader)
     end
 
     def self.serial_sync(modules)
+      updated_modules = []
       modules.each do |mod|
-        mod.sync
+        updated = mod.sync
+        updated_modules << mod.name if updated
       end
+      updated_modules
     end
 
+    # Returns a Queue of the names of modules actually updated
     def self.concurrent_accept(modules, visitor, loader, pool_size, logger)
       mods_queue = modules_visit_queue(modules, visitor, loader)
       sync_queue(mods_queue, pool_size, logger)
     end
 
+    # Returns a Queue of the names of modules actually updated
     def self.concurrent_sync(modules, pool_size, logger)
       mods_queue = modules_sync_queue(modules)
       sync_queue(mods_queue, pool_size, logger)
     end
 
+    # Returns a Queue of the names of modules actually updated
     def self.sync_queue(mods_queue, pool_size, logger)
       logger.debug _("Updating modules with %{pool_size} threads") % {pool_size: pool_size}
-      thread_pool = pool_size.times.map { sync_thread(mods_queue, logger) }
+      updated_modules = Queue.new
+      thread_pool = pool_size.times.map { sync_thread(mods_queue, logger, updated_modules) }
       thread_exception = nil
 
       # If any threads raise an exception the deployment is considered a failure.
       # In that event clear the queue, wait for other threads to finish their
       # current work, then re-raise the first exception caught.
       begin
         thread_pool.each(&:join)
+        # Return the list of all modules that were actually updated
+        updated_modules
       rescue => e
         logger.error _("Error during concurrent deploy of a module: %{message}") % {message: e.message}
         mods_queue.clear
@@ -65,11 +74,14 @@ def self.enqueue_modules(queue, modules)
       modules_by_cachedir.values.each {|mods| queue << mods }
     end
 
-    def self.sync_thread(mods_queue, logger)
+    def self.sync_thread(mods_queue, logger, updated_modules)
       Thread.new do
         begin
           while mods = mods_queue.pop(true) do
-            mods.each { |mod| mod.sync }
+            mods.each do |mod|
+              udpated = mod.sync
+              updated_modules << mod.name if udpated
+            end
           end
         rescue ThreadError => e
           logger.debug _("Module thread %{id} exiting: %{message}") % {message: e.message, id: Thread.current.object_id}

lib/r10k/environment/base.rb

@@ -147,20 +147,24 @@ def accept(visitor)
     end
   end
 
+
+  # Returns a Queue of the names of modules actually updated
   def deploy
     if @base_modules.nil?
       load_puppetfile_modules
     end
 
     if ! @base_modules.empty?
       pool_size = @overrides.dig(:modules, :pool_size)
-      R10K::ContentSynchronizer.concurrent_sync(@base_modules, pool_size, logger)
+      updated_modules = R10K::ContentSynchronizer.concurrent_sync(@base_modules, pool_size, logger)
     end
 
     if (@overrides.dig(:purging, :purge_levels) || []).include?(:puppetfile)
       logger.debug("Purging unmanaged Puppetfile content for environment '#{dirname}'...")
       @puppetfile_cleaner.purge!
     end
+
+    updated_modules
   end
 
   def load_puppetfile_modules

lib/r10k/git/stateful_repository.rb

@@ -35,6 +35,7 @@ def resolve(ref)
     @cache.resolve(ref)
   end
 
+  # Returns true if the sync actually updated the repo, false otherwise
   def sync(ref, force=true)
     @cache.sync if sync_cache?(ref)
 
@@ -46,6 +47,7 @@ def sync(ref, force=true)
 
     workdir_status = status(ref)
 
+    updated = true
     case workdir_status
     when :absent
       logger.debug(_("Cloning %{repo_path} and checking out %{ref}") % {repo_path: @repo.path, ref: ref })
@@ -67,7 +69,9 @@ def sync(ref, force=true)
       end
     else
       logger.debug(_("%{repo_path} is already at Git ref %{ref}") % {repo_path: @repo.path, ref: ref })
+      updated = false
     end
+    updated
   end
 
   def status(ref)

lib/r10k/module/base.rb

@@ -106,6 +106,7 @@ def delete_spec_dir
 
   # Synchronize this module with the indicated state.
   # @param [Hash] opts Deprecated
+  # @return [Boolean] true if the module was updated, false otherwise
   def sync(opts={})
     raise NotImplementedError
   end

lib/r10k/module/definition.rb

@@ -23,8 +23,10 @@ def to_implementation
   end
 
   # syncing is a noop for module definitions
+  # Returns false to inidicate the module was not updated
   def sync(args = {})
     logger.debug1(_("Not updating module %{name}, assuming content unchanged") % {name: name})
+    false
   end
 
   def status

lib/r10k/module/forge.rb

@@ -60,18 +60,24 @@ def initialize(title, dirname, opts, environment=nil)
   end
 
   # @param [Hash] opts Deprecated
+  # @return [Boolean] true if the module was updated, false otherwise
   def sync(opts={})
+    updated = false
     if should_sync?
       case status
       when :absent
+        updated = true
         install
       when :outdated
+        updated = true
         upgrade
       when :mismatched
+        updated = true
         reinstall
       end
       maybe_delete_spec_dir
     end
+    updated
   end
 
   def properties

lib/r10k/module/git.rb

@@ -98,10 +98,16 @@ def properties
   end
 
   # @param [Hash] opts Deprecated
+  # @return [Boolean] true if the module was updated, false otherwise
   def sync(opts={})
     force = opts[:force] || @force
-    @repo.sync(version, force) if should_sync?
+    if should_sync?
+      updated = @repo.sync(version, force)
+    else
+      updated = false
+    end
     maybe_delete_spec_dir
+    updated
   end
 
   def status

lib/r10k/module/local.rb

@@ -28,7 +28,9 @@ def status
   end
 
   # @param [Hash] opts Deprecated
+  # @return [Boolean] false, because local modules are always considered in-sync
   def sync(opts={})
     logger.debug1 _("Module %{title} is a local module, always indicating synced.") % {title: title}
+    false
   end
 end

lib/r10k/module/svn.rb

@@ -75,17 +75,22 @@ def status
 
   # @param [Hash] opts Deprecated
   def sync(opts={})
+    updated = false
     if should_sync?
       case status
       when :absent
+        updated = true
         install
       when :mismatched
+        updated = true
         reinstall
       when :outdated
+        updated = true
         update
       end
       maybe_delete_spec_dir
     end
+    updated
   end
 
   def exist?

spec/unit/action/deploy/module_spec.rb

@@ -103,21 +103,14 @@
       end
 
       before do
-        @modules = []
         allow(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
-          mod = R10K::Module::Local.new(environment.name, '/fakedir', {}, environment)
-          if mod.name == 'first'
+          if environment.name == 'first'
+            expect(environment).to receive(:deploy).and_return(['first'])
             expect(environment).to receive(:generate_types!)
           else
+            expect(environment).to receive(:deploy).and_return([])
             expect(environment).not_to receive(:generate_types!)
           end
-          @modules << mod
-          allow(environment.loader).to receive(:load).and_return({
-            modules: [mod],
-            desired_contents: [],
-            managed_directories: [],
-            purge_exclusions: []
-          }).once
           original.call(environment, &block)
         end
       end
@@ -126,9 +119,8 @@
         expect(subject.settings[:overrides][:environments][:generate_types]).to eq(true)
       end
 
-      it 'only calls puppet generate types on environments with specified module' do
+      it 'only calls puppet generate types on environments where the specified module was updated' do
         subject.call
-        expect(@modules.length).to be(2)
       end
     end
 
@@ -422,16 +414,13 @@
           allow(mock_subprocess).to receive(:logger=)
           expect(mock_subprocess).to receive(:execute)
 
-          mock_mod = double('mock_mod', name: 'mod1')
-
           expect(R10K::Util::Subprocess).to receive(:new).
             with(["/generate/types/wrapper", "first"]).
             and_return(mock_subprocess)
 
           expect(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
             if environment.name == 'first'
-              expect(environment).to receive(:deploy).and_return(true)
-              expect(environment).to receive(:modules).and_return([mock_mod])
+              expect(environment).to receive(:deploy).and_return(['first'])
             end
             original.call(environment, &block)
           end.exactly(3).times
@@ -462,12 +451,9 @@
             with(["/generate/types/wrapper", "first third"]).
             and_return(mock_subprocess)
 
-          mock_mod = double('mock_mod', name: 'mod1')
-
           expect(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
-            expect(environment).to receive(:deploy).and_return(true)
             if ['first', 'third'].include?(environment.name)
-              expect(environment).to receive(:modules).and_return([mock_mod])
+              expect(environment).to receive(:deploy).and_return(['mod1'])
             end
             original.call(environment, &block)
           end.exactly(3).times
@@ -480,9 +466,7 @@
 
           mock_mod2 = double('mock_mod', name: 'mod2')
           expect(subject).to receive(:visit_environment).and_wrap_original do |original, environment, &block|
-            expect(environment).to receive(:deploy).and_return(true)
-            # Envs have a different module than the one we asked to deploy
-            expect(environment).to receive(:modules).and_return([mock_mod2])
+            expect(environment).to receive(:deploy).and_return([])
             original.call(environment, &block)
           end.exactly(3).times
 

spec/unit/module/forge_spec.rb

@@ -212,25 +212,31 @@
       expect(subject).to receive(:install).never
       expect(subject).to receive(:upgrade).never
       expect(subject).to receive(:reinstall).never
-      subject.sync
+      expect(subject.sync).to be false
     end
 
     it 'reinstalls the module when it is mismatched' do
       allow(subject).to receive(:status).and_return :mismatched
       expect(subject).to receive(:reinstall)
-      subject.sync
+      expect(subject.sync).to be true
     end
 
     it 'upgrades the module when it is outdated' do
       allow(subject).to receive(:status).and_return :outdated
       expect(subject).to receive(:upgrade)
-      subject.sync
+      expect(subject.sync).to be true
     end
 
     it 'installs the module when it is absent' do
       allow(subject).to receive(:status).and_return :absent
       expect(subject).to receive(:install)
-      subject.sync
+      expect(subject.sync).to be true
+    end
+
+    it 'returns false if `should_sync?` is false' do
+      # modules do not sync if they are not requested
+      mod = described_class.new('my_org/my_mod', '/path/to/mod', { overrides: { modules: { requested_modules: ['other_mod'] } } })
+      expect(mod.sync).to be false
     end
   end
 

spec/unit/module/git_spec.rb

@@ -128,6 +128,25 @@
       subject.sync
       expect(Dir.exist?(spec_path)).to eq true
     end
+
+    it 'returns true if repo was updated' do
+      allow(mock_repo).to receive(:resolve).with('master').and_return('abc123')
+      expect(mock_repo).to receive(:sync).and_return(true)
+      expect(subject.sync).to be true
+    end
+
+    it 'returns false if repo was not updated (in-sync)' do
+      allow(mock_repo).to receive(:resolve).with('master').and_return('abc123')
+      expect(mock_repo).to receive(:sync).and_return(false)
+      expect(subject.sync).to be false
+    end
+
+    it 'returns false if `should_sync?` is false' do
+      # modules do not sync if they are not requested
+      mod = described_class.new(title, dirname, { overrides: { modules: { requested_modules: ['other_mod'] } } })
+      allow(mock_repo).to receive(:resolve).with('master').and_return('abc123')
+      expect(mod.sync).to be false
+    end
   end
 
   describe "determining the status" do