Merge pull request #1216 from Magisus/module-postrun-bug

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

CHANGELOG.mkd

@@ -8,6 +8,7 @@ Unreleased
 - (CODEMGMT-1415) Provide a `--incremental` flag to only sync those modules in a Puppetfile whose definitions have changed since last sync, or those whose versions could change. [#1200](https://github.com/puppetlabs/r10k/pull/1200)
 - (CODEMGMT-1454) Ensure missing repo caches are re-synced [#1210](https://github.com/puppetlabs/r10k/pull/1210)
 - (PF-2437) Allow token authentication to be used with the Forge. [#1192](https://github.com/puppetlabs/r10k/pull/1192)
+- Only run the module postrun command for environments in which the module was modified. [#1215](https://github.com/puppetlabs/r10k/issues/1215)
 
 3.11.0
 ------

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|
+              updated = mod.sync
+              updated_modules << mod.name if updated
+            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 })
@@ -64,10 +66,13 @@ def sync(ref, force=true)
         @repo.checkout(sha, {:force => force})
       else
         logger.warn(_("Skipping %{repo_path} due to local modifications") % {repo_path: @repo.path})
+        updated = false
       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
         install
+        updated = true
       when :outdated
         upgrade
+        updated = true
       when :mismatched
         reinstall
+        updated = true
       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

@@ -74,18 +74,24 @@ def status
   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
         install
+        updated = true
       when :mismatched
         reinstall
+        updated = true
       when :outdated
         update
+        updated = true
       end
       maybe_delete_spec_dir
     end
+    updated
   end
 
   def exist?

spec/unit/action/deploy/module_spec.rb

@@ -102,33 +102,22 @@
         )
       end
 
-      before do
-        @modules = []
+      it 'generate_types is true' do
+        expect(subject.settings[:overrides][:environments][:generate_types]).to eq(true)
+      end
+
+      it 'only calls puppet generate types on environments where the specified module was updated' do
         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
-
-      it 'generate_types is true' do
-        expect(subject.settings[:overrides][:environments][:generate_types]).to eq(true)
-      end
-
-      it 'only calls puppet generate types on environments with specified module' do
         subject.call
-        expect(@modules.length).to be(2)
       end
     end
 
@@ -422,16 +411,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 +448,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 +463,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