Merge branch 'feature/gb/migrate-pipeline-stages' into feature/gb/persist-pipeline-stages

* feature/gb/migrate-pipeline-stages:
  Migrate stages only with correct foreign references
  Create index on pipeline stages after migrating stages
  Improve indexes and refs in pipeline stages migrations
  Fix typo in build stages reference migration
  Improve order of columns in pipeline stages table
  Make pipeline stages ref migration more readable

db/migrate/20170525132202_create_pipeline_stages.rb

@@ -3,12 +3,23 @@ class CreatePipelineStages < ActiveRecord::Migration
 
   DOWNTIME = false
 
-  def change
+  disable_ddl_transaction!
+
+  def up
     create_table :ci_stages do |t|
       t.integer :project_id
       t.integer :pipeline_id
-      t.string :name
       t.timestamps null: true
+      t.string :name
     end
+
+    add_concurrent_foreign_key :ci_stages, :projects, column: :project_id, on_delete: :cascade
+    add_concurrent_foreign_key :ci_stages, :ci_pipelines, column: :pipeline_id, on_delete: :cascade
+    add_concurrent_index :ci_stages, :project_id
+    add_concurrent_index :ci_stages, :pipeline_id
+  end
+
+  def down
+    drop_table :ci_stages
   end
 end

db/migrate/20170526185602_add_stage_id_to_ci_builds.rb

@@ -3,7 +3,19 @@ class AddStageIdToCiBuilds < ActiveRecord::Migration
 
   DOWNTIME = false
 
-  def change
+  disable_ddl_transaction!
+
+  def up
     add_column :ci_builds, :stage_id, :integer
+
+    add_concurrent_foreign_key :ci_builds, :ci_stages, column: :stage_id, on_delete: :cascade
+    add_concurrent_index :ci_builds, :stage_id
+  end
+
+  def down
+    remove_concurrent_index :ci_builds, :stage_id
+    remove_foreign_key :ci_builds, column: :stage_id
+
+    remove_column :ci_builds, :stage_id, :integer
   end
 end

db/post_migrate/20170526185842_migrate_pipeline_stages.rb

@@ -11,13 +11,12 @@ class MigratePipelineStages < ActiveRecord::Migration
     execute <<-SQL.strip_heredoc
       INSERT INTO ci_stages (project_id, pipeline_id, name)
         SELECT project_id, commit_id, stage FROM ci_builds
-          WHERE stage IS NOT NULL AND stage_id IS NULL
+          WHERE stage IS NOT NULL
+          AND stage_id IS NULL
+          AND EXISTS (SELECT 1 FROM projects WHERE projects.id = ci_builds.project_id)
+          AND EXISTS (SELECT 1 FROM ci_pipelines WHERE ci_pipelines.id = ci_builds.commit_id)
           GROUP BY project_id, commit_id, stage
           ORDER BY MAX(stage_idx)
     SQL
   end
-
-  def down
-    execute('TRUNCATE TABLE ci_stages')
-  end
 end

db/post_migrate/20170526185921_migrate_build_stage_reference.rb

@@ -6,9 +6,11 @@ class MigrateBuildStageReference < ActiveRecord::Migration
   def up
     disable_statement_timeout
 
-    stage_id = Arel.sql('(SELECT id FROM ci_stages ' \
-                         'WHERE ci_stages.pipeline_id = ci_builds.commit_id ' \
-                         'AND ci_stages.name = ci_builds.stage)')
+    stage_id = Arel.sql <<-SQL.strip_heredoc
+      (SELECT id FROM ci_stages
+         WHERE ci_stages.pipeline_id = ci_builds.commit_id
+           AND ci_stages.name = ci_builds.stage)
+    SQL
 
     update_column_in_batches(:ci_builds, :stage_id, stage_id) do |table, query|
       query.where(table[:stage_id].eq(nil))

db/post_migrate/20170526190708_create_foreign_keys_for_pipeline_stages.rb

-class CreateForeignKeysForPipelineStages < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    disable_statement_timeout
-
-    execute <<~SQL
-      DELETE FROM ci_stages
-        WHERE NOT EXISTS (
-          SELECT true FROM projects
-            WHERE projects.id = ci_stages.project_id
-        )
-    SQL
-
-    execute <<~SQL
-      DELETE FROM ci_builds
-        WHERE NOT EXISTS (
-          SELECT true FROM ci_stages
-            WHERE ci_stages.id = ci_builds.stage_id
-        )
-    SQL
-
-    add_concurrent_foreign_key :ci_stages, :projects, column: :project_id, on_delete: :cascade
-    add_concurrent_foreign_key :ci_builds, :ci_stages, column: :stage_id, on_delete: :cascade
-  end
-
-  def down
-    remove_foreign_key :ci_stages, column: :project_id
-    remove_foreign_key :ci_builds, column: :stage_id
-  end
-end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170526190708) do
+ActiveRecord::Schema.define(version: 20170526185921) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -243,6 +243,7 @@ ActiveRecord::Schema.define(version: 20170526190708) do
   add_index "ci_builds", ["commit_id", "type", "ref"], name: "index_ci_builds_on_commit_id_and_type_and_ref", using: :btree
   add_index "ci_builds", ["project_id"], name: "index_ci_builds_on_project_id", using: :btree
   add_index "ci_builds", ["runner_id"], name: "index_ci_builds_on_runner_id", using: :btree
