Schedule background migrations and specs

4 files changed, 27 insertions, 56 deletions

diff --git a/db/migrate/20190211131150_add_state_id_to_issuables.rb b/db/migrate/20190211131150_add_state_id_to_issuables.rb
index af02aa84afd..b9d52fe63cd 100644
--- a/db/migrate/20190211131150_add_state_id_to_issuables.rb
+++ b/db/migrate/20190211131150_add_state_id_to_issuables.rb
@@ -1,5 +1,6 @@
 class AddStateIdToIssuables < ActiveRecord::Migration[5.0]
   include Gitlab::Database::MigrationHelpers
+  #include AfterCommitQueue
 
   DOWNTIME = false
   MIGRATION = 'SyncIssuablesStateId'.freeze
@@ -26,8 +27,13 @@ class AddStateIdToIssuables < ActiveRecord::Migration[5.0]
     add_column :issues, :state_id, :integer, limit: 1
     add_column :merge_requests, :state_id, :integer, limit: 1
 
-    queue_background_migration_jobs_by_range_at_intervals(Issue.where(state_id: nil), MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
-    queue_background_migration_jobs_by_range_at_intervals(MergeRequest.where(state_id: nil), MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+    # Is this safe?
+    # Added to avoid an warning about jobs running inside transactions.
+    # Since we only add a column this should be ok
+    Sidekiq::Worker.skipping_transaction_check do
+      queue_background_migration_jobs_by_range_at_intervals(Issue.where(state_id: nil), MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+      queue_background_migration_jobs_by_range_at_intervals(MergeRequest.where(state_id: nil), MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
+    end
   end
 
   def down
diff --git a/lib/gitlab/background_migration/sync_issuables_state_id.rb b/lib/gitlab/background_migration/sync_issuables_state_id.rb
index 1ac86b8acf2..95734a7310e 100644
--- a/lib/gitlab/background_migration/sync_issuables_state_id.rb
+++ b/lib/gitlab/background_migration/sync_issuables_state_id.rb
@@ -4,15 +4,15 @@
 module Gitlab
   module BackgroundMigration
     class SyncIssuablesStateId
-      def perform(start_id, end_id, model_class)
-        populate_new_state_id(start_id, end_id, model_class)
+      def perform(start_id, end_id, table_name)
+        populate_new_state_id(start_id, end_id, table_name)
       end
 
-      def populate_new_state_id(start_id, end_id, model_class)
-        Rails.logger.info("#{model_class.model_name.human} - Populating state_id: #{start_id} - #{end_id}")
+      def populate_new_state_id(start_id, end_id, table_name)
+        Rails.logger.info("#{table_name} - Populating state_id: #{start_id} - #{end_id}")
 
         ActiveRecord::Base.connection.execute <<~SQL
-          UPDATE #{model_class.table_name}
+          UPDATE #{table_name}
           SET state_id =
             CASE state
             WHEN 'opened' THEN 1
diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb
index 20cbb9e096b..46b36d07c20 100644
--- a/lib/gitlab/database/migration_helpers.rb
+++ b/lib/gitlab/database/migration_helpers.rb
@@ -1029,11 +1029,12 @@ into similar problems in the future (e.g. when new tables are created).
 
         model_class.each_batch(of: batch_size) do |relation, index|
           start_id, end_id = relation.pluck('MIN(id), MAX(id)').first
+          table_name = relation.base_class.table_name
 
           # `BackgroundMigrationWorker.bulk_perform_in` schedules all jobs for
           # the same time, which is not helpful in most cases where we wish to
           # spread the work over time.
-          BackgroundMigrationWorker.perform_in(delay_interval * index, job_class_name, [start_id, end_id, model_class])
+          BackgroundMigrationWorker.perform_in(delay_interval * index, job_class_name, [start_id, end_id, table_name])
         end
       end
 
diff --git a/spec/migrations/add_state_id_to_issuables_spec.rb b/spec/migrations/add_state_id_to_issuables_spec.rb
index 4416f416c18..b0e285db1f3 100644
--- a/spec/migrations/add_state_id_to_issuables_spec.rb
+++ b/spec/migrations/add_state_id_to_issuables_spec.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 require 'spec_helper'
-require Rails.root.join('db', 'migrate', '20190206144959_change_issuable_states_to_integer.rb')
+require Rails.root.join('db', 'migrate', '20190211131150_add_state_id_to_issuables.rb')
 
 describe AddStateIdToIssuables, :migration do
   let(:namespaces) { table(:namespaces) }
@@ -20,14 +20,15 @@ describe AddStateIdToIssuables, :migration do
       it 'migrates state column to integer' do
         opened_issue = issues.create!(description: 'first', state: 'opened')
         closed_issue = issues.create!(description: 'second', state: 'closed')
+        invalid_state_issue = issues.create!(description: 'fourth', state: 'not valid')
         nil_state_issue = issues.create!(description: 'third', state: nil)
 
         migrate!
 
-        issues.reset_column_information
-        expect(opened_issue.reload.state).to eq(Issue.states.opened)
-        expect(closed_issue.reload.state).to eq(Issue.states.closed)
-        expect(nil_state_issue.reload.state).to eq(nil)
+        expect(opened_issue.reload.state_id).to eq(Issue.states.opened)
+        expect(closed_issue.reload.state_id).to eq(Issue.states.closed)
+        expect(invalid_state_issue.reload.state_id).to be_nil
+        expect(nil_state_issue.reload.state_id).to be_nil
       end
     end
 
@@ -37,52 +38,15 @@ describe AddStateIdToIssuables, :migration do
         closed_merge_request = merge_requests.create!(state: 'closed', target_project_id: @project.id, target_branch: 'feature2', source_branch: 'master')
         merged_merge_request = merge_requests.create!(state: 'merged', target_project_id: @project.id, target_branch: 'feature3', source_branch: 'master')
         locked_merge_request = merge_requests.create!(state: 'locked', target_project_id: @project.id, target_branch: 'feature4', source_branch: 'master')
-        nil_state_merge_request = merge_requests.create!(state: nil, target_project_id: @project.id, target_branch: 'feature5', source_branch: 'master')
+        invalid_state_merge_request = merge_requests.create!(state: 'not valid', target_project_id: @project.id, target_branch: 'feature5', source_branch: 'master')
 
         migrate!
 
-        merge_requests.reset_column_information
-        expect(opened_merge_request.reload.state).to eq(MergeRequest.states.opened)
-        expect(closed_merge_request.reload.state).to eq(MergeRequest.states.closed)
-        expect(merged_merge_request.reload.state).to eq(MergeRequest.states.merged)
-        expect(locked_merge_request.reload.state).to eq(MergeRequest.states.locked)
-        expect(nil_state_merge_request.reload.state).to eq(nil)
-      end
-    end
-  end
-
-  describe '#down' do
-    context 'issues' do
-      it 'migrates state column to string' do
-        opened_issue = issues.create!(description: 'first', state: 1)
-        closed_issue = issues.create!(description: 'second', state: 2)
-        nil_state_issue = issues.create!(description: 'third', state: nil)
-
-        migration.down
-
-        issues.reset_column_information
-        expect(opened_issue.reload.state).to eq('opened')
-        expect(closed_issue.reload.state).to eq('closed')
-        expect(nil_state_issue.reload.state).to eq(nil)
-      end
-    end
-
-    context 'merge requests' do
-      it 'migrates state column to string' do
-        opened_merge_request = merge_requests.create!(state: 1, target_project_id: @project.id, target_branch: 'feature1', source_branch: 'master')
-        closed_merge_request = merge_requests.create!(state: 2, target_project_id: @project.id, target_branch: 'feature2', source_branch: 'master')
-        merged_merge_request = merge_requests.create!(state: 3, target_project_id: @project.id, target_branch: 'feature3', source_branch: 'master')
-        locked_merge_request = merge_requests.create!(state: 4, target_project_id: @project.id, target_branch: 'feature4', source_branch: 'master')
-        nil_state_merge_request = merge_requests.create!(state: nil, target_project_id: @project.id, target_branch: 'feature5', source_branch: 'master')
-
-        migration.down
-
-        merge_requests.reset_column_information
-        expect(opened_merge_request.reload.state).to eq('opened')
-        expect(closed_merge_request.reload.state).to eq('closed')
-        expect(merged_merge_request.reload.state).to eq('merged')
-        expect(locked_merge_request.reload.state).to eq('locked')
-        expect(nil_state_merge_request.reload.state).to eq(nil)
+        expect(opened_merge_request.reload.state_id).to eq(MergeRequest.states.opened)
+        expect(closed_merge_request.reload.state_id).to eq(MergeRequest.states.closed)
+        expect(merged_merge_request.reload.state_id).to eq(MergeRequest.states.merged)
+        expect(locked_merge_request.reload.state_id).to eq(MergeRequest.states.locked)
+        expect(invalid_state_merge_request.reload.state_id).to be_nil
       end
     end
   end