Refactor puppet modules properties

This commit introduces a new class PuppetModule in order to simplify properties computation.

This new class:
 - allows a single `name`, `namespace` and the working directory computation
 - introduces the `given_name` property, which is the name the user puts in
 its configuration file
 - adds the capability a have a longer `namespace` (e.g. puppet/modules)

This commit also exposes the options through ModuleSync.options instead
of passing it to almost any methods.

As `name` and `namespace` can be confusing in code, so they have been
renamed to `repository_name` and `repository_namespace`.

This commit only introduces minor changes on the output of `msync`: some
single quotes have been added as delimiters for relevant dynamic part of
the messages; and the use of `given_name` (i.e. user-provided name) when
relevant (e.g. "Syncing 'voxpupuli/puppet-module'").

features/cli.feature

@@ -35,6 +35,9 @@ Feature: CLI
       """
     And a git_base option appended to "modulesync.yml" for local tests
     And a directory named "moduleroot"
-    When I run `msync update --noop --namespace fakenamespace`
+    When I run `msync update --noop --namespace fakenamespace --branch command-line-branch`
     Then the exit status should be 0
-    And the output should match /Syncing fakenamespace/
+    And the output should contain:
+      """
+      Creating new branch command-line-branch
+      """

features/update.feature

@@ -300,7 +300,7 @@ Feature: update
       """
     And the output should match:
       """
-      Not managing Gemfile in puppet-test
+      Not managing 'Gemfile' in 'puppet-test'
       """
     And the exit status should be 0
     And the file named "modules/fakenamespace/puppet-test/Gemfile" should contain:
@@ -370,7 +370,7 @@ Feature: update
     When I run `msync update --offline`
     Then the output should contain:
       """
-      Not managing spec/spec_helper.rb in puppet-apache
+      Not managing 'spec/spec_helper.rb' in 'puppet-apache'
       """
     And the exit status should be 0
     And the file named "modules/puppetlabs/puppet-apache/spec/spec_helper.rb" should contain:
