Housekeeper: Stop revealing RuboCop TODOs during autocorrection

Move RuboCop autocorrect call into a helper method so it's easier to
reuse.

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)

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

gems/gitlab-housekeeper/spec/gitlab/housekeeper/keeps/rubocop_fixer_spec.rb

 # 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

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

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