+  add_index "ci_builds", ["stage_id"], name: "index_ci_builds_on_stage_id", using: :btree
   add_index "ci_builds", ["status", "type", "runner_id"], name: "index_ci_builds_on_status_and_type_and_runner_id", using: :btree
   add_index "ci_builds", ["status"], name: "index_ci_builds_on_status", using: :btree
   add_index "ci_builds", ["token"], name: "index_ci_builds_on_token", unique: true, using: :btree
@@ -330,12 +331,14 @@ ActiveRecord::Schema.define(version: 20170526190708) do
   create_table "ci_stages", force: :cascade do |t|
     t.integer "project_id"
     t.integer "pipeline_id"
-    t.string "name"
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.string "name"
   end
 
   add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", using: :btree
+  add_index "ci_stages", ["pipeline_id"], name: "index_ci_stages_on_pipeline_id", using: :btree
+  add_index "ci_stages", ["project_id"], name: "index_ci_stages_on_project_id", using: :btree
 
   create_table "ci_trigger_requests", force: :cascade do |t|
     t.integer "trigger_id", null: false
@@ -1497,6 +1500,7 @@ ActiveRecord::Schema.define(version: 20170526190708) do
   add_foreign_key "ci_pipeline_schedules", "users", column: "owner_id", name: "fk_9ea99f58d2", on_delete: :nullify
   add_foreign_key "ci_pipelines", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_3d34ab2e06", on_delete: :nullify
   add_foreign_key "ci_pipelines", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_262d4c2d19", on_delete: :nullify
+  add_foreign_key "ci_stages", "ci_pipelines", column: "pipeline_id", name: "fk_fb57e6cc56", on_delete: :cascade
   add_foreign_key "ci_stages", "projects", name: "fk_2360681d1d", on_delete: :cascade
   add_foreign_key "ci_trigger_requests", "ci_triggers", column: "trigger_id", name: "fk_b8ec8b7245", on_delete: :cascade
   add_foreign_key "ci_triggers", "users", column: "owner_id", name: "fk_e8e10d1964", on_delete: :cascade

spec/migrations/migrate_build_stage_reference_spec.rb

@@ -9,8 +9,15 @@ describe MigrateBuildStageReference, :migration do
   let(:jobs) { table(:ci_builds) }
   let(:stages) { table(:ci_stages) }
   let(:pipelines) { table(:ci_pipelines) }
+  let(:projects) { table(:projects) }
 
   before do
+    # Create projects
+    #
+    projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1')
+    projects.create!(id: 456, name: 'gitlab2', path: 'gitlab2')
+    projects.create!(id: 798, name: 'gitlab3', path: 'gitlab3')
+
     # Create CI/CD pipelines
     #
     pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a')

spec/migrations/migrate_pipeline_stages_spec.rb

@@ -9,8 +9,14 @@ describe MigratePipelineStages, :migration do
   let(:jobs) { table(:ci_builds) }
   let(:stages) { table(:ci_stages) }
   let(:pipelines) { table(:ci_pipelines) }
+  let(:projects) { table(:projects) }
 
   before do
+    # Create projects
+    #
+    projects.create!(id: 123, name: 'gitlab1', path: 'gitlab1')
+    projects.create!(id: 456, name: 'gitlab2', path: 'gitlab2')
+
     # Create CI/CD pipelines
     #
     pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a')
@@ -28,7 +34,8 @@ describe MigratePipelineStages, :migration do
     jobs.create!(id: 8, commit_id: 2, project_id: 456, stage_idx: 1, stage: 'test:1')
     jobs.create!(id: 9, commit_id: 2, project_id: 456, stage_idx: 1, stage: 'test:1')
     jobs.create!(id: 10, commit_id: 2, project_id: 456, stage_idx: 2, stage: 'test:2')
-    jobs.create!(id: 11, commit_id: 3, project_id: 789, stage_idx: 3, stage: 'deploy')
+    jobs.create!(id: 11, commit_id: 3, project_id: 456, stage_idx: 3, stage: 'deploy')
+    jobs.create!(id: 12, commit_id: 2, project_id: 789, stage_idx: 3, stage: 'deploy')
   end
 
   it 'correctly migrates pipeline stages' do
@@ -36,12 +43,14 @@ describe MigratePipelineStages, :migration do
 
     migrate!
 
-    expect(stages.count).to eq 7
+    expect(stages.count).to eq 6
     expect(stages.all.pluck(:name))
-      .to match_array %w[test build deploy test:1 test:2 deploy deploy]
+      .to match_array %w[test build deploy test:1 test:2 deploy]
     expect(stages.where(pipeline_id: 1).order(:id).pluck(:name))
       .to eq %w[test build deploy]
     expect(stages.where(pipeline_id: 2).order(:id).pluck(:name))
       .to eq %w[test:1 test:2 deploy]
+    expect(stages.where(pipeline_id: 3).count).to be_zero
+    expect(stages.where(project_id: 789).count).to be_zero
   end
 end