diff --git a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/keeps/rubocop_fixer.rb b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/keeps/rubocop_fixer.rb
index f0085e17ed7834e01ca3e7dc0b8da3e89dea9c62..1ce66805621cf6b535624bd2d10c76c7a9565f10 100644
--- a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/keeps/rubocop_fixer.rb
+++ b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/keeps/rubocop_fixer.rb
@@ -40,9 +40,7 @@ def each_change
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
+ unless Gitlab::Housekeeper::Shell.rubocop_autocorrect(violating_files)
@logger.warn "Failed to autocorrect files. Reverting"
# 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)
diff --git a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/shell.rb b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/shell.rb
index ed51073f0edfd080f76993f07ba5217d4eec5bb8..4a8034d849ec3cf4d54eb1e08acd4633ebfb32d4 100644
--- a/gems/gitlab-housekeeper/lib/gitlab/housekeeper/shell.rb
+++ b/gems/gitlab-housekeeper/lib/gitlab/housekeeper/shell.rb
@@ -7,8 +7,10 @@ module Housekeeper
class Shell
Error = Class.new(StandardError)
- def self.execute(*cmd)
- stdin, stdout, stderr, wait_thr = Open3.popen3(*cmd)
+ def self.execute(*cmd, env: {})
+ env = ENV.to_h.merge(env)
+
+ stdin, stdout, stderr, wait_thr = Open3.popen3(env, *cmd)
stdin.close
out = stdout.read
@@ -22,6 +24,20 @@ def self.execute(*cmd)
out + err
end
+
+ # Run `rubocop --autocorrect --force-exclusion`.
+ #
+ # RuboCop is run without revealed TODOs.
+ def self.rubocop_autocorrect(files)
+ # Stop revealing RuboCop TODOs so RuboCop is only fixing material offenses.
+ env = { 'REVEAL_RUBOCOP_TODO' => nil }
+ cmd = ['rubocop', '--autocorrect', '--force-exclusion']
+
+ ::Gitlab::Housekeeper::Shell.execute(*cmd, *files, env: env)
+ true
+ rescue ::Gitlab::Housekeeper::Shell::Error
+ false
+ end
end
end
end
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
index 80dad94dc4f7d9d3a8f2fbdde201756512e50cef..6c40290c9d81785eb996e0f264cf470f2c1080c1 100644
--- a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/keeps/rubocop_fixer_spec.rb
+++ b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/keeps/rubocop_fixer_spec.rb
@@ -1,5 +1,6 @@
# frozen_string_literal: true
+require 'tmpdir'
require 'spec_helper'
# rubocop:disable RSpec/MultipleMemoizedHelpers
@@ -49,7 +50,10 @@
it 'iterates over todo_dir_pattern files' do
yielded_times = 0
+ # Stub out git
allow(::Gitlab::Housekeeper::Shell).to receive(:execute)
+ # Spy on rubocop
+ allow(::Gitlab::Housekeeper::Shell).to receive(:rubocop_autocorrect).and_call_original
rubocop_fixer.each_change do |change|
yielded_times += 1
@@ -66,8 +70,8 @@
*rule1_violating_files
])
- expect(::Gitlab::Housekeeper::Shell).to have_received(:execute)
- .with('rubocop', '--autocorrect', *rule1_violating_files)
+ expect(::Gitlab::Housekeeper::Shell).to have_received(:rubocop_autocorrect)
+ .with(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)
@@ -79,8 +83,8 @@
*rule2_violating_files.first(5)
])
- expect(::Gitlab::Housekeeper::Shell).to have_received(:execute)
- .with('rubocop', '--autocorrect', *rule2_violating_files.first(5))
+ expect(::Gitlab::Housekeeper::Shell).to have_received(:rubocop_autocorrect)
+ .with(rule2_violating_files.first(5))
else
raise "Unexpected change: #{change.identifiers}"
end
@@ -91,24 +95,21 @@
context 'when rubocop fails to fix the errors' do
it 'checks out the files' do
- expect(::Gitlab::Housekeeper::Shell).to receive(:execute)
+ expect(::Gitlab::Housekeeper::Shell).to receive(:rubocop_autocorrect)
.once
- .ordered
- .with('rubocop', '--autocorrect', *rule1_violating_files)
- .and_raise(::Gitlab::Housekeeper::Shell::Error)
+ .with(rule1_violating_files)
+ .and_return(false)
expect(::Gitlab::Housekeeper::Shell).to receive(:execute)
.once
.ordered
.with('git', 'checkout', rule1_file, *rule1_violating_files)
- expect(::Gitlab::Housekeeper::Shell).to receive(:execute)
+ expect(::Gitlab::Housekeeper::Shell).to receive(:rubocop_autocorrect)
.once
- .ordered
- .with('rubocop', '--autocorrect', *rule2_violating_files.first(5))
- .and_raise(::Gitlab::Housekeeper::Shell::Error)
+ .with(rule2_violating_files.first(5))
+ .and_return(false)
expect(::Gitlab::Housekeeper::Shell).to receive(:execute)
.once
- .ordered
.with('git', 'checkout', rule2_file, *rule2_violating_files.first(5))
rubocop_fixer.each_change
diff --git a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/shell_spec.rb b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/shell_spec.rb
index 2c7b3fb01c898c70b070cb4d440e123a55f8160b..de301f9274319ad579cdbc775384af807c1f218c 100644
--- a/gems/gitlab-housekeeper/spec/gitlab/housekeeper/shell_spec.rb
+++ b/gems/gitlab-housekeeper/spec/gitlab/housekeeper/shell_spec.rb
@@ -6,7 +6,7 @@
RSpec.describe ::Gitlab::Housekeeper::Shell do
describe '.execute' do
it 'delegates to popen3 and returns stdout' do
- expect(Open3).to receive(:popen3).with('echo', 'hello world')
+ expect(Open3).to receive(:popen3).with(ENV.to_h, 'echo', 'hello world')
.and_call_original
expect(described_class.execute('echo', 'hello world')).to eq("hello world\n")
@@ -21,4 +21,27 @@
)
end
end
+
+ describe '.rubocop_autocorrect' do
+ it 'delegates to #execute and returns true on success' do
+ expect(described_class)
+ .to receive(:execute)
+ .with(
+ 'rubocop', '--autocorrect', '--force-exclusion', 'foo.rb',
+ env: a_hash_including('REVEAL_RUBOCOP_TODO' => nil)
+ )
+ .and_return('output')
+
+ expect(described_class.rubocop_autocorrect('foo.rb')).to eq(true)
+ end
+
+ it 'delegates to #execute and returns false on failure' do
+ allow(described_class)
+ .to receive(:execute)
+ .with(any_args)
+ .and_raise(described_class::Error.new)
+
+ expect(described_class.rubocop_autocorrect('foo.rb')).to eq(false)
+ end
+ end
end
diff --git a/keeps/overdue_finalize_background_migration.rb b/keeps/overdue_finalize_background_migration.rb
index a7ebbc041a999372fdaff668a614dd83fad5f814..19d4735e4891805928f0f73b72c5b026693fcab5 100644
--- a/keeps/overdue_finalize_background_migration.rb
+++ b/keeps/overdue_finalize_background_migration.rb
@@ -80,7 +80,7 @@ def each_change
change.changed_files = [migration_file]
add_ensure_call_to_migration(migration_file, queue_method_node, job_name, migration_record)
- ::Gitlab::Housekeeper::Shell.execute('rubocop', '-a', migration_file)
+ ::Gitlab::Housekeeper::Shell.rubocop_autocorrect(migration_file)
digest = Digest::SHA256.hexdigest(generator.migration_number)
digest_file = Pathname.new('db').join('schema_migrations', generator.migration_number.to_s).to_s