@@ -607,7 +607,7 @@ Feature: update
     Then the exit status should be 0
     And the output should match:
       """
-      Not managing spec/spec_helper.rb in puppet-test
+      Not managing 'spec/spec_helper.rb' in 'puppet-test'
       """
     And the file named "modules/fakenamespace/puppet-test/global-test.md" should contain:
       """

lib/modulesync.rb

@@ -1,12 +1,15 @@
 require 'fileutils'
 require 'pathname'
+
 require 'modulesync/cli'
 require 'modulesync/constants'
 require 'modulesync/git'
 require 'modulesync/hook'
+require 'modulesync/puppet_module'
 require 'modulesync/renderer'
 require 'modulesync/settings'
 require 'modulesync/util'
+
 require 'monkey_patches'
 
 module ModuleSync # rubocop:disable Metrics/ModuleLength
@@ -21,12 +24,16 @@ def self.config_defaults
     }
   end
 
+  def self.options
+    @options
+  end
+
   def self.local_file(config_path, file)
     File.join(config_path, MODULE_FILES_DIR, file)
   end
 
-  def self.module_file(project_root, namespace, puppet_module, *parts)
-    File.join(project_root, namespace, puppet_module, *parts)
+  def self.module_file(puppet_module, *parts)
+    File.join(puppet_module.working_directory, *parts)
   end
 
   # List all template files.
@@ -50,7 +57,11 @@ def self.relative_names(file_list, path)
     file_list.map { |file| file.sub(/#{path}/, '') }
   end
 
-  def self.managed_modules(config_file, filter, negative_filter)
+  def self.managed_modules
+    config_file = config_path(options[:managed_modules_conf], options)
+    filter = options[:filter]
+    negative_filter = options[:negative_filter]
+
     managed_modules = Util.parse_config(config_file)
     if managed_modules.empty?
       $stderr.puts "No modules found in #{config_file}." \
@@ -59,12 +70,7 @@ def self.managed_modules(config_file, filter, negative_filter)
     end
     managed_modules.select! { |m| m =~ Regexp.new(filter) } unless filter.nil?
     managed_modules.reject! { |m| m =~ Regexp.new(negative_filter) } unless negative_filter.nil?
-    managed_modules
-  end
-
-  def self.module_name(module_name, default_namespace)
-    return [default_namespace, module_name] unless module_name.include?('/')
-    ns, mod = module_name.split('/')
+    managed_modules.map { |given_name, options| PuppetModule.new(given_name, options) }
   end
 
   def self.hook(options)
@@ -78,11 +84,11 @@ def self.hook(options)
     end
   end
 
-  def self.manage_file(filename, settings, options)
+  def self.manage_file(puppet_module, filename, settings, options)
     namespace = settings.additional_settings[:namespace]
     module_name = settings.additional_settings[:puppet_module]
     configs = settings.build_file_configs(filename)
-    target_file = module_file(options[:project_root], namespace, module_name, filename)
+    target_file = module_file(puppet_module, filename)
     if configs['delete']
       Renderer.remove(target_file)
     else
@@ -92,7 +98,7 @@ def self.manage_file(filename, settings, options)
         # Meta data passed to the template as @metadata[:name]
         metadata = {
           :module_name => module_name,
-          :workdir     => module_file(options[:project_root], namespace, module_name),
+          :workdir     => puppet_module.working_directory,
           :target_file => target_file,
         }
         template = Renderer.render(erb, configs, metadata)
@@ -104,38 +110,33 @@ def self.manage_file(filename, settings, options)
     end
   end
 
-  def self.manage_module(puppet_module, module_files, module_options, defaults, options)
-    default_namespace = options[:namespace]
-    if module_options.is_a?(Hash) && module_options.key?(:namespace)
-      default_namespace = module_options[:namespace]
-    end
-    namespace, module_name = module_name(puppet_module, default_namespace)
-    git_repo = File.join(namespace, module_name)
-    unless options[:offline]
-      Git.pull(options[:git_base], git_repo, options[:branch], options[:project_root], module_options || {})
-    end
+  def self.manage_module(puppet_module, module_files, defaults)
+    Git.pull(puppet_module, options[:branch]) unless options[:offline]
 
-    module_configs = Util.parse_config(module_file(options[:project_root], namespace, module_name, MODULE_CONF_FILE))
+    module_configs = Util.parse_config(module_file(puppet_module, MODULE_CONF_FILE))
     settings = Settings.new(defaults[GLOBAL_DEFAULTS_KEY] || {},
                             defaults,
                             module_configs[GLOBAL_DEFAULTS_KEY] || {},
                             module_configs,
-                            :puppet_module => module_name,
+                            :puppet_module => puppet_module.repository_name,
                             :git_base => options[:git_base],
-                            :namespace => namespace)
+                            :namespace => puppet_module.repository_namespace)
+
     settings.unmanaged_files(module_files).each do |filename|
-      $stdout.puts "Not managing #{filename} in #{module_name}"
+      $stdout.puts "Not managing '#{filename}' in '#{puppet_module.given_name}'"
     end
 
     files_to_manage = settings.managed_files(module_files)
-    files_to_manage.each { |filename| manage_file(filename, settings, options) }
+    files_to_manage.each { |filename| manage_file(puppet_module, filename, settings, options) }
 
     if options[:noop]
-      Git.update_noop(git_repo, options)
-      options[:pr] && pr(module_options).manage(namespace, module_name, options)
+      Git.update_noop(puppet_module.repository_path, options)
+      options[:pr] && \
+        pr(puppet_module).manage(puppet_module.repository_namespace, puppet_module.repository_name, options)
     elsif !options[:offline]
-      pushed = Git.update(git_repo, files_to_manage, options)
-      pushed && options[:pr] && pr(module_options).manage(namespace, module_name, options)
+      pushed = Git.update(puppet_module.repository_path, files_to_manage, options)
+      pushed && options[:pr] && \
+        pr(puppet_module).manage(puppet_module.repository_namespace, puppet_module.repository_name, options)
     end
   end
 
@@ -148,9 +149,10 @@ def config_path(file, options)
     self.class.config_path(file, options)
   end
 
-  def self.update(options)
-    options = config_defaults.merge(options)
+  def self.update(cli_options)
+    @options = config_defaults.merge(cli_options)
     defaults = Util.parse_config(config_path(CONF_FILE, options))
+
     if options[:pr]
       unless options[:branch]
         $stderr.puts 'A branch must be specified with --branch to use --pr!'
@@ -164,28 +166,23 @@ def self.update(options)
     local_files = find_template_files(local_template_dir)
     module_files = relative_names(local_files, local_template_dir)
 
-    managed_modules = self.managed_modules(config_path(options[:managed_modules_conf], options),
-                                           options[:filter],
-                                           options[:negative_filter])
-
     errors = false
     # managed_modules is either an array or a hash
-    managed_modules.each do |puppet_module, module_options|
+    managed_modules.each do |puppet_module|
       begin
-        mod_options = module_options.nil? ? nil : Util.symbolize_keys(module_options)
-        manage_module(puppet_module, module_files, mod_options, defaults, options)
+        manage_module(puppet_module, module_files, defaults)
       rescue # rubocop:disable Lint/RescueWithoutErrorClass
-        $stderr.puts "Error while updating #{puppet_module}"
+        warn "Error while updating '#{puppet_module.given_name}'"
         raise unless options[:skip_broken]
         errors = true
-        $stdout.puts "Skipping #{puppet_module} as update process failed"
+        $stdout.puts "Skipping '#{puppet_module.given_name}' as update process failed"
       end
     end
     exit 1 if errors && options[:fail_on_warnings]
   end
 
-  def self.pr(module_options)
-    module_options ||= {}
+  def self.pr(puppet_module)
+    module_options = puppet_module.options
     github_conf = module_options[:github]
     gitlab_conf = module_options[:gitlab]
 

lib/modulesync/git.rb

@@ -45,23 +45,22 @@ def self.switch_branch(repo, branch)
       end
     end
 
-    def self.pull(git_base, name, branch, project_root, opts)
-      puts "Syncing #{name}"
-      Dir.mkdir(project_root) unless Dir.exist?(project_root)
+    def self.pull(puppet_module, branch)
+      puts "Syncing '#{puppet_module.given_name}'"
 
       # Repo needs to be cloned in the cwd
-      if !Dir.exist?("#{project_root}/#{name}") || !Dir.exist?("#{project_root}/#{name}/.git")
+      if !Dir.exist?("#{puppet_module.working_directory}/.git")
         puts 'Cloning repository fresh'
-        remote = opts[:remote] || (git_base.start_with?('file://') ? "#{git_base}#{name}" : "#{git_base}#{name}.git")
-        local = "#{project_root}/#{name}"
-        puts "Cloning from #{remote}"
+        remote = puppet_module.repository_remote
+        local = puppet_module.working_directory
+        puts "Cloning from '#{remote}'"
         repo = ::Git.clone(remote, local)
         switch_branch(repo, branch)
       # Repo already cloned, check out master and override local changes
       else
         # Some versions of git can't properly handle managing a repo from outside the repo directory
-        Dir.chdir("#{project_root}/#{name}") do
-          puts "Overriding any local changes to repositories in #{project_root}"
+        Dir.chdir(puppet_module.working_directory) do
+          puts "Overriding any local changes to repositories in '#{puppet_module.working_directory}'"
           repo = ::Git.open('.')
           repo.fetch
           repo.reset_hard

lib/modulesync/puppet_module.rb

@@ -0,0 +1,49 @@
+require 'modulesync'
+require 'modulesync/util'
+
+module ModuleSync
+  # Provide methods to retrieve puppet module attributes
+  class PuppetModule
+    attr_reader :given_name
+    attr_reader :options
+
+    def initialize(given_name, options)
+      options ||= {}
+      @options = Util.symbolize_keys(options)
+
+      @given_name = given_name
+
+      return unless given_name.include?('/')
+
+      @repository_name = given_name.split('/').last
+      @repository_namespace = given_name.split('/')[0...-1].join('/')
+    end
+
+    def repository_name
+      @repository_name ||= given_name
+    end
+
+    def repository_namespace
+      @repository_namespace ||= @options[:namespace] || ModuleSync.options[:namespace]
+    end
+
+    def repository_path
+      @repository_path ||= "#{repository_namespace}/#{repository_name}"
+    end
+
+    def repository_remote
+      @repository_remote ||= @options[:remote] || _repository_remote
+    end
+
+    def working_directory
+      @working_directory ||= File.join(ModuleSync.options[:project_root], repository_path)
+    end
+
+    private
+
+    def _repository_remote
+      git_base = ModuleSync.options[:git_base]
+      git_base.start_with?('file://') ? "#{git_base}#{repository_path}" : "#{git_base}#{repository_path}.git"
+    end
+  end
+end

spec/unit/modulesync_spec.rb

@@ -1,11 +1,13 @@
 require 'spec_helper'
 
+require 'modulesync/puppet_module'
+
 describe ModuleSync do
   context '::update' do
     it 'loads the managed modules from the specified :managed_modules_conf' do
       allow(ModuleSync).to receive(:find_template_files).and_return([])
       allow(ModuleSync::Util).to receive(:parse_config).with('./config_defaults.yml').and_return({})
-      expect(ModuleSync).to receive(:managed_modules).with('./test_file.yml', nil, nil).and_return([])
+      expect(ModuleSync).to receive(:managed_modules).with(no_args).and_return([])
 
       options = { managed_modules_conf: 'test_file.yml' }
       ModuleSync.update(options)
@@ -14,8 +16,12 @@
 
   context '::pr' do
     describe "Raise Error" do
+      let(:puppet_module) do
+        ModuleSync::PuppetModule.new 'puppet-test', remote: 'dummy'
+      end
+
       it 'raises an error when neither GITHUB_TOKEN nor GITLAB_TOKEN are set for PRs' do
-        expect { ModuleSync.pr({}) }.to raise_error(RuntimeError).and output(/No GitHub or GitLab token specified for --pr/).to_stderr
+        expect { ModuleSync.pr(puppet_module) }.to raise_error(RuntimeError).and output(/No GitHub or GitLab token specified for --pr/).to_stderr
       end
     end
   end