From 2fe2318d340a51e89691aaf88c487b4e72a0282d Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Wed, 21 Apr 2021 12:35:18 +0200 Subject: [PATCH 1/3] Rubocop: Update autogenerated todo file --- .rubocop_todo.yml | 40 +++++++++++----------------------------- 1 file changed, 11 insertions(+), 29 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index b620e5d4..7b76c81e 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2015-10-07 12:25:07 -0700 using RuboCop version 0.34.2. +# on 2021-04-21 12:34:40 +0200 using RuboCop version 0.50.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -10,61 +10,43 @@ Lint/UselessAssignment: Exclude: - 'lib/modulesync.rb' - - 'lib/modulesync/cli.rb' -# Offense count: 6 +# Offense count: 10 Metrics/AbcSize: - Max: 64 + Max: 41 # Offense count: 1 # Configuration parameters: CountComments. Metrics/ClassLength: - Max: 107 + Max: 106 # Offense count: 4 Metrics/CyclomaticComplexity: - Max: 13 + Max: 12 -# Offense count: 8 +# Offense count: 12 # Configuration parameters: CountComments. Metrics/MethodLength: - Max: 79 - -# Offense count: 1 -# Configuration parameters: CountComments. -Metrics/ModuleLength: - Max: 140 + Max: 43 # Offense count: 4 Metrics/PerceivedComplexity: - Max: 16 + Max: 15 # Offense count: 9 -# Configuration parameters: Exclude. Style/Documentation: Exclude: + - 'spec/**/*' + - 'test/**/*' - 'lib/modulesync.rb' - 'lib/modulesync/cli.rb' - - 'lib/modulesync/constants.rb' - 'lib/modulesync/git.rb' - 'lib/modulesync/hook.rb' - 'lib/modulesync/renderer.rb' - 'lib/modulesync/util.rb' -# Offense count: 1 -Style/EachWithObject: - Exclude: - - 'lib/modulesync/util.rb' - -# Offense count: 1 -# Configuration parameters: MinBodyLength. -Style/GuardClause: - Exclude: - - 'lib/modulesync/cli.rb' - # Offense count: 1 # Cop supports --auto-correct. -# Configuration parameters: AllowAsExpressionSeparator. -Style/Semicolon: +Style/EachWithObject: Exclude: - 'lib/modulesync/util.rb' From 0db72d079dcd9f4cc09aa5aacfeb59f12d063d38 Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Thu, 24 Dec 2020 20:53:58 +0100 Subject: [PATCH 2/3] 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 | 7 ++- features/update.feature | 6 +-- lib/modulesync.rb | 85 ++++++++++++++++----------------- lib/modulesync/git.rb | 17 ++++--- lib/modulesync/puppet_module.rb | 49 +++++++++++++++++++ spec/unit/modulesync_spec.rb | 8 +++- 6 files changed, 112 insertions(+), 60 deletions(-) create mode 100644 lib/modulesync/puppet_module.rb diff --git a/features/cli.feature b/features/cli.feature index 111de29f..ce3d6e61 100644 --- a/features/cli.feature +++ b/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 + """ diff --git a/features/update.feature b/features/update.feature index f7d94c79..8ac83e0b 100644 --- a/features/update.feature +++ b/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: """ diff --git a/lib/modulesync.rb b/lib/modulesync.rb index 9efed9c7..0adbfee3 100644 --- a/lib/modulesync.rb +++ b/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] diff --git a/lib/modulesync/git.rb b/lib/modulesync/git.rb index e6f70b69..10cab787 100644 --- a/lib/modulesync/git.rb +++ b/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 diff --git a/lib/modulesync/puppet_module.rb b/lib/modulesync/puppet_module.rb new file mode 100644 index 00000000..d485c5e3 --- /dev/null +++ b/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 diff --git a/spec/unit/modulesync_spec.rb b/spec/unit/modulesync_spec.rb index b1d0d977..42b82168 100644 --- a/spec/unit/modulesync_spec.rb +++ b/spec/unit/modulesync_spec.rb @@ -5,7 +5,7 @@ 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 +14,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 From 900b02cc5b60f3ade6404973869de83779497495 Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Wed, 21 Apr 2021 12:41:15 +0200 Subject: [PATCH 3/3] Rubocop: Update autogenerated todo file Please note that previous commit introduced some offences that will be fixed in upcomming commits (some are already available in #206 --- .rubocop_todo.yml | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 7b76c81e..6a376960 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,35 +1,41 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2021-04-21 12:34:40 +0200 using RuboCop version 0.50.0. +# on 2021-04-21 12:35:40 +0200 using RuboCop version 0.50.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. -# Offense count: 2 +# Offense count: 1 Lint/UselessAssignment: Exclude: - 'lib/modulesync.rb' -# Offense count: 10 +# Offense count: 11 Metrics/AbcSize: - Max: 41 + Max: 48 # Offense count: 1 # Configuration parameters: CountComments. Metrics/ClassLength: Max: 106 -# Offense count: 4 +# Offense count: 3 Metrics/CyclomaticComplexity: Max: 12 -# Offense count: 12 +# Offense count: 2 +# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns. +# URISchemes: http, https +Metrics/LineLength: + Max: 132 + +# Offense count: 13 # Configuration parameters: CountComments. Metrics/MethodLength: Max: 43 -# Offense count: 4 +# Offense count: 3 Metrics/PerceivedComplexity: Max: 15