diff --git a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/keeps/rubocop_fixer.rb b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/keeps/rubocop_fixer.rb new file mode 100644 index 0000000000000000000000000000000000000000..06f13b792136553993ba50494e6bfedecffb7905 --- /dev/null +++ b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/keeps/rubocop_fixer.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'yaml' + +module Gitlab + module Housekeeper + module Keeps + class RubocopFixer < Keep + LIMIT_FIXES = 20 + RUBOCOP_TODO_DIR_PATTERN = ".rubocop_todo/**/*.yml" + + def initialize(todo_dir_pattern: RUBOCOP_TODO_DIR_PATTERN, limit_fixes: LIMIT_FIXES) + @todo_dir_pattern = todo_dir_pattern + @limit_fixes = limit_fixes + end + + def each_change + each_allowed_rubocop_rule do |rule, rule_file_path, violating_files| + remove_allow_rule = true + + if violating_files.count > @limit_fixes + violating_files = violating_files.first(@limit_fixes) + remove_allow_rule = false + end + + change = Change.new + change.title = "Fix #{violating_files.count} rubocop violations for #{rule}" + change.identifiers = [self.class.name, rule, violating_files.last] + change.description = <<~MARKDOWN + Fixes the #{violating_files.count} violations for the rubocop rule `#{rule}` + that were previously excluded in `#{rule_file_path}`. + The exclusions have now been removed. + MARKDOWN + + if remove_allow_rule + FileUtils.rm(rule_file_path) + else + remove_first_exclusions(rule, rule_file_path, violating_files.count) + end + + begin + ::Gitlab::Housekeeper::Shell.execute('rubocop', '--autocorrect', *violating_files) + rescue ::Gitlab::Housekeeper::Shell::Error + # Ignore when it cannot be automatically fixed. But we need to checkout any files we might have updated. + ::Gitlab::Housekeeper::Shell.execute('git', 'checkout', rule_file_path, *violating_files) + next + end + + change.changed_files = [rule_file_path, *violating_files] + + yield(change) + end + end + + def each_allowed_rubocop_rule + Dir.glob(@todo_dir_pattern).each do |file| + content = File.read(file) + next unless content.include?('Cop supports --autocorrect') + + data = YAML.safe_load(content) + + # Assume one per file since that's how it is in gitlab-org/gitlab + next unless data.keys.count == 1 + + rule = data.keys[0] + violating_files = data[rule]['Exclude'] + next unless violating_files&.count&.positive? + + yield rule, file, violating_files + end + end + + def remove_first_exclusions(rule, file, remove_count) + content = File.read(file) + skipped = 0 + + output = content.each_line.filter do |line| + if skipped < remove_count && line.match?(/\s+-\s+/) + skipped += 1 + false + else + true + end + end + + File.write(file, output.join) + end + end + end + end +end diff --git a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/runner.rb b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/runner.rb index ffc8d51690fbe9bd92b06fb7389afb4d8df81020..090f0c2d5e8a6a09e9428e8940cb60d5b208fabd 100644 --- a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/runner.rb +++ b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/runner.rb @@ -2,6 +2,7 @@ require 'active_support/core_ext/string' require 'gitlab/housekeeper/keep' +require 'gitlab/housekeeper/keeps/rubocop_fixer' require 'gitlab/housekeeper/gitlab_client' require 'gitlab/housekeeper/git' require 'gitlab/housekeeper/change' diff --git a/gems/gitlab-housekeeper/spec/fixtures/rubocop_todo1.yml b/gems/gitlab-housekeeper/spec/fixtures/rubocop_todo1.yml new file mode 100644 index 0000000000000000000000000000000000000000..522b3f9ce5f061b807e3744d9ea09d288c7e9579 --- /dev/null +++ b/gems/gitlab-housekeeper/spec/fixtures/rubocop_todo1.yml @@ -0,0 +1,9 @@ +--- +# Cop supports --autocorrect. +RuboCop/FakeRule1: + Exclude: + - 'rule1_violation1.rb' + - 'rule1_violation2.rb' + - 'rule1_violation3.rb' + - 'rule1_violation4.rb' + - 'rule1_violation5.rb' diff --git a/gems/gitlab-housekeeper/spec/fixtures/rubocop_todo2.yml b/gems/gitlab-housekeeper/spec/fixtures/rubocop_todo2.yml new file mode 100644 index 0000000000000000000000000000000000000000..9d9efc265af799d9a0967834a1da0f3578e1e119 --- /dev/null +++ b/gems/gitlab-housekeeper/spec/fixtures/rubocop_todo2.yml @@ -0,0 +1,12 @@ +--- +# Cop supports --autocorrect. +RuboCop/FakeRule2: + Exclude: + - rule2_violation1.rb + - rule2_violation2.rb + - rule2_violation3.rb + - rule2_violation4.rb + - rule2_violation5.rb + - rule2_violation6.rb + - rule2_violation7.rb + - rule2_violation8.rb diff --git a/gems/gitlab-housekeeper/spec/fixtures/rubocop_todo_not_autocorrectable.yml b/gems/gitlab-housekeeper/spec/fixtures/rubocop_todo_not_autocorrectable.yml new file mode 100644 index 0000000000000000000000000000000000000000..ef260026ba92b90bcdad149c60de920e33aaad45 --- /dev/null +++ b/gems/gitlab-housekeeper/spec/fixtures/rubocop_todo_not_autocorrectable.yml @@ -0,0 +1,6 @@ +--- +RuboCop/FakeRuleNotAutocorrectable: + Exclude: + - not_autocorrectable_rule1_violation1.rb + - not_autocorrectable_rule1_violation2.rb + - not_autocorrectable_rule1_violation3.rb diff --git a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/keeps/rubocop_fixer_spec.rb b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/keeps/rubocop_fixer_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..2b145794f5818fac5183081c13ad405d613925d2 --- /dev/null +++ b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/keeps/rubocop_fixer_spec.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Gitlab::Housekeeper::Keeps::RubocopFixer do + let(:todo_dir) { Dir.mktmpdir } + let(:rule1_file) { Pathname(todo_dir).join('rule1.yml').to_s } + let(:rule2_file) { Pathname(todo_dir).join('rule2.yml').to_s } + let(:not_autocorrectable_file) { Pathname(todo_dir).join('not_autocorrectable.yml').to_s } + let(:todo_dir_pattern) { Pathname(todo_dir).join('**/*.yml').to_s } + + before do + dir = Pathname.new(todo_dir) + FileUtils.cp('spec/fixtures/rubocop_todo1.yml', rule1_file) + FileUtils.cp('spec/fixtures/rubocop_todo2.yml', rule2_file) + FileUtils.cp('spec/fixtures/rubocop_todo_not_autocorrectable.yml', not_autocorrectable_file) + end + + after do + FileUtils.remove_entry(todo_dir) + end + + let(:rubocop_fixer) { described_class.new(todo_dir_pattern: todo_dir_pattern, limit_fixes: 5) } + + let(:rule1_violating_files) do + [ + 'rule1_violation1.rb', + 'rule1_violation2.rb', + 'rule1_violation3.rb', + 'rule1_violation4.rb', + 'rule1_violation5.rb' + ] + end + + let(:rule2_violating_files) do + [ + 'rule2_violation1.rb', + 'rule2_violation2.rb', + 'rule2_violation3.rb', + 'rule2_violation4.rb', + 'rule2_violation5.rb', + 'rule2_violation6.rb', + 'rule2_violation7.rb', + 'rule2_violation8.rb' + ] + end + + describe '#each_change' do + it 'iterates over todo_dir_pattern files' do + yielded_times = 0 + + allow(::Gitlab::Housekeeper::Shell).to receive(:execute) + + rubocop_fixer.each_change do |change| + yielded_times += 1 + + expect(change.title).to include("rubocop violations") + expect(change.description).to include("Fixes") + + if change.identifiers.include?('RuboCop/FakeRule1') + # rule1 contained only 5 exclusions and we fixed all of them so we should have deleted the file + expect(File).not_to exist(rule1_file) + + expect(change.changed_files).to eq([ + rule1_file, + *rule1_violating_files + ]) + + expect(::Gitlab::Housekeeper::Shell).to have_received(:execute) + .with('rubocop', '--autocorrect', *rule1_violating_files) + elsif change.identifiers.include?('RuboCop/FakeRule2') + # rule2 contained 8 total exclusions and we fixed 5 of them so there should be 3 left + expect(File).to exist(rule2_file) + rule2_content = YAML.load_file(rule2_file) + expect(rule2_content['RuboCop/FakeRule2']['Exclude']).to eq(rule2_violating_files.last(3)) + + expect(change.changed_files).to eq([ + rule2_file, + *rule2_violating_files.first(5) + ]) + + expect(::Gitlab::Housekeeper::Shell).to have_received(:execute) + .with('rubocop', '--autocorrect', *rule2_violating_files.first(5)) + else + raise "Unexpected change: #{change.identifiers}" + end + end + + expect(yielded_times).to eq(2) + end + + context 'when rubocop fails to fix the errors' do + it 'checks out the files' do + expect(::Gitlab::Housekeeper::Shell).to receive(:execute) + .once + .ordered + .with('rubocop', '--autocorrect', *rule1_violating_files) + .and_raise(::Gitlab::Housekeeper::Shell::Error) + expect(::Gitlab::Housekeeper::Shell).to receive(:execute) + .once + .ordered + .with('git', 'checkout', rule1_file, *rule1_violating_files) + + expect(::Gitlab::Housekeeper::Shell).to receive(:execute) + .once + .ordered + .with('rubocop', '--autocorrect', *rule2_violating_files.first(5)) + .and_raise(::Gitlab::Housekeeper::Shell::Error) + expect(::Gitlab::Housekeeper::Shell).to receive(:execute) + .once + .ordered + .with('git', 'checkout', rule2_file, *rule2_violating_files.first(5)) + + rubocop_fixer.each_change + end + end + end +end