From c573ceb88fc171a3e6a0bcb7f14a9e61792796f1 Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Thu, 24 Dec 2020 23:17:58 +0100 Subject: [PATCH 1/4] Refactor ModuleSync::Git (1/3) This commit annotates ModuleSync::Git methods to show how the code will be splitted. --- lib/modulesync/git.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/modulesync/git.rb b/lib/modulesync/git.rb index 10cab787..72f41bdc 100644 --- a/lib/modulesync/git.rb +++ b/lib/modulesync/git.rb @@ -5,25 +5,30 @@ module ModuleSync module Git # rubocop:disable Metrics/ModuleLength include Constants + # Repository def self.remote_branch_exists?(repo, branch) repo.branches.remote.collect(&:name).include?(branch) end + # Repository def self.local_branch_exists?(repo, branch) repo.branches.local.collect(&:name).include?(branch) end + # Repository def self.remote_branch_differ?(repo, local_branch, remote_branch) !remote_branch_exists?(repo, remote_branch) || repo.diff("#{local_branch}..origin/#{remote_branch}").any? end + # Repository def self.default_branch(repo) symbolic_ref = repo.branches.find { |b| b.full =~ %r{remotes/origin/HEAD} } return unless symbolic_ref %r{remotes/origin/HEAD\s+->\s+origin/(?.+?)$}.match(symbolic_ref.full)[:branch] end + # Repository def self.switch_branch(repo, branch) unless branch branch = default_branch(repo) @@ -45,6 +50,7 @@ def self.switch_branch(repo, branch) end end + # Repository (renamed to prepare_workspace) def self.pull(puppet_module, branch) puts "Syncing '#{puppet_module.given_name}'" @@ -70,6 +76,7 @@ def self.pull(puppet_module, branch) end end + # PuppetModule, is it used? def self.update_changelog(repo, version, message, module_root) changelog = "#{module_root}/CHANGELOG.md" if File.exist?(changelog) @@ -88,6 +95,7 @@ def self.update_changelog(repo, version, message, module_root) end end + # PuppetModule def self.bump(repo, m, message, module_root, changelog = false) new = m.bump! puts "Bumped to version #{new}" @@ -98,6 +106,7 @@ def self.bump(repo, m, message, module_root, changelog = false) new end + # Repository def self.tag(repo, version, tag_pattern) tag = tag_pattern % version puts "Tagging with #{tag}" @@ -105,6 +114,7 @@ def self.tag(repo, version, tag_pattern) repo.push('origin', tag) end + # Repository def self.checkout_branch(repo, branch) selected_branch = branch || repo.current_branch || 'master' repo.branch(selected_branch).checkout @@ -112,6 +122,7 @@ def self.checkout_branch(repo, branch) end private_class_method :checkout_branch + # Repository (renamed to submit_changes) # Git add/rm, git commit, git push def self.update(name, files, options) module_root = "#{options[:project_root]}/#{name}" @@ -161,6 +172,7 @@ def self.update(name, files, options) true end + # Repository # Needed because of a bug in the git gem that lists ignored files as # untracked under some circumstances # https://github.com/schacon/ruby-git/issues/130 @@ -170,6 +182,7 @@ def self.untracked_unignored_files(repo) repo.status.untracked.keep_if { |f, _| ignored.none? { |i| File.fnmatch(i, f) } } end + # Repository (renamed to show_changes) def self.update_noop(name, options) puts "Using no-op. Files in #{name} may be changed but will not be committed." From 1f0fead0305e1964e763ab6c43c02710f1a0a23d Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Thu, 24 Dec 2020 23:21:00 +0100 Subject: [PATCH 2/4] Refactor ModuleSync::Git (2/3) This commit renames ModuleSync::Git in ModuleSync::Repository --- lib/modulesync.rb | 8 ++++---- lib/modulesync/{git.rb => repository.rb} | 3 +-- 2 files changed, 5 insertions(+), 6 deletions(-) rename lib/modulesync/{git.rb => repository.rb} (98%) diff --git a/lib/modulesync.rb b/lib/modulesync.rb index 0adbfee3..25c1448e 100644 --- a/lib/modulesync.rb +++ b/lib/modulesync.rb @@ -3,7 +3,7 @@ require 'modulesync/cli' require 'modulesync/constants' -require 'modulesync/git' +require 'modulesync/repository' require 'modulesync/hook' require 'modulesync/puppet_module' require 'modulesync/renderer' @@ -111,7 +111,7 @@ def self.manage_file(puppet_module, filename, settings, options) end def self.manage_module(puppet_module, module_files, defaults) - Git.pull(puppet_module, options[:branch]) unless options[:offline] + Repository.pull(puppet_module, options[:branch]) unless options[:offline] module_configs = Util.parse_config(module_file(puppet_module, MODULE_CONF_FILE)) settings = Settings.new(defaults[GLOBAL_DEFAULTS_KEY] || {}, @@ -130,11 +130,11 @@ def self.manage_module(puppet_module, module_files, defaults) files_to_manage.each { |filename| manage_file(puppet_module, filename, settings, options) } if options[:noop] - Git.update_noop(puppet_module.repository_path, options) + Repository.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(puppet_module.repository_path, files_to_manage, options) + pushed = Repository.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 diff --git a/lib/modulesync/git.rb b/lib/modulesync/repository.rb similarity index 98% rename from lib/modulesync/git.rb rename to lib/modulesync/repository.rb index 72f41bdc..fc43e0bd 100644 --- a/lib/modulesync/git.rb +++ b/lib/modulesync/repository.rb @@ -2,8 +2,7 @@ require 'puppet_blacksmith' module ModuleSync - module Git # rubocop:disable Metrics/ModuleLength - include Constants + module Repository # Repository def self.remote_branch_exists?(repo, branch) From dcc462a4ed9fb42dc62098be566286188cefcecc Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Thu, 24 Dec 2020 23:58:13 +0100 Subject: [PATCH 3/4] Refactor ModuleSync::Git (3/3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit is the main refactoring part of ModuleSync::Repository (formerly ModuleSync::Git). - It turns ModuleSync::Repository into a class to hold its properties (ie. directory and remote) - It renames some methods for readability: * #pull → #prepare_workspace * #update_noop → #show_changes * #update → submit_changes - It moves some `puts` in ModuleSync for consistency and code reusability Note: A method Repository#repo have been created as an alias for #git to keep history. --- features/update.feature | 1 + lib/modulesync.rb | 9 ++- lib/modulesync/repository.rb | 121 +++++++++++++++++------------------ 3 files changed, 64 insertions(+), 67 deletions(-) diff --git a/features/update.feature b/features/update.feature index 8ac83e0b..653eaf48 100644 --- a/features/update.feature +++ b/features/update.feature @@ -461,6 +461,7 @@ Feature: update And a directory named "moduleroot" When I run `msync update --message "Running without changes"` Then the exit status should be 0 + And the stdout should contain "There were no changes in 'modules/fakenamespace/puppet-test'. Not committing." And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba" Scenario: When specifying configurations in managed_modules.yml diff --git a/lib/modulesync.rb b/lib/modulesync.rb index 25c1448e..2837ccfa 100644 --- a/lib/modulesync.rb +++ b/lib/modulesync.rb @@ -111,7 +111,9 @@ def self.manage_file(puppet_module, filename, settings, options) end def self.manage_module(puppet_module, module_files, defaults) - Repository.pull(puppet_module, options[:branch]) unless options[:offline] + repository = Repository.new directory: puppet_module.working_directory, remote: puppet_module.repository_remote + puts "Syncing '#{puppet_module.given_name}'" + repository.prepare_workspace(options[:branch]) unless options[:offline] module_configs = Util.parse_config(module_file(puppet_module, MODULE_CONF_FILE)) settings = Settings.new(defaults[GLOBAL_DEFAULTS_KEY] || {}, @@ -130,11 +132,12 @@ def self.manage_module(puppet_module, module_files, defaults) files_to_manage.each { |filename| manage_file(puppet_module, filename, settings, options) } if options[:noop] - Repository.update_noop(puppet_module.repository_path, options) + puts "Using no-op. Files in '#{puppet_module.given_name}' may be changed but will not be committed." + repository.show_changes(options) options[:pr] && \ pr(puppet_module).manage(puppet_module.repository_namespace, puppet_module.repository_name, options) elsif !options[:offline] - pushed = Repository.update(puppet_module.repository_path, files_to_manage, options) + pushed = repository.submit_changes(files_to_manage, options) pushed && options[:pr] && \ pr(puppet_module).manage(puppet_module.repository_namespace, puppet_module.repository_name, options) end diff --git a/lib/modulesync/repository.rb b/lib/modulesync/repository.rb index fc43e0bd..071b9d78 100644 --- a/lib/modulesync/repository.rb +++ b/lib/modulesync/repository.rb @@ -2,43 +2,52 @@ require 'puppet_blacksmith' module ModuleSync - module Repository + # Wrapper for Git in ModuleSync context + class Repository + def initialize(directory:, remote:) + @directory = directory + @remote = remote + end + + def git + @git ||= Git.open @directory + end - # Repository - def self.remote_branch_exists?(repo, branch) + # This is an alias to minimize code alteration + def repo + git + end + + def remote_branch_exists?(branch) repo.branches.remote.collect(&:name).include?(branch) end - # Repository - def self.local_branch_exists?(repo, branch) + def local_branch_exists?(branch) repo.branches.local.collect(&:name).include?(branch) end - # Repository - def self.remote_branch_differ?(repo, local_branch, remote_branch) - !remote_branch_exists?(repo, remote_branch) || + def remote_branch_differ?(local_branch, remote_branch) + !remote_branch_exists?(remote_branch) || repo.diff("#{local_branch}..origin/#{remote_branch}").any? end - # Repository - def self.default_branch(repo) + def default_branch symbolic_ref = repo.branches.find { |b| b.full =~ %r{remotes/origin/HEAD} } return unless symbolic_ref %r{remotes/origin/HEAD\s+->\s+origin/(?.+?)$}.match(symbolic_ref.full)[:branch] end - # Repository - def self.switch_branch(repo, branch) + def switch_branch(branch) unless branch - branch = default_branch(repo) + branch = default_branch puts "Using repository's default branch: #{branch}" end return if repo.current_branch == branch - if local_branch_exists?(repo, branch) + if local_branch_exists?(branch) puts "Switching to branch #{branch}" repo.checkout(branch) - elsif remote_branch_exists?(repo, branch) + elsif remote_branch_exists?(branch) puts "Creating local branch #{branch} from origin/#{branch}" repo.checkout("origin/#{branch}") repo.branch(branch).checkout @@ -49,35 +58,30 @@ def self.switch_branch(repo, branch) end end - # Repository (renamed to prepare_workspace) - def self.pull(puppet_module, branch) - puts "Syncing '#{puppet_module.given_name}'" - + def prepare_workspace(branch) # Repo needs to be cloned in the cwd - if !Dir.exist?("#{puppet_module.working_directory}/.git") + if !Dir.exist?("#{@directory}/.git") puts 'Cloning repository fresh' - remote = puppet_module.repository_remote - local = puppet_module.working_directory - puts "Cloning from '#{remote}'" - repo = ::Git.clone(remote, local) - switch_branch(repo, branch) + puts "Cloning from '#{@remote}'" + @git = Git.clone(@remote, @directory) + switch_branch(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(puppet_module.working_directory) do - puts "Overriding any local changes to repositories in '#{puppet_module.working_directory}'" - repo = ::Git.open('.') + Dir.chdir(@directory) do + puts "Overriding any local changes to repository in '#{@directory}'" + @git = Git.open('.') repo.fetch repo.reset_hard - switch_branch(repo, branch) - repo.pull('origin', branch) if remote_branch_exists?(repo, branch) + switch_branch(branch) + git.pull('origin', branch) if remote_branch_exists?(branch) end end end # PuppetModule, is it used? - def self.update_changelog(repo, version, message, module_root) - changelog = "#{module_root}/CHANGELOG.md" + def update_changelog(version, message) + changelog = "#{@directory}/CHANGELOG.md" if File.exist?(changelog) puts "Updating #{changelog} for version #{version}" changes = File.readlines(changelog) @@ -95,43 +99,38 @@ def self.update_changelog(repo, version, message, module_root) end # PuppetModule - def self.bump(repo, m, message, module_root, changelog = false) + def bump(message, changelog = false) + m = Blacksmith::Modulefile.new("#{@directory}/metadata.json") new = m.bump! puts "Bumped to version #{new}" repo.add('metadata.json') - update_changelog(repo, new, message, module_root) if changelog + update_changelog(new, message) if changelog repo.commit("Release version #{new}") repo.push new end - # Repository - def self.tag(repo, version, tag_pattern) + def tag(version, tag_pattern) tag = tag_pattern % version puts "Tagging with #{tag}" repo.add_tag(tag) repo.push('origin', tag) end - # Repository - def self.checkout_branch(repo, branch) + def checkout_branch(branch) selected_branch = branch || repo.current_branch || 'master' repo.branch(selected_branch).checkout selected_branch end - private_class_method :checkout_branch - # Repository (renamed to submit_changes) # Git add/rm, git commit, git push - def self.update(name, files, options) - module_root = "#{options[:project_root]}/#{name}" + def submit_changes(files, options) message = options[:message] - repo = ::Git.open(module_root) - branch = checkout_branch(repo, options[:branch]) + branch = checkout_branch(options[:branch]) files.each do |file| if repo.status.deleted.include?(file) repo.remove(file) - elsif File.exist?("#{module_root}/#{file}") + elsif File.exist?("#{@directory}/#{file}") repo.add(file) end end @@ -142,28 +141,27 @@ def self.update(name, files, options) opts_push = { :force => true } if options[:force] if options[:pre_commit_script] script = "#{File.dirname(File.dirname(__FILE__))}/../contrib/#{options[:pre_commit_script]}" - `#{script} #{module_root}` + `#{script} #{@directory}` end repo.commit(message, opts_commit) if options[:remote_branch] - if remote_branch_differ?(repo, branch, options[:remote_branch]) + if remote_branch_differ?(branch, options[:remote_branch]) repo.push('origin', "#{branch}:#{options[:remote_branch]}", opts_push) end else repo.push('origin', branch, opts_push) end # Only bump/tag if pushing didn't fail (i.e. there were changes) - m = Blacksmith::Modulefile.new("#{module_root}/metadata.json") if options[:bump] - new = bump(repo, m, message, module_root, options[:changelog]) - tag(repo, new, options[:tag_pattern]) if options[:tag] + new = bump(message, options[:changelog]) + tag(new, options[:tag_pattern]) if options[:tag] end - rescue ::Git::GitExecuteError => git_error - if git_error.message.match?(/working (directory|tree) clean/) - puts "There were no files to update in #{name}. Not committing." + rescue Git::GitExecuteError => e + if e.message.match?(/working (directory|tree) clean/) + puts "There were no changes in '#{@directory}'. Not committing." return false else - puts git_error + puts e raise end end @@ -171,22 +169,17 @@ def self.update(name, files, options) true end - # Repository # Needed because of a bug in the git gem that lists ignored files as # untracked under some circumstances # https://github.com/schacon/ruby-git/issues/130 - def self.untracked_unignored_files(repo) - ignore_path = "#{repo.dir.path}/.gitignore" + def untracked_unignored_files + ignore_path = "#{@directory}/.gitignore" ignored = File.exist?(ignore_path) ? File.read(ignore_path).split : [] repo.status.untracked.keep_if { |f, _| ignored.none? { |i| File.fnmatch(i, f) } } end - # Repository (renamed to show_changes) - def self.update_noop(name, options) - puts "Using no-op. Files in #{name} may be changed but will not be committed." - - repo = ::Git.open("#{options[:project_root]}/#{name}") - checkout_branch(repo, options[:branch]) + def show_changes(options) + checkout_branch(options[:branch]) puts 'Files changed:' repo.diff('HEAD', '--').each do |diff| @@ -194,7 +187,7 @@ def self.update_noop(name, options) end puts 'Files added:' - untracked_unignored_files(repo).each_key do |file| + untracked_unignored_files.each_key do |file| puts file end From 2461eded8c0f27253c593b38c29fc0d0ac431602 Mon Sep 17 00:00:00 2001 From: Romuald Conty Date: Wed, 21 Apr 2021 19:04:56 +0200 Subject: [PATCH 4/4] Rubocop: Update autogenerated todo file --- .rubocop_todo.yml | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 6a376960..20ac188a 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2021-04-21 12:35:40 +0200 using RuboCop version 0.50.0. +# on 2021-04-21 19:04:03 +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 @@ -11,42 +11,35 @@ Lint/UselessAssignment: Exclude: - 'lib/modulesync.rb' -# Offense count: 11 +# Offense count: 10 Metrics/AbcSize: - Max: 48 + Max: 53 -# Offense count: 1 +# Offense count: 2 # Configuration parameters: CountComments. Metrics/ClassLength: - Max: 106 + Max: 158 # Offense count: 3 Metrics/CyclomaticComplexity: Max: 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 + Max: 40 # Offense count: 3 Metrics/PerceivedComplexity: Max: 15 -# Offense count: 9 +# Offense count: 8 Style/Documentation: Exclude: - 'spec/**/*' - 'test/**/*' - 'lib/modulesync.rb' - 'lib/modulesync/cli.rb' - - 'lib/modulesync/git.rb' - 'lib/modulesync/hook.rb' - 'lib/modulesync/renderer.rb' - 'lib/modulesync/util.rb'