Merge pull request #1161 from justinstoller/no-visit-modules

(maint) Do not use visitors to synchronize modules

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

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
 

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
 
@@ -51,11 +58,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,11 +195,6 @@ def visit_puppetfile(puppetfile)
           end
         end
 
-        def visit_module(mod)
-          logger.info _("Deploying %{origin} content %{path}") % {origin: mod.origin, path: mod.path}
-          mod.sync(force: @settings.dig(:overrides, :modules, :force))
-        end
-
         def write_environment_info!(environment, started_at, success)
           module_deploys =
             begin

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/, '_')] : []
@@ -28,7 +34,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
               },
@@ -40,12 +46,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
 
@@ -67,7 +78,15 @@ 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
 
@@ -76,20 +95,6 @@ def visit_puppetfile(puppetfile)
           yield
         end
 
-        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))
-            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})
-          end
-        end
-
         def allowed_initialize_opts
           super.merge(environment: true,
                       cachedir: :self,

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})
-          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,17 +39,6 @@ def visit_puppetfile(pf)
                                   pf.purge_exclusions).purge!
         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)
-        end
-
         def allowed_initialize_opts
           super.merge(root: :self, puppetfile: :self, moduledir: :self, force: :self )
         end

lib/r10k/content_synchronizer.rb

@@ -4,15 +4,15 @@ 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
 
     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}

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

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)
@@ -87,25 +87,27 @@ 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
 
   # @param [String] name
-  # @param [*Object] args
+  # @param [Hash] 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)

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

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) || []
+    @should_sync = (@requested_modules.empty? || @requested_modules.include?(@name))
   end
 
   # @deprecated
@@ -61,11 +65,22 @@ def full_path
   end
 
   # Synchronize this module with the indicated state.
-  # @abstract
+  # @param [Hash] opts Deprecated
   def sync(opts={})
     raise NotImplementedError
   end
 
+  def should_sync?
+    if @should_sync
+      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})
+      false
+    end
+  end
+
+
   # Return the desired version of this module
   # @abstract
   def version
@@ -87,6 +102,7 @@ def status
     raise NotImplementedError
   end
 
+  # Deprecated
   def accept(visitor)
     visitor.visit(:module, self)
   end