Merge branch '343047-restrict-direct-scheduling-of-bg-migrations' into 'master'

Update BG migration cops for ci database worker

See merge request gitlab-org/gitlab!79684

rubocop/cop/migration/schedule_async.rb

@@ -16,14 +16,23 @@ module RuboCop
         MSG
 
         def_node_matcher :calls_background_migration_worker?, <<~PATTERN
-          (send (const nil? :BackgroundMigrationWorker) {:perform_async :perform_in :bulk_perform_async :bulk_perform_in} ... )
+          (send (const {cbase nil?} :BackgroundMigrationWorker) #perform_method? ...)
+        PATTERN
+
+        def_node_matcher :calls_ci_database_worker?, <<~PATTERN
+          (send (const {(const {cbase nil?} :BackgroundMigration) nil?} :CiDatabaseWorker) #perform_method? ...)
+        PATTERN
+
+        def_node_matcher :perform_method?, <<~PATTERN
+          {:perform_async :bulk_perform_async :perform_in :bulk_perform_in}
         PATTERN
 
         def on_send(node)
           return unless in_migration?(node)
           return if version(node) < ENFORCED_SINCE
+          return unless calls_background_migration_worker?(node) || calls_ci_database_worker?(node)
 
-          add_offense(node, location: :expression) if calls_background_migration_worker?(node)
+          add_offense(node, location: :expression)
         end
       end
     end

rubocop/cop/scalability/bulk_perform_with_context.rb

@@ -23,6 +23,8 @@ module RuboCop
           Read more about it https://docs.gitlab.com/ee/development/sidekiq_style_guide.html#worker-context
         MSG
 
+        BACKGROUND_MIGRATION_WORKER_NAMES = %w[BackgroundMigrationWorker CiDatabaseWorker].freeze
+
         def_node_matcher :schedules_in_batch_without_context?, <<~PATTERN
           (send (...) {:bulk_perform_async :bulk_perform_in} _*)
         PATTERN
@@ -30,7 +32,7 @@ module RuboCop
         def on_send(node)
           return if in_migration?(node) || in_spec?(node)
           return unless schedules_in_batch_without_context?(node)
-          return if name_of_receiver(node) == "BackgroundMigrationWorker"
+          return if scheduled_for_background_migration?(node)
 
           add_offense(node, location: :expression)
         end
@@ -40,6 +42,10 @@ module RuboCop
         def in_spec?(node)
           file_path_for_node(node).end_with?("_spec.rb")
         end
+
+        def scheduled_for_background_migration?(node)
+          BACKGROUND_MIGRATION_WORKER_NAMES.include?(name_of_receiver(node))
+        end
       end
     end
   end

spec/rubocop/cop/migration/schedule_async_spec.rb

@@ -53,6 +53,17 @@ RSpec.describe RuboCop::Cop::Migration::ScheduleAsync do
         end
       end
 
+      context 'CiDatabaseWorker.perform_async' do
+        it 'adds an offense when calling `CiDatabaseWorker.peform_async`' do
+          expect_offense(<<~RUBY)
+            def up
+              CiDatabaseWorker.perform_async(ClazzName, "Bar", "Baz")
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't call [...]
+            end
+          RUBY
+        end
+      end
+
       context 'BackgroundMigrationWorker.perform_in' do
         it 'adds an offense' do
           expect_offense(<<~RUBY)
@@ -65,6 +76,18 @@ RSpec.describe RuboCop::Cop::Migration::ScheduleAsync do
         end
       end
 
+      context 'CiDatabaseWorker.perform_in' do
+        it 'adds an offense' do
+          expect_offense(<<~RUBY)
+            def up
+              CiDatabaseWorker
+              ^^^^^^^^^^^^^^^^ Don't call [...]
+                .perform_in(delay, ClazzName, "Bar", "Baz")
+            end
+          RUBY
+        end
+      end
+
       context 'BackgroundMigrationWorker.bulk_perform_async' do
         it 'adds an offense' do
           expect_offense(<<~RUBY)
@@ -77,12 +100,36 @@ RSpec.describe RuboCop::Cop::Migration::ScheduleAsync do
         end
       end
 
+      context 'CiDatabaseWorker.bulk_perform_async' do
+        it 'adds an offense' do
+          expect_offense(<<~RUBY)
+            def up
+              BackgroundMigration::CiDatabaseWorker
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't call [...]
+                .bulk_perform_async(jobs)
+            end
+          RUBY
+        end
+      end
+
       context 'BackgroundMigrationWorker.bulk_perform_in' do
         it 'adds an offense' do
           expect_offense(<<~RUBY)
             def up
-              BackgroundMigrationWorker
-              ^^^^^^^^^^^^^^^^^^^^^^^^^ Don't call [...]
+              ::BackgroundMigrationWorker
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't call [...]
+                .bulk_perform_in(5.minutes, jobs)
+            end
+          RUBY
+        end
+      end
+
+      context 'CiDatabaseWorker.bulk_perform_in' do
+        it 'adds an offense' do
+          expect_offense(<<~RUBY)
+            def up
+              ::BackgroundMigration::CiDatabaseWorker
+              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Don't call [...]
                 .bulk_perform_in(5.minutes, jobs)
             end
           RUBY

spec/rubocop/cop/scalability/bulk_perform_with_context_spec.rb

@@ -39,9 +39,15 @@ RSpec.describe RuboCop::Cop::Scalability::BulkPerformWithContext do
     CODE
   end
 
-  it "does not add an offense for scheduling BackgroundMigrations" do
+  it "does not add an offense for scheduling on the BackgroundMigrationWorker" do
     expect_no_offenses(<<~CODE)
       BackgroundMigrationWorker.bulk_perform_in(args)
     CODE
   end
+
+  it "does not add an offense for scheduling on the CiDatabaseWorker" do
+    expect_no_offenses(<<~CODE)
+      BackgroundMigration::CiDatabaseWorker.bulk_perform_in(args)
+    CODE
+  end
 end