From 2ab69f0d0c825f8546f189a61189246d6c90b7ff Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 5 Dec 2017 12:02:07 +0100 Subject: Schedule full build stage migration in the background For builds that are still missing `stage_id`. --- ...0171205101928_schedule_build_stage_migration.rb | 20 ++++++++++ .../schedule_build_stage_migration_spec.rb | 43 ++++++++++++++++++++++ 2 files changed, 63 insertions(+) create mode 100644 db/post_migrate/20171205101928_schedule_build_stage_migration.rb create mode 100644 spec/migrations/schedule_build_stage_migration_spec.rb diff --git a/db/post_migrate/20171205101928_schedule_build_stage_migration.rb b/db/post_migrate/20171205101928_schedule_build_stage_migration.rb new file mode 100644 index 00000000000..60293c60e8e --- /dev/null +++ b/db/post_migrate/20171205101928_schedule_build_stage_migration.rb @@ -0,0 +1,20 @@ +class ScheduleBuildStageMigration < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + MIGRATION = 'MigrateBuildStage'.freeze + BATCH = 10_000 + + class Build < ActiveRecord::Base + include EachBatch + self.table_name = 'ci_builds' + end + + def change + Build.where('stage_id IS NULL').each_batch(of: BATCH) do |builds, index| + builds.pluck(:id).map { |id| [MIGRATION, [id]] }.tap do |migrations| + BackgroundMigrationWorker.perform_bulk_in(index.minutes, migrations) + end + end + end +end diff --git a/spec/migrations/schedule_build_stage_migration_spec.rb b/spec/migrations/schedule_build_stage_migration_spec.rb new file mode 100644 index 00000000000..f80c0a94b32 --- /dev/null +++ b/spec/migrations/schedule_build_stage_migration_spec.rb @@ -0,0 +1,43 @@ +require 'spec_helper' +require Rails.root.join('db', 'post_migrate', '20171205101928_schedule_build_stage_migration') + +describe ScheduleBuildStageMigration, :migration do + let(:projects) { table(:projects) } + let(:pipelines) { table(:ci_pipelines) } + let(:stages) { table(:ci_stages) } + let(:jobs) { table(:ci_builds) } + + before do + ## + # Dependencies + # + projects.create!(id: 123, name: 'gitlab', path: 'gitlab-ce') + pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') + stages.create!(id: 1, project_id: 123, pipeline_id: 1, name: 'test') + + ## + # CI/CD jobs + # + jobs.create!(id: 10, commit_id: 1, project_id: 123, stage_id: nil) + jobs.create!(id: 20, commit_id: 1, project_id: 123, stage_id: nil) + jobs.create!(id: 30, commit_id: 1, project_id: 123, stage_id: nil) + jobs.create!(id: 40, commit_id: 1, project_id: 123, stage_id: 1) + end + + before do + stub_const("#{described_class}::BATCH", 1) + end + + it 'schedules background migrations in batches in bulk' do + Sidekiq::Testing.fake! do + Timecop.freeze do + migrate! + + expect(described_class::MIGRATION).to be_scheduled_migration(1.minutes, 10) + expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 20) + expect(described_class::MIGRATION).to be_scheduled_migration(3.minutes, 30) + expect(BackgroundMigrationWorker.jobs.size).to eq 3 + end + end + end +end -- cgit v1.2.1 From cd6d0dbd5fe9548ab7c0cd7b09e640a7b425daf1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 5 Dec 2017 12:42:24 +0100 Subject: Migrate a build stage completely in a background migration --- .../background_migration/migrate_build_stage.rb | 67 ++++++++++++++++++++++ .../migrate_build_stage_spec.rb | 48 ++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 lib/gitlab/background_migration/migrate_build_stage.rb create mode 100644 spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb diff --git a/lib/gitlab/background_migration/migrate_build_stage.rb b/lib/gitlab/background_migration/migrate_build_stage.rb new file mode 100644 index 00000000000..05edaab29d2 --- /dev/null +++ b/lib/gitlab/background_migration/migrate_build_stage.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true +# rubocop:disable Metrics/AbcSize +# rubocop:disable Style/Documentation + +module Gitlab + module BackgroundMigration + class MigrateBuildStage + def perform(id) + Ci::Build.find_by(id: id).try do |build| + Stage.new(build).tap do |stage| + return if stage.exists? + + stage.ensure! + stage.migrate_reference! + stage.migrate_status! + end + end + end + + private + + class Ci::Stage < ActiveRecord::Base + self.table_name = 'ci_stages' + end + + class Ci::Build < ActiveRecord::Base + self.table_name = 'ci_builds' + end + + class Stage + def initialize(build) + @build = build + end + + def exists? + @build.reload.stage_id.present? + end + + def ensure! + find || create! + end + + def find + Ci::Stage.find_by(name: @build.stage, + pipeline_id: @build.commit_id, + project_id: @build.project_id) + end + + def create! + Ci::Stage.create!(name: @build.stage, + pipeline_id: @build.commit_id, + project_id: @build.project_id) + end + + def migrate_reference! + MigrateBuildStageIdReference.new.perform(@build.id, @build.id) + end + + def migrate_status! + raise ArgumentError unless exists? + + MigrateStageStatus.new.perform(@build.stage_id, @build.stage_id) + end + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb b/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb new file mode 100644 index 00000000000..baa9c532c6d --- /dev/null +++ b/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb @@ -0,0 +1,48 @@ +require 'spec_helper' + +describe Gitlab::BackgroundMigration::MigrateBuildStage, :migration, schema: 20171205101928 do + let(:projects) { table(:projects) } + let(:pipelines) { table(:ci_pipelines) } + let(:stages) { table(:ci_stages) } + let(:jobs) { table(:ci_builds) } + + STATUSES = { created: 0, pending: 1, running: 2, success: 3, + failed: 4, canceled: 5, skipped: 6, manual: 7 }.freeze + + before do + ## + # Dependencies + # + projects.create!(id: 123, name: 'gitlab', path: 'gitlab-ce') + pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') + + ## + # CI/CD jobs + # + jobs.create!(id: 1, commit_id: 1, project_id: 123, + stage_idx: 2, stage: 'build', status: :success) + jobs.create!(id: 2, commit_id: 1, project_id: 123, + stage_idx: 2, stage: 'build', status: :success) + jobs.create!(id: 3, commit_id: 1, project_id: 123, + stage_idx: 1, stage: 'test', status: :failed) + jobs.create!(id: 4, commit_id: 1, project_id: 123, + stage_idx: 1, stage: 'test', status: :success) + jobs.create!(id: 5, commit_id: 1, project_id: 123, + stage_idx: 3, stage: 'deploy', status: :pending) + end + + it 'correctly migrates builds stages' do + expect(stages.count).to be_zero + + jobs.all.find_each do |job| + described_class.new.perform(job.id) + end + + expect(stages.count).to eq 3 + expect(stages.all.pluck(:name)).to match_array %w[test build deploy] + expect(jobs.where(stage_id: nil)).to be_empty + expect(stages.all.pluck(:status)).to match_array [STATUSES[:success], + STATUSES[:failed], + STATUSES[:pending]] + end +end -- cgit v1.2.1 From b50270a54efaa448e06018ad9a824ef4a512da31 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 5 Dec 2017 14:20:17 +0100 Subject: Fix Rubocop offenses in build status migrations :cop: --- lib/gitlab/background_migration/migrate_build_stage.rb | 4 +--- spec/migrations/schedule_build_stage_migration_spec.rb | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/background_migration/migrate_build_stage.rb b/lib/gitlab/background_migration/migrate_build_stage.rb index 05edaab29d2..a870609e68d 100644 --- a/lib/gitlab/background_migration/migrate_build_stage.rb +++ b/lib/gitlab/background_migration/migrate_build_stage.rb @@ -8,7 +8,7 @@ module Gitlab def perform(id) Ci::Build.find_by(id: id).try do |build| Stage.new(build).tap do |stage| - return if stage.exists? + break if stage.exists? stage.ensure! stage.migrate_reference! @@ -17,8 +17,6 @@ module Gitlab end end - private - class Ci::Stage < ActiveRecord::Base self.table_name = 'ci_stages' end diff --git a/spec/migrations/schedule_build_stage_migration_spec.rb b/spec/migrations/schedule_build_stage_migration_spec.rb index f80c0a94b32..c840d0b77fc 100644 --- a/spec/migrations/schedule_build_stage_migration_spec.rb +++ b/spec/migrations/schedule_build_stage_migration_spec.rb @@ -33,7 +33,7 @@ describe ScheduleBuildStageMigration, :migration do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to be_scheduled_migration(1.minutes, 10) + expect(described_class::MIGRATION).to be_scheduled_migration(1.minute, 10) expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 20) expect(described_class::MIGRATION).to be_scheduled_migration(3.minutes, 30) expect(BackgroundMigrationWorker.jobs.size).to eq 3 -- cgit v1.2.1 From 1ab0ffe3830340c0c9c2def74f59c95a44bcf9f7 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 6 Jan 2018 14:21:45 +0100 Subject: Update background stages migration timestamp --- .../20171205101928_schedule_build_stage_migration.rb | 20 -------------------- .../20180105101928_schedule_build_stage_migration.rb | 20 ++++++++++++++++++++ db/schema.rb | 2 +- .../background_migration/migrate_build_stage_spec.rb | 2 +- .../schedule_build_stage_migration_spec.rb | 2 +- 5 files changed, 23 insertions(+), 23 deletions(-) delete mode 100644 db/post_migrate/20171205101928_schedule_build_stage_migration.rb create mode 100644 db/post_migrate/20180105101928_schedule_build_stage_migration.rb diff --git a/db/post_migrate/20171205101928_schedule_build_stage_migration.rb b/db/post_migrate/20171205101928_schedule_build_stage_migration.rb deleted file mode 100644 index 60293c60e8e..00000000000 --- a/db/post_migrate/20171205101928_schedule_build_stage_migration.rb +++ /dev/null @@ -1,20 +0,0 @@ -class ScheduleBuildStageMigration < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - MIGRATION = 'MigrateBuildStage'.freeze - BATCH = 10_000 - - class Build < ActiveRecord::Base - include EachBatch - self.table_name = 'ci_builds' - end - - def change - Build.where('stage_id IS NULL').each_batch(of: BATCH) do |builds, index| - builds.pluck(:id).map { |id| [MIGRATION, [id]] }.tap do |migrations| - BackgroundMigrationWorker.perform_bulk_in(index.minutes, migrations) - end - end - end -end diff --git a/db/post_migrate/20180105101928_schedule_build_stage_migration.rb b/db/post_migrate/20180105101928_schedule_build_stage_migration.rb new file mode 100644 index 00000000000..3d7f6278244 --- /dev/null +++ b/db/post_migrate/20180105101928_schedule_build_stage_migration.rb @@ -0,0 +1,20 @@ +class ScheduleBuildStageMigration < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + MIGRATION = 'MigrateBuildStage'.freeze + BATCH = 10_000 + + class Build < ActiveRecord::Base + include EachBatch + self.table_name = 'ci_builds' + end + + def change + Build.where('stage_id IS NULL').each_batch(of: BATCH) do |builds, index| + builds.pluck(:id).map { |id| [MIGRATION, [id]] }.tap do |migrations| + BackgroundMigrationWorker.bulk_perform_in(index.minutes, migrations) + end + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 740e80ccfd4..1eba30b28c8 100644 --- a/db/schema.rb +++ b/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: 20171230123729) do +ActiveRecord::Schema.define(version: 20180105101928) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb b/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb index baa9c532c6d..b675db463af 100644 --- a/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb +++ b/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::BackgroundMigration::MigrateBuildStage, :migration, schema: 20171205101928 do +describe Gitlab::BackgroundMigration::MigrateBuildStage, :migration, schema: 20180105101928 do let(:projects) { table(:projects) } let(:pipelines) { table(:ci_pipelines) } let(:stages) { table(:ci_stages) } diff --git a/spec/migrations/schedule_build_stage_migration_spec.rb b/spec/migrations/schedule_build_stage_migration_spec.rb index c840d0b77fc..485ff84819d 100644 --- a/spec/migrations/schedule_build_stage_migration_spec.rb +++ b/spec/migrations/schedule_build_stage_migration_spec.rb @@ -1,5 +1,5 @@ require 'spec_helper' -require Rails.root.join('db', 'post_migrate', '20171205101928_schedule_build_stage_migration') +require Rails.root.join('db', 'post_migrate', '20180105101928_schedule_build_stage_migration') describe ScheduleBuildStageMigration, :migration do let(:projects) { table(:projects) } -- cgit v1.2.1 From aaa678cbc69ad5768d076a8e1cc3f249b2adeb29 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 6 Jan 2018 14:27:07 +0100 Subject: Update specs for delayed background migration of stages --- spec/migrations/schedule_build_stage_migration_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/migrations/schedule_build_stage_migration_spec.rb b/spec/migrations/schedule_build_stage_migration_spec.rb index 485ff84819d..c96acceeeda 100644 --- a/spec/migrations/schedule_build_stage_migration_spec.rb +++ b/spec/migrations/schedule_build_stage_migration_spec.rb @@ -28,14 +28,14 @@ describe ScheduleBuildStageMigration, :migration do stub_const("#{described_class}::BATCH", 1) end - it 'schedules background migrations in batches in bulk' do + it 'schedules delayed background migrations in batches in bulk' do Sidekiq::Testing.fake! do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to be_scheduled_migration(1.minute, 10) - expect(described_class::MIGRATION).to be_scheduled_migration(2.minutes, 20) - expect(described_class::MIGRATION).to be_scheduled_migration(3.minutes, 30) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(1.minute, 10) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 20) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(3.minutes, 30) expect(BackgroundMigrationWorker.jobs.size).to eq 3 end end -- cgit v1.2.1 From 3c89252e53147fed0d134988a6ec29aa2c02abe5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 6 Jan 2018 14:29:35 +0100 Subject: Reduce batch size of stages we schedule for a migration --- db/post_migrate/20180105101928_schedule_build_stage_migration.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/post_migrate/20180105101928_schedule_build_stage_migration.rb b/db/post_migrate/20180105101928_schedule_build_stage_migration.rb index 3d7f6278244..b33127140bb 100644 --- a/db/post_migrate/20180105101928_schedule_build_stage_migration.rb +++ b/db/post_migrate/20180105101928_schedule_build_stage_migration.rb @@ -3,7 +3,7 @@ class ScheduleBuildStageMigration < ActiveRecord::Migration DOWNTIME = false MIGRATION = 'MigrateBuildStage'.freeze - BATCH = 10_000 + BATCH = 1000 class Build < ActiveRecord::Base include EachBatch -- cgit v1.2.1 From 2d2f9e51594e33f04bfda4012e3d4c7e48539fe3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 6 Jan 2018 14:44:43 +0100 Subject: Do not attempt to migrate legacy pipeline stages --- .../background_migration/migrate_build_stage.rb | 33 ++++++++++++++-------- .../migrate_build_stage_spec.rb | 5 +++- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/lib/gitlab/background_migration/migrate_build_stage.rb b/lib/gitlab/background_migration/migrate_build_stage.rb index a870609e68d..7139ac03737 100644 --- a/lib/gitlab/background_migration/migrate_build_stage.rb +++ b/lib/gitlab/background_migration/migrate_build_stage.rb @@ -6,9 +6,9 @@ module Gitlab module BackgroundMigration class MigrateBuildStage def perform(id) - Ci::Build.find_by(id: id).try do |build| - Stage.new(build).tap do |stage| - break if stage.exists? + DatabaseBuild.find_by(id: id).try do |build| + MigratableStage.new(build).tap do |stage| + break if stage.exists? || stage.legacy? stage.ensure! stage.migrate_reference! @@ -17,15 +17,15 @@ module Gitlab end end - class Ci::Stage < ActiveRecord::Base + class DatabaseStage < ActiveRecord::Base self.table_name = 'ci_stages' end - class Ci::Build < ActiveRecord::Base + class DatabaseBuild < ActiveRecord::Base self.table_name = 'ci_builds' end - class Stage + class MigratableStage def initialize(build) @build = build end @@ -34,20 +34,29 @@ module Gitlab @build.reload.stage_id.present? end + ## + # We can have some very old stages that do not have `ci_builds.stage` set. + # + # In that case we just don't migrate such stage. + # + def legacy? + @build.stage.nil? + end + def ensure! find || create! end def find - Ci::Stage.find_by(name: @build.stage, - pipeline_id: @build.commit_id, - project_id: @build.project_id) + DatabaseStage.find_by(name: @build.stage, + pipeline_id: @build.commit_id, + project_id: @build.project_id) end def create! - Ci::Stage.create!(name: @build.stage, - pipeline_id: @build.commit_id, - project_id: @build.project_id) + DatabaseStage.create!(name: @build.stage, + pipeline_id: @build.commit_id, + project_id: @build.project_id) end def migrate_reference! diff --git a/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb b/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb index b675db463af..d20b5c29a71 100644 --- a/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb +++ b/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb @@ -29,6 +29,8 @@ describe Gitlab::BackgroundMigration::MigrateBuildStage, :migration, schema: 201 stage_idx: 1, stage: 'test', status: :success) jobs.create!(id: 5, commit_id: 1, project_id: 123, stage_idx: 3, stage: 'deploy', status: :pending) + jobs.create!(id: 6, commit_id: 1, project_id: 123, + stage_idx: 3, stage: nil, status: :pending) end it 'correctly migrates builds stages' do @@ -40,7 +42,8 @@ describe Gitlab::BackgroundMigration::MigrateBuildStage, :migration, schema: 201 expect(stages.count).to eq 3 expect(stages.all.pluck(:name)).to match_array %w[test build deploy] - expect(jobs.where(stage_id: nil)).to be_empty + expect(jobs.where(stage_id: nil)).to be_one + expect(jobs.find_by(stage_id: nil).id).to eq 6 expect(stages.all.pluck(:status)).to match_array [STATUSES[:success], STATUSES[:failed], STATUSES[:pending]] -- cgit v1.2.1 From fefb8ef74f2c269f1aecffc59fba32eb90c8ba73 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 6 Jan 2018 14:56:09 +0100 Subject: Randomize build ids in stages migration test data --- spec/migrations/schedule_build_stage_migration_spec.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/migrations/schedule_build_stage_migration_spec.rb b/spec/migrations/schedule_build_stage_migration_spec.rb index c96acceeeda..79f00fdbcf0 100644 --- a/spec/migrations/schedule_build_stage_migration_spec.rb +++ b/spec/migrations/schedule_build_stage_migration_spec.rb @@ -18,10 +18,10 @@ describe ScheduleBuildStageMigration, :migration do ## # CI/CD jobs # - jobs.create!(id: 10, commit_id: 1, project_id: 123, stage_id: nil) - jobs.create!(id: 20, commit_id: 1, project_id: 123, stage_id: nil) - jobs.create!(id: 30, commit_id: 1, project_id: 123, stage_id: nil) - jobs.create!(id: 40, commit_id: 1, project_id: 123, stage_id: 1) + jobs.create!(id: 11, commit_id: 1, project_id: 123, stage_id: nil) + jobs.create!(id: 206, commit_id: 1, project_id: 123, stage_id: nil) + jobs.create!(id: 3413, commit_id: 1, project_id: 123, stage_id: nil) + jobs.create!(id: 4109, commit_id: 1, project_id: 123, stage_id: 1) end before do @@ -33,9 +33,9 @@ describe ScheduleBuildStageMigration, :migration do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to be_scheduled_delayed_migration(1.minute, 10) - expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 20) - expect(described_class::MIGRATION).to be_scheduled_delayed_migration(3.minutes, 30) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(1.minute, 11) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 206) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(3.minutes, 3413) expect(BackgroundMigrationWorker.jobs.size).to eq 3 end end -- cgit v1.2.1 From c181683d99e4447c44a5963afe3c2680f0c82a9a Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Sat, 6 Jan 2018 16:07:55 +0100 Subject: Fix rubocop offense in build stage migration classes --- lib/gitlab/background_migration/migrate_build_stage.rb | 2 +- spec/migrations/schedule_build_stage_migration_spec.rb | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/gitlab/background_migration/migrate_build_stage.rb b/lib/gitlab/background_migration/migrate_build_stage.rb index 7139ac03737..6819810b145 100644 --- a/lib/gitlab/background_migration/migrate_build_stage.rb +++ b/lib/gitlab/background_migration/migrate_build_stage.rb @@ -40,7 +40,7 @@ module Gitlab # In that case we just don't migrate such stage. # def legacy? - @build.stage.nil? + @build.stage.nil? end def ensure! diff --git a/spec/migrations/schedule_build_stage_migration_spec.rb b/spec/migrations/schedule_build_stage_migration_spec.rb index 79f00fdbcf0..b475e1fe7de 100644 --- a/spec/migrations/schedule_build_stage_migration_spec.rb +++ b/spec/migrations/schedule_build_stage_migration_spec.rb @@ -8,6 +8,8 @@ describe ScheduleBuildStageMigration, :migration do let(:jobs) { table(:ci_builds) } before do + stub_const("#{described_class}::BATCH", 1) + ## # Dependencies # @@ -24,10 +26,6 @@ describe ScheduleBuildStageMigration, :migration do jobs.create!(id: 4109, commit_id: 1, project_id: 123, stage_id: 1) end - before do - stub_const("#{described_class}::BATCH", 1) - end - it 'schedules delayed background migrations in batches in bulk' do Sidekiq::Testing.fake! do Timecop.freeze do -- cgit v1.2.1 From acc70f7da1c21071a11bb07b86486c8e61799be8 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 12 Feb 2018 13:23:03 +0100 Subject: Move pipeline stages background migration in time --- .../20180105101928_schedule_build_stage_migration.rb | 20 -------------------- .../20180212101928_schedule_build_stage_migration.rb | 20 ++++++++++++++++++++ db/schema.rb | 2 +- .../schedule_build_stage_migration_spec.rb | 2 +- 4 files changed, 22 insertions(+), 22 deletions(-) delete mode 100644 db/post_migrate/20180105101928_schedule_build_stage_migration.rb create mode 100644 db/post_migrate/20180212101928_schedule_build_stage_migration.rb diff --git a/db/post_migrate/20180105101928_schedule_build_stage_migration.rb b/db/post_migrate/20180105101928_schedule_build_stage_migration.rb deleted file mode 100644 index b33127140bb..00000000000 --- a/db/post_migrate/20180105101928_schedule_build_stage_migration.rb +++ /dev/null @@ -1,20 +0,0 @@ -class ScheduleBuildStageMigration < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - MIGRATION = 'MigrateBuildStage'.freeze - BATCH = 1000 - - class Build < ActiveRecord::Base - include EachBatch - self.table_name = 'ci_builds' - end - - def change - Build.where('stage_id IS NULL').each_batch(of: BATCH) do |builds, index| - builds.pluck(:id).map { |id| [MIGRATION, [id]] }.tap do |migrations| - BackgroundMigrationWorker.bulk_perform_in(index.minutes, migrations) - end - end - end -end diff --git a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb new file mode 100644 index 00000000000..b33127140bb --- /dev/null +++ b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb @@ -0,0 +1,20 @@ +class ScheduleBuildStageMigration < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + MIGRATION = 'MigrateBuildStage'.freeze + BATCH = 1000 + + class Build < ActiveRecord::Base + include EachBatch + self.table_name = 'ci_builds' + end + + def change + Build.where('stage_id IS NULL').each_batch(of: BATCH) do |builds, index| + builds.pluck(:id).map { |id| [MIGRATION, [id]] }.tap do |migrations| + BackgroundMigrationWorker.bulk_perform_in(index.minutes, migrations) + end + end + end +end diff --git a/db/schema.rb b/db/schema.rb index d07a4c31618..e3474a4995f 100644 --- a/db/schema.rb +++ b/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: 20180206200543) do +ActiveRecord::Schema.define(version: 20180212101928) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/spec/migrations/schedule_build_stage_migration_spec.rb b/spec/migrations/schedule_build_stage_migration_spec.rb index b475e1fe7de..5e18d2e72f9 100644 --- a/spec/migrations/schedule_build_stage_migration_spec.rb +++ b/spec/migrations/schedule_build_stage_migration_spec.rb @@ -1,5 +1,5 @@ require 'spec_helper' -require Rails.root.join('db', 'post_migrate', '20180105101928_schedule_build_stage_migration') +require Rails.root.join('db', 'post_migrate', '20180212101928_schedule_build_stage_migration') describe ScheduleBuildStageMigration, :migration do let(:projects) { table(:projects) } -- cgit v1.2.1 From 8488c98e70561c6d7dde025131c15df4692b20b1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 13 Feb 2018 16:18:09 +0100 Subject: Schedule pipeline stages migration every 5 minutes [ci skip] --- .../20180212101928_schedule_build_stage_migration.rb | 18 +++++++++++++----- spec/migrations/schedule_build_stage_migration_spec.rb | 14 ++++---------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb index b33127140bb..f03a3f8d355 100644 --- a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb +++ b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb @@ -3,18 +3,26 @@ class ScheduleBuildStageMigration < ActiveRecord::Migration DOWNTIME = false MIGRATION = 'MigrateBuildStage'.freeze - BATCH = 1000 + BATCH_SIZE = 500 + + disable_ddl_transaction! class Build < ActiveRecord::Base include EachBatch self.table_name = 'ci_builds' end - def change - Build.where('stage_id IS NULL').each_batch(of: BATCH) do |builds, index| - builds.pluck(:id).map { |id| [MIGRATION, [id]] }.tap do |migrations| - BackgroundMigrationWorker.bulk_perform_in(index.minutes, migrations) + def up + disable_statement_timeout + + Build.where('stage_id IS NULL').each_batch(of: BATCH_SIZE) do |builds, index| + builds.pluck('MIN(id)', 'MAX(id)').first.tap do |range| + BackgroundMigrationWorker.perform_in(index * 5.minutes, MIGRATION, range) end end end + + def down + # noop + end end diff --git a/spec/migrations/schedule_build_stage_migration_spec.rb b/spec/migrations/schedule_build_stage_migration_spec.rb index 5e18d2e72f9..06657410173 100644 --- a/spec/migrations/schedule_build_stage_migration_spec.rb +++ b/spec/migrations/schedule_build_stage_migration_spec.rb @@ -8,18 +8,12 @@ describe ScheduleBuildStageMigration, :migration do let(:jobs) { table(:ci_builds) } before do - stub_const("#{described_class}::BATCH", 1) + stub_const("#{described_class}::BATCH_SIZE", 1) - ## - # Dependencies - # projects.create!(id: 123, name: 'gitlab', path: 'gitlab-ce') pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') stages.create!(id: 1, project_id: 123, pipeline_id: 1, name: 'test') - ## - # CI/CD jobs - # jobs.create!(id: 11, commit_id: 1, project_id: 123, stage_id: nil) jobs.create!(id: 206, commit_id: 1, project_id: 123, stage_id: nil) jobs.create!(id: 3413, commit_id: 1, project_id: 123, stage_id: nil) @@ -31,9 +25,9 @@ describe ScheduleBuildStageMigration, :migration do Timecop.freeze do migrate! - expect(described_class::MIGRATION).to be_scheduled_delayed_migration(1.minute, 11) - expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 206) - expect(described_class::MIGRATION).to be_scheduled_delayed_migration(3.minutes, 3413) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(5.minutes, 11, 11) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(10.minutes, 206, 206) + expect(described_class::MIGRATION).to be_scheduled_delayed_migration(15.minutes, 3413, 3413) expect(BackgroundMigrationWorker.jobs.size).to eq 3 end end -- cgit v1.2.1 From 378b2bad11b893fabebc083d000295c1ec499b23 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 14 Feb 2018 13:59:16 +0100 Subject: Migrate pipeline stages in batches instead of single row --- .../background_migration/migrate_build_stage.rb | 84 ++++++++-------------- .../migrate_build_stage_spec.rb | 12 +--- 2 files changed, 32 insertions(+), 64 deletions(-) diff --git a/lib/gitlab/background_migration/migrate_build_stage.rb b/lib/gitlab/background_migration/migrate_build_stage.rb index 6819810b145..3e297a90b19 100644 --- a/lib/gitlab/background_migration/migrate_build_stage.rb +++ b/lib/gitlab/background_migration/migrate_build_stage.rb @@ -5,69 +5,45 @@ module Gitlab module BackgroundMigration class MigrateBuildStage - def perform(id) - DatabaseBuild.find_by(id: id).try do |build| - MigratableStage.new(build).tap do |stage| - break if stage.exists? || stage.legacy? - - stage.ensure! - stage.migrate_reference! - stage.migrate_status! - end - end - end - - class DatabaseStage < ActiveRecord::Base - self.table_name = 'ci_stages' - end - - class DatabaseBuild < ActiveRecord::Base - self.table_name = 'ci_builds' - end - - class MigratableStage - def initialize(build) - @build = build - end - - def exists? - @build.reload.stage_id.present? + module Migratable + class Stage < ActiveRecord::Base + self.table_name = 'ci_stages' end - ## - # We can have some very old stages that do not have `ci_builds.stage` set. - # - # In that case we just don't migrate such stage. - # - def legacy? - @build.stage.nil? - end + class Build < ActiveRecord::Base + self.table_name = 'ci_builds' - def ensure! - find || create! - end + def ensure_stage! + find || create! + rescue ActiveRecord::RecordNotUnique + # TODO + end - def find - DatabaseStage.find_by(name: @build.stage, - pipeline_id: @build.commit_id, - project_id: @build.project_id) - end + def find + Stage.find_by(name: self.stage, + pipeline_id: self.commit_id, + project_id: self.project_id) + end - def create! - DatabaseStage.create!(name: @build.stage, - pipeline_id: @build.commit_id, - project_id: @build.project_id) + def create! + Stage.create!(name: self.stage || 'test', + pipeline_id: self.commit_id, + project_id: self.project_id) + end end + end - def migrate_reference! - MigrateBuildStageIdReference.new.perform(@build.id, @build.id) - end + def perform(start_id, stop_id) + # TODO, should we disable_statement_timeout? + # TODO, use plain SQL query? - def migrate_status! - raise ArgumentError unless exists? + stages = Migratable::Build.where('stage_id IS NULL') + .where("id BETWEEN #{start_id.to_i} AND #{stop_id.to_i}") + .map { |build| build.ensure_stage! } + .compact.map(&:id) - MigrateStageStatus.new.perform(@build.stage_id, @build.stage_id) - end + MigrateBuildStageIdReference.new.perform(start_id, stop_id) + MigrateStageStatus.new.perform(stages.min, stages.max) end end end diff --git a/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb b/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb index d20b5c29a71..27cb63848de 100644 --- a/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb +++ b/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Gitlab::BackgroundMigration::MigrateBuildStage, :migration, schema: 20180105101928 do +describe Gitlab::BackgroundMigration::MigrateBuildStage, :migration, schema: 20180212101928 do let(:projects) { table(:projects) } let(:pipelines) { table(:ci_pipelines) } let(:stages) { table(:ci_stages) } @@ -10,15 +10,9 @@ describe Gitlab::BackgroundMigration::MigrateBuildStage, :migration, schema: 201 failed: 4, canceled: 5, skipped: 6, manual: 7 }.freeze before do - ## - # Dependencies - # projects.create!(id: 123, name: 'gitlab', path: 'gitlab-ce') pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a') - ## - # CI/CD jobs - # jobs.create!(id: 1, commit_id: 1, project_id: 123, stage_idx: 2, stage: 'build', status: :success) jobs.create!(id: 2, commit_id: 1, project_id: 123, @@ -36,9 +30,7 @@ describe Gitlab::BackgroundMigration::MigrateBuildStage, :migration, schema: 201 it 'correctly migrates builds stages' do expect(stages.count).to be_zero - jobs.all.find_each do |job| - described_class.new.perform(job.id) - end + described_class.new.perform(1, 6) expect(stages.count).to eq 3 expect(stages.all.pluck(:name)).to match_array %w[test build deploy] -- cgit v1.2.1 From 6cb5b7c8729151c95d1610f0b2f7255fdac2bdec Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 15 Feb 2018 10:13:20 +0100 Subject: Recover from unique constraint violation in stages migration --- .../20180212101928_schedule_build_stage_migration.rb | 2 +- lib/gitlab/background_migration/migrate_build_stage.rb | 18 +++++++++--------- .../background_migration/migrate_build_stage_spec.rb | 11 +++++++++++ 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb index f03a3f8d355..ad65a6fb16e 100644 --- a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb +++ b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb @@ -3,7 +3,7 @@ class ScheduleBuildStageMigration < ActiveRecord::Migration DOWNTIME = false MIGRATION = 'MigrateBuildStage'.freeze - BATCH_SIZE = 500 + BATCH_SIZE = 800 disable_ddl_transaction! diff --git a/lib/gitlab/background_migration/migrate_build_stage.rb b/lib/gitlab/background_migration/migrate_build_stage.rb index 3e297a90b19..adca16751af 100644 --- a/lib/gitlab/background_migration/migrate_build_stage.rb +++ b/lib/gitlab/background_migration/migrate_build_stage.rb @@ -13,19 +13,20 @@ module Gitlab class Build < ActiveRecord::Base self.table_name = 'ci_builds' - def ensure_stage! - find || create! + def ensure_stage!(attempts: 2) + find_stage || create_stage! rescue ActiveRecord::RecordNotUnique - # TODO + retry if (attempts -= 1) > 0 + raise end - def find - Stage.find_by(name: self.stage, + def find_stage + Stage.find_by(name: self.stage || 'test', pipeline_id: self.commit_id, project_id: self.project_id) end - def create! + def create_stage! Stage.create!(name: self.stage || 'test', pipeline_id: self.commit_id, project_id: self.project_id) @@ -34,11 +35,10 @@ module Gitlab end def perform(start_id, stop_id) - # TODO, should we disable_statement_timeout? - # TODO, use plain SQL query? + # TODO, statement timeout? stages = Migratable::Build.where('stage_id IS NULL') - .where("id BETWEEN #{start_id.to_i} AND #{stop_id.to_i}") + .where('id BETWEEN ? AND ?', start_id, stop_id) .map { |build| build.ensure_stage! } .compact.map(&:id) diff --git a/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb b/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb index 27cb63848de..e112e9e9e3d 100644 --- a/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb +++ b/spec/lib/gitlab/background_migration/migrate_build_stage_spec.rb @@ -40,4 +40,15 @@ describe Gitlab::BackgroundMigration::MigrateBuildStage, :migration, schema: 201 STATUSES[:failed], STATUSES[:pending]] end + + it 'recovers from unique constraint violation only twice' do + allow(described_class::Migratable::Stage) + .to receive(:find_by).and_return(nil) + + expect(described_class::Migratable::Stage) + .to receive(:find_by).exactly(3).times + + expect { described_class.new.perform(1, 6) } + .to raise_error ActiveRecord::RecordNotUnique + end end -- cgit v1.2.1 From dfef5437a26320fc4a55fc857b6e59d5e92c13c3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Fri, 16 Feb 2018 12:28:34 +0100 Subject: Use a helper to schedule pipeline stages migration --- .../20180212101928_schedule_build_stage_migration.rb | 11 ++++++----- lib/gitlab/background_migration/migrate_build_stage.rb | 2 -- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb index ad65a6fb16e..df15b2cd9d4 100644 --- a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb +++ b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb @@ -3,7 +3,7 @@ class ScheduleBuildStageMigration < ActiveRecord::Migration DOWNTIME = false MIGRATION = 'MigrateBuildStage'.freeze - BATCH_SIZE = 800 + BATCH_SIZE = 500 disable_ddl_transaction! @@ -15,10 +15,11 @@ class ScheduleBuildStageMigration < ActiveRecord::Migration def up disable_statement_timeout - Build.where('stage_id IS NULL').each_batch(of: BATCH_SIZE) do |builds, index| - builds.pluck('MIN(id)', 'MAX(id)').first.tap do |range| - BackgroundMigrationWorker.perform_in(index * 5.minutes, MIGRATION, range) - end + Build.where('stage_id IS NULL').tap do |relation| + queue_background_migration_jobs_by_range_at_intervals(relation, + MIGRATION, + 5.minutes, + batch_size: BATCH_SIZE) end end diff --git a/lib/gitlab/background_migration/migrate_build_stage.rb b/lib/gitlab/background_migration/migrate_build_stage.rb index adca16751af..8fe4f1a2289 100644 --- a/lib/gitlab/background_migration/migrate_build_stage.rb +++ b/lib/gitlab/background_migration/migrate_build_stage.rb @@ -35,8 +35,6 @@ module Gitlab end def perform(start_id, stop_id) - # TODO, statement timeout? - stages = Migratable::Build.where('stage_id IS NULL') .where('id BETWEEN ? AND ?', start_id, stop_id) .map { |build| build.ensure_stage! } -- cgit v1.2.1 From 2d027056f9c56ad733bcccf54c3b7cf8c7cb95bf Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 20 Feb 2018 13:37:59 +0100 Subject: Add index before going through builds in a migration --- db/post_migrate/20180212101928_schedule_build_stage_migration.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb index df15b2cd9d4..f42a09f2223 100644 --- a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb +++ b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb @@ -15,12 +15,17 @@ class ScheduleBuildStageMigration < ActiveRecord::Migration def up disable_statement_timeout + add_concurrent_index(:ci_builds, :stage_id, where: 'stage_id IS NULL', + name: 'tmp_stage_id_partial_null_index') + Build.where('stage_id IS NULL').tap do |relation| queue_background_migration_jobs_by_range_at_intervals(relation, MIGRATION, 5.minutes, batch_size: BATCH_SIZE) end + + remove_concurrent_index_by_name(:ci_builds, 'tmp_stage_id_partial_null_index') end def down -- cgit v1.2.1 From 2f3f9a6bb0b4777146eb1ccc33ba14910cbd4746 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Tue, 20 Feb 2018 13:53:54 +0100 Subject: Separate adding and removing index from stages migration --- .../20180212101828_add_tmp_partial_null_index_to_builds.rb | 14 ++++++++++++++ .../20180212101928_schedule_build_stage_migration.rb | 5 ----- ...0212102028_remove_tmp_partial_null_index_from_builds.rb | 14 ++++++++++++++ db/schema.rb | 2 +- 4 files changed, 29 insertions(+), 6 deletions(-) create mode 100644 db/post_migrate/20180212101828_add_tmp_partial_null_index_to_builds.rb create mode 100644 db/post_migrate/20180212102028_remove_tmp_partial_null_index_from_builds.rb diff --git a/db/post_migrate/20180212101828_add_tmp_partial_null_index_to_builds.rb b/db/post_migrate/20180212101828_add_tmp_partial_null_index_to_builds.rb new file mode 100644 index 00000000000..df62ddf36c7 --- /dev/null +++ b/db/post_migrate/20180212101828_add_tmp_partial_null_index_to_builds.rb @@ -0,0 +1,14 @@ +class AddTmpPartialNullIndexToBuilds < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def up + add_concurrent_index(:ci_builds, :stage_id, where: 'stage_id IS NULL', + name: 'tmp_stage_id_partial_null_index') + end + + def down + remove_concurrent_index_by_name(:ci_builds, 'tmp_stage_id_partial_null_index') + end +end diff --git a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb index f42a09f2223..df15b2cd9d4 100644 --- a/db/post_migrate/20180212101928_schedule_build_stage_migration.rb +++ b/db/post_migrate/20180212101928_schedule_build_stage_migration.rb @@ -15,17 +15,12 @@ class ScheduleBuildStageMigration < ActiveRecord::Migration def up disable_statement_timeout - add_concurrent_index(:ci_builds, :stage_id, where: 'stage_id IS NULL', - name: 'tmp_stage_id_partial_null_index') - Build.where('stage_id IS NULL').tap do |relation| queue_background_migration_jobs_by_range_at_intervals(relation, MIGRATION, 5.minutes, batch_size: BATCH_SIZE) end - - remove_concurrent_index_by_name(:ci_builds, 'tmp_stage_id_partial_null_index') end def down diff --git a/db/post_migrate/20180212102028_remove_tmp_partial_null_index_from_builds.rb b/db/post_migrate/20180212102028_remove_tmp_partial_null_index_from_builds.rb new file mode 100644 index 00000000000..139097399a6 --- /dev/null +++ b/db/post_migrate/20180212102028_remove_tmp_partial_null_index_from_builds.rb @@ -0,0 +1,14 @@ +class RemoveTmpPartialNullIndexFromBuilds < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + disable_ddl_transaction! + + def up + remove_concurrent_index_by_name(:ci_builds, 'tmp_stage_id_partial_null_index') + end + + def down + add_concurrent_index(:ci_builds, :stage_id, where: 'stage_id IS NULL', + name: 'tmp_stage_id_partial_null_index') + end +end diff --git a/db/schema.rb b/db/schema.rb index b57094818e2..1f41b97041e 100644 --- a/db/schema.rb +++ b/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: 20180212101928) do +ActiveRecord::Schema.define(version: 20180212102028) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" -- cgit v1.2.1 From e79784b79a00507d9b16be52ea01465f6b0be7ff Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 21 Feb 2018 09:27:07 +0100 Subject: Create index on id instead of stage_id in migration --- .../20180212101828_add_tmp_partial_null_index_to_builds.rb | 6 +++--- .../20180212102028_remove_tmp_partial_null_index_from_builds.rb | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/db/post_migrate/20180212101828_add_tmp_partial_null_index_to_builds.rb b/db/post_migrate/20180212101828_add_tmp_partial_null_index_to_builds.rb index df62ddf36c7..e55e2e6f888 100644 --- a/db/post_migrate/20180212101828_add_tmp_partial_null_index_to_builds.rb +++ b/db/post_migrate/20180212101828_add_tmp_partial_null_index_to_builds.rb @@ -4,11 +4,11 @@ class AddTmpPartialNullIndexToBuilds < ActiveRecord::Migration disable_ddl_transaction! def up - add_concurrent_index(:ci_builds, :stage_id, where: 'stage_id IS NULL', - name: 'tmp_stage_id_partial_null_index') + add_concurrent_index(:ci_builds, :id, where: 'stage_id IS NULL', + name: 'tmp_id_partial_null_index') end def down - remove_concurrent_index_by_name(:ci_builds, 'tmp_stage_id_partial_null_index') + remove_concurrent_index_by_name(:ci_builds, 'tmp_id_partial_null_index') end end diff --git a/db/post_migrate/20180212102028_remove_tmp_partial_null_index_from_builds.rb b/db/post_migrate/20180212102028_remove_tmp_partial_null_index_from_builds.rb index 139097399a6..ed7b1fc72f4 100644 --- a/db/post_migrate/20180212102028_remove_tmp_partial_null_index_from_builds.rb +++ b/db/post_migrate/20180212102028_remove_tmp_partial_null_index_from_builds.rb @@ -4,11 +4,11 @@ class RemoveTmpPartialNullIndexFromBuilds < ActiveRecord::Migration disable_ddl_transaction! def up - remove_concurrent_index_by_name(:ci_builds, 'tmp_stage_id_partial_null_index') + remove_concurrent_index_by_name(:ci_builds, 'tmp_id_partial_null_index') end def down - add_concurrent_index(:ci_builds, :stage_id, where: 'stage_id IS NULL', - name: 'tmp_stage_id_partial_null_index') + add_concurrent_index(:ci_builds, :id, where: 'stage_id IS NULL', + name: 'tmp_id_partial_null_index') end end -- cgit v1.2.1 From a75cce1febf0403d66631841fff3bfbeefbfe6e3 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 21 Feb 2018 14:43:06 +0100 Subject: Add changelog entry for pipeline stages migration [ci skip] --- changelogs/unreleased/feature-gb-pipeline-variable-expressions.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/feature-gb-pipeline-variable-expressions.yml diff --git a/changelogs/unreleased/feature-gb-pipeline-variable-expressions.yml b/changelogs/unreleased/feature-gb-pipeline-variable-expressions.yml new file mode 100644 index 00000000000..28820649af3 --- /dev/null +++ b/changelogs/unreleased/feature-gb-pipeline-variable-expressions.yml @@ -0,0 +1,5 @@ +--- +title: Add catch-up background migration to migrate pipeline stages +merge_request: 15741 +author: +type: performance -- cgit v1.2.1 From 44fed816ee510c0aa56406c9221a0ffb2601d8f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Wed, 14 Feb 2018 23:09:18 +0000 Subject: Do not validate individual Variables when saving Project/Group --- app/models/group.rb | 2 +- app/models/project.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 75bf013ecd2..45160875dfb 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -26,7 +26,7 @@ class Group < Namespace has_many :shared_projects, through: :project_group_links, source: :project has_many :notification_settings, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent has_many :labels, class_name: 'GroupLabel' - has_many :variables, class_name: 'Ci::GroupVariable' + has_many :variables, class_name: 'Ci::GroupVariable', validate: false has_many :custom_attributes, class_name: 'GroupCustomAttribute' has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/models/project.rb b/app/models/project.rb index 4ad6f025e5c..fd09bb6a4d3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -210,7 +210,7 @@ class Project < ActiveRecord::Base has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName' has_many :runner_projects, class_name: 'Ci::RunnerProject' has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' - has_many :variables, class_name: 'Ci::Variable' + has_many :variables, class_name: 'Ci::Variable', validate: false has_many :triggers, class_name: 'Ci::Trigger' has_many :environments has_many :deployments -- cgit v1.2.1 From 4319b15a78be70ccabb31a25ffba37f77de27b28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 15 Feb 2018 13:59:11 +0100 Subject: Condition associated variable validation in Project and Group --- app/models/group.rb | 3 ++- app/models/project.rb | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index 45160875dfb..e7640d1c8ec 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -26,7 +26,7 @@ class Group < Namespace has_many :shared_projects, through: :project_group_links, source: :project has_many :notification_settings, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent has_many :labels, class_name: 'GroupLabel' - has_many :variables, class_name: 'Ci::GroupVariable', validate: false + has_many :variables, class_name: 'Ci::GroupVariable' has_many :custom_attributes, class_name: 'GroupCustomAttribute' has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -37,6 +37,7 @@ class Group < Namespace validate :visibility_level_allowed_by_sub_groups validate :visibility_level_allowed_by_parent validates :variables, variable_duplicates: true + validates_associated :variables, if: proc { |group| group.errors[:variables].nil? } validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } diff --git a/app/models/project.rb b/app/models/project.rb index fd09bb6a4d3..4ad6f025e5c 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -210,7 +210,7 @@ class Project < ActiveRecord::Base has_many :build_trace_section_names, class_name: 'Ci::BuildTraceSectionName' has_many :runner_projects, class_name: 'Ci::RunnerProject' has_many :runners, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' - has_many :variables, class_name: 'Ci::Variable', validate: false + has_many :variables, class_name: 'Ci::Variable' has_many :triggers, class_name: 'Ci::Trigger' has_many :environments has_many :deployments -- cgit v1.2.1 From c65529e8f66bf5367ad2d989a556bf766701d7b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Sun, 18 Feb 2018 21:54:55 +0100 Subject: Skip variables duplicates validator if variable is already a duplicate --- app/models/group.rb | 1 - app/validators/variable_duplicates_validator.rb | 2 ++ spec/support/features/variable_list_shared_examples.rb | 2 ++ 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/group.rb b/app/models/group.rb index e7640d1c8ec..75bf013ecd2 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -37,7 +37,6 @@ class Group < Namespace validate :visibility_level_allowed_by_sub_groups validate :visibility_level_allowed_by_parent validates :variables, variable_duplicates: true - validates_associated :variables, if: proc { |group| group.errors[:variables].nil? } validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } diff --git a/app/validators/variable_duplicates_validator.rb b/app/validators/variable_duplicates_validator.rb index 4bfa3c45303..72660be6c43 100644 --- a/app/validators/variable_duplicates_validator.rb +++ b/app/validators/variable_duplicates_validator.rb @@ -5,6 +5,8 @@ # - Use `validates :xxx, uniqueness: { scope: :xxx_id }` in a child model class VariableDuplicatesValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) + return if record.errors.include?(:"#{attribute}.key") + if options[:scope] scoped = value.group_by do |variable| Array(options[:scope]).map { |attr| variable.send(attr) } # rubocop:disable GitlabSecurity/PublicSend diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb index 0d8f7a7aae6..f7f851eb1eb 100644 --- a/spec/support/features/variable_list_shared_examples.rb +++ b/spec/support/features/variable_list_shared_examples.rb @@ -261,6 +261,8 @@ shared_examples 'variable list' do click_button('Save variables') wait_for_requests + expect(all('.js-ci-variable-list-section .js-ci-variable-error-box ul li').count).to eq(1) + # We check the first row because it re-sorts to alphabetical order on refresh page.within('.js-ci-variable-list-section') do expect(find('.js-ci-variable-error-box')).to have_content(/Validation failed Variables have duplicate values \(.+\)/) -- cgit v1.2.1 From c7dac22e52ceb6f47492af0cc57ce3f16894b90f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Thu, 22 Feb 2018 21:10:02 +0100 Subject: Add CHANGELOG entry --- changelogs/unreleased/43275-improve-variables-validation-message.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/43275-improve-variables-validation-message.yml diff --git a/changelogs/unreleased/43275-improve-variables-validation-message.yml b/changelogs/unreleased/43275-improve-variables-validation-message.yml new file mode 100644 index 00000000000..88ef93123a0 --- /dev/null +++ b/changelogs/unreleased/43275-improve-variables-validation-message.yml @@ -0,0 +1,5 @@ +--- +title: Remove duplicated error message on duplicate variable validation +merge_request: 17135 +author: +type: fixed -- cgit v1.2.1 From f2695aa84b68cd905eff90c598e452cbf63d98ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 23 Feb 2018 01:19:46 +0100 Subject: Include CI Variable Key in its uniqueness validation error message --- app/models/ci/group_variable.rb | 5 ++++- app/models/ci/variable.rb | 5 ++++- spec/models/ci/group_variable_spec.rb | 2 +- spec/models/ci/variable_spec.rb | 2 +- 4 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/models/ci/group_variable.rb b/app/models/ci/group_variable.rb index afeae69ba39..1dd0e050ba9 100644 --- a/app/models/ci/group_variable.rb +++ b/app/models/ci/group_variable.rb @@ -6,7 +6,10 @@ module Ci belongs_to :group - validates :key, uniqueness: { scope: :group_id } + validates :key, uniqueness: { + scope: :group_id, + message: "(%{value}) has already been taken" + } scope :unprotected, -> { where(protected: false) } end diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index 67d3ec81b6f..7c71291de84 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -6,7 +6,10 @@ module Ci belongs_to :project - validates :key, uniqueness: { scope: [:project_id, :environment_scope] } + validates :key, uniqueness: { + scope: [:project_id, :environment_scope], + message: "(%{value}) has already been taken" + } scope :unprotected, -> { where(protected: false) } end diff --git a/spec/models/ci/group_variable_spec.rb b/spec/models/ci/group_variable_spec.rb index 145189e7469..1b10501701c 100644 --- a/spec/models/ci/group_variable_spec.rb +++ b/spec/models/ci/group_variable_spec.rb @@ -5,7 +5,7 @@ describe Ci::GroupVariable do it { is_expected.to include_module(HasVariable) } it { is_expected.to include_module(Presentable) } - it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id) } + it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id).with_message(/\(\w+\) has already been taken/) } describe '.unprotected' do subject { described_class.unprotected } diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index e4ff551151e..875e8b2b682 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -6,7 +6,7 @@ describe Ci::Variable do describe 'validations' do it { is_expected.to include_module(HasVariable) } it { is_expected.to include_module(Presentable) } - it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope) } + it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope).with_message(/\(\w+\) has already been taken/) } end describe '.unprotected' do -- cgit v1.2.1 From 57719d34d3fcc15f39354b0e9dc1da41bbe5d1a8 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 22 Feb 2018 18:34:04 +0100 Subject: Expose ChatName objects to slash commands Instead of only exposing a User to slash commands we now also expose the ChatName object that the User object is retrieved from. This is necessary for GitLab Chatops as we need for example the user ID of the chat user. --- app/models/chat_name.rb | 21 ++++++++++++++++++ .../project_services/slash_commands_service.rb | 6 +++--- app/services/chat_names/find_user_service.rb | 4 ++-- lib/gitlab/slash_commands/base_command.rb | 7 +++--- lib/gitlab/slash_commands/command.rb | 5 +++-- spec/lib/gitlab/slash_commands/command_spec.rb | 5 +++-- spec/lib/gitlab/slash_commands/deploy_spec.rb | 3 ++- spec/lib/gitlab/slash_commands/issue_new_spec.rb | 3 ++- .../lib/gitlab/slash_commands/issue_search_spec.rb | 3 ++- spec/lib/gitlab/slash_commands/issue_show_spec.rb | 3 ++- spec/models/chat_name_spec.rb | 20 +++++++++++++++++ spec/services/chat_names/find_user_service_spec.rb | 25 +++++++++++++++------- 12 files changed, 81 insertions(+), 24 deletions(-) diff --git a/app/models/chat_name.rb b/app/models/chat_name.rb index f321db75eeb..fbd0f123341 100644 --- a/app/models/chat_name.rb +++ b/app/models/chat_name.rb @@ -1,4 +1,6 @@ class ChatName < ActiveRecord::Base + LAST_USED_AT_INTERVAL = 1.hour + belongs_to :service belongs_to :user @@ -9,4 +11,23 @@ class ChatName < ActiveRecord::Base validates :user_id, uniqueness: { scope: [:service_id] } validates :chat_id, uniqueness: { scope: [:service_id, :team_id] } + + # Updates the "last_used_timestamp" but only if it wasn't already updated + # recently. + # + # The throttling this method uses is put in place to ensure that high chat + # traffic doesn't result in many UPDATE queries being performed. + def update_last_used_at + return unless update_last_used_at? + + obtained = Gitlab::ExclusiveLease + .new("chat_name/last_used_at/#{id}", timeout: LAST_USED_AT_INTERVAL.to_i) + .try_obtain + + touch(:last_used_at) if obtained + end + + def update_last_used_at? + last_used_at.nil? || last_used_at > LAST_USED_AT_INTERVAL.ago + end end diff --git a/app/models/project_services/slash_commands_service.rb b/app/models/project_services/slash_commands_service.rb index eb4da68bb7e..37ea45109ae 100644 --- a/app/models/project_services/slash_commands_service.rb +++ b/app/models/project_services/slash_commands_service.rb @@ -30,10 +30,10 @@ class SlashCommandsService < Service def trigger(params) return unless valid_token?(params[:token]) - user = find_chat_user(params) + chat_user = find_chat_user(params) - if user - Gitlab::SlashCommands::Command.new(project, user, params).execute + if chat_user&.user + Gitlab::SlashCommands::Command.new(project, chat_user, params).execute else url = authorize_chat_name_url(params) Gitlab::SlashCommands::Presenters::Access.new(url).authorize diff --git a/app/services/chat_names/find_user_service.rb b/app/services/chat_names/find_user_service.rb index 4f5c5567b42..d458b814183 100644 --- a/app/services/chat_names/find_user_service.rb +++ b/app/services/chat_names/find_user_service.rb @@ -9,8 +9,8 @@ module ChatNames chat_name = find_chat_name return unless chat_name - chat_name.touch(:last_used_at) - chat_name.user + chat_name.update_last_used_at + chat_name end private diff --git a/lib/gitlab/slash_commands/base_command.rb b/lib/gitlab/slash_commands/base_command.rb index cc3c9a50555..466554e398c 100644 --- a/lib/gitlab/slash_commands/base_command.rb +++ b/lib/gitlab/slash_commands/base_command.rb @@ -31,10 +31,11 @@ module Gitlab raise NotImplementedError end - attr_accessor :project, :current_user, :params + attr_accessor :project, :current_user, :params, :chat_name - def initialize(project, user, params = {}) - @project, @current_user, @params = project, user, params.dup + def initialize(project, chat_name, params = {}) + @project, @current_user, @params = project, chat_name.user, params.dup + @chat_name = chat_name end private diff --git a/lib/gitlab/slash_commands/command.rb b/lib/gitlab/slash_commands/command.rb index a78408b0519..85aaa6b0eba 100644 --- a/lib/gitlab/slash_commands/command.rb +++ b/lib/gitlab/slash_commands/command.rb @@ -13,12 +13,13 @@ module Gitlab if command if command.allowed?(project, current_user) - command.new(project, current_user, params).execute(match) + command.new(project, chat_name, params).execute(match) else Gitlab::SlashCommands::Presenters::Access.new.access_denied end else - Gitlab::SlashCommands::Help.new(project, current_user, params).execute(available_commands, params[:text]) + Gitlab::SlashCommands::Help.new(project, chat_name, params) + .execute(available_commands, params[:text]) end end diff --git a/spec/lib/gitlab/slash_commands/command_spec.rb b/spec/lib/gitlab/slash_commands/command_spec.rb index 0173a45d480..e3447d974aa 100644 --- a/spec/lib/gitlab/slash_commands/command_spec.rb +++ b/spec/lib/gitlab/slash_commands/command_spec.rb @@ -3,10 +3,11 @@ require 'spec_helper' describe Gitlab::SlashCommands::Command do let(:project) { create(:project) } let(:user) { create(:user) } + let(:chat_name) { double(:chat_name, user: user) } describe '#execute' do subject do - described_class.new(project, user, params).execute + described_class.new(project, chat_name, params).execute end context 'when no command is available' do @@ -88,7 +89,7 @@ describe Gitlab::SlashCommands::Command do end describe '#match_command' do - subject { described_class.new(project, user, params).match_command.first } + subject { described_class.new(project, chat_name, params).match_command.first } context 'IssueShow is triggered' do let(:params) { { text: 'issue show 123' } } diff --git a/spec/lib/gitlab/slash_commands/deploy_spec.rb b/spec/lib/gitlab/slash_commands/deploy_spec.rb index 74b5ef4bb26..0d57334aa4c 100644 --- a/spec/lib/gitlab/slash_commands/deploy_spec.rb +++ b/spec/lib/gitlab/slash_commands/deploy_spec.rb @@ -4,6 +4,7 @@ describe Gitlab::SlashCommands::Deploy do describe '#execute' do let(:project) { create(:project) } let(:user) { create(:user) } + let(:chat_name) { double(:chat_name, user: user) } let(:regex_match) { described_class.match('deploy staging to production') } before do @@ -16,7 +17,7 @@ describe Gitlab::SlashCommands::Deploy do end subject do - described_class.new(project, user).execute(regex_match) + described_class.new(project, chat_name).execute(regex_match) end context 'if no environment is defined' do diff --git a/spec/lib/gitlab/slash_commands/issue_new_spec.rb b/spec/lib/gitlab/slash_commands/issue_new_spec.rb index 3b077c58c50..8e7df946529 100644 --- a/spec/lib/gitlab/slash_commands/issue_new_spec.rb +++ b/spec/lib/gitlab/slash_commands/issue_new_spec.rb @@ -4,6 +4,7 @@ describe Gitlab::SlashCommands::IssueNew do describe '#execute' do let(:project) { create(:project) } let(:user) { create(:user) } + let(:chat_name) { double(:chat_name, user: user) } let(:regex_match) { described_class.match("issue create bird is the word") } before do @@ -11,7 +12,7 @@ describe Gitlab::SlashCommands::IssueNew do end subject do - described_class.new(project, user).execute(regex_match) + described_class.new(project, chat_name).execute(regex_match) end context 'without description' do diff --git a/spec/lib/gitlab/slash_commands/issue_search_spec.rb b/spec/lib/gitlab/slash_commands/issue_search_spec.rb index 35d01efc1bd..189e9592f1b 100644 --- a/spec/lib/gitlab/slash_commands/issue_search_spec.rb +++ b/spec/lib/gitlab/slash_commands/issue_search_spec.rb @@ -6,10 +6,11 @@ describe Gitlab::SlashCommands::IssueSearch do let!(:confidential) { create(:issue, :confidential, project: project, title: 'mepmep find') } let(:project) { create(:project) } let(:user) { create(:user) } + let(:chat_name) { double(:chat_name, user: user) } let(:regex_match) { described_class.match("issue search find") } subject do - described_class.new(project, user).execute(regex_match) + described_class.new(project, chat_name).execute(regex_match) end context 'when the user has no access' do diff --git a/spec/lib/gitlab/slash_commands/issue_show_spec.rb b/spec/lib/gitlab/slash_commands/issue_show_spec.rb index e5834d5a2ee..b1db1638237 100644 --- a/spec/lib/gitlab/slash_commands/issue_show_spec.rb +++ b/spec/lib/gitlab/slash_commands/issue_show_spec.rb @@ -5,6 +5,7 @@ describe Gitlab::SlashCommands::IssueShow do let(:issue) { create(:issue, project: project) } let(:project) { create(:project) } let(:user) { issue.author } + let(:chat_name) { double(:chat_name, user: user) } let(:regex_match) { described_class.match("issue show #{issue.iid}") } before do @@ -12,7 +13,7 @@ describe Gitlab::SlashCommands::IssueShow do end subject do - described_class.new(project, user).execute(regex_match) + described_class.new(project, chat_name).execute(regex_match) end context 'the issue exists' do diff --git a/spec/models/chat_name_spec.rb b/spec/models/chat_name_spec.rb index e89e534d914..504bc710b25 100644 --- a/spec/models/chat_name_spec.rb +++ b/spec/models/chat_name_spec.rb @@ -14,4 +14,24 @@ describe ChatName do it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(:service_id) } it { is_expected.to validate_uniqueness_of(:chat_id).scoped_to(:service_id, :team_id) } + + describe '#update_last_used_at', :clean_gitlab_redis_shared_state do + it 'updates the last_used_at timestamp' do + expect(subject.last_used_at).to be_nil + + subject.update_last_used_at + + expect(subject.last_used_at).to be_present + end + + it 'does not update last_used_at if it was recently updated' do + subject.update_last_used_at + + time = subject.last_used_at + + subject.update_last_used_at + + expect(subject.last_used_at).to eq(time) + end + end end diff --git a/spec/services/chat_names/find_user_service_spec.rb b/spec/services/chat_names/find_user_service_spec.rb index 79aaac3aeb6..5734b10109a 100644 --- a/spec/services/chat_names/find_user_service_spec.rb +++ b/spec/services/chat_names/find_user_service_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe ChatNames::FindUserService do +describe ChatNames::FindUserService, :clean_gitlab_redis_shared_state do describe '#execute' do let(:service) { create(:service) } @@ -13,21 +13,30 @@ describe ChatNames::FindUserService do context 'when existing user is requested' do let(:params) { { team_id: chat_name.team_id, user_id: chat_name.chat_id } } - it 'returns the existing user' do - is_expected.to eq(user) + it 'returns the existing chat_name' do + is_expected.to eq(chat_name) end - it 'updates when last time chat name was used' do + it 'updates the last used timestamp if one is not already set' do expect(chat_name.last_used_at).to be_nil subject - initial_last_used = chat_name.reload.last_used_at - expect(initial_last_used).to be_present + expect(chat_name.reload.last_used_at).to be_present + end + + it 'only updates an existing timestamp once within a certain time frame' do + service = described_class.new(service, params) + + expect(chat_name.last_used_at).to be_nil + + service.execute + + time = chat_name.reload.last_used_at - Timecop.travel(2.days.from_now) { described_class.new(service, params).execute } + service.execute - expect(chat_name.reload.last_used_at).to be > initial_last_used + expect(chat_name.reload.last_used_at).to eq(time) end end -- cgit v1.2.1 From fffc1317959940bd12f6fec6fb9f095dfc48c994 Mon Sep 17 00:00:00 2001 From: Constance Okoghenun Date: Fri, 23 Feb 2018 17:18:24 +0100 Subject: removed profile webpack bundle tag --- app/assets/javascripts/dispatcher.js | 5 - app/assets/javascripts/pages/profiles/index.js | 19 +++ .../javascripts/pages/profiles/index/index.js | 4 +- app/assets/javascripts/profile/profile.js | 150 +++++++++------------ app/assets/javascripts/profile/profile_bundle.js | 2 - app/views/profiles/_head.html.haml | 2 - app/views/profiles/accounts/show.html.haml | 1 - app/views/profiles/audit_log.html.haml | 1 - app/views/profiles/chat_names/index.html.haml | 1 - app/views/profiles/emails/index.html.haml | 1 - app/views/profiles/gpg_keys/index.html.haml | 1 - app/views/profiles/keys/index.html.haml | 1 - app/views/profiles/keys/show.html.haml | 1 - app/views/profiles/notifications/show.html.haml | 1 - .../personal_access_tokens/index.html.haml | 1 - app/views/profiles/preferences/show.html.haml | 1 - app/views/profiles/show.html.haml | 1 - app/views/profiles/two_factor_auths/show.html.haml | 1 - config/webpack.config.js | 1 - 19 files changed, 87 insertions(+), 108 deletions(-) create mode 100644 app/assets/javascripts/pages/profiles/index.js delete mode 100644 app/assets/javascripts/profile/profile_bundle.js delete mode 100644 app/views/profiles/_head.html.haml diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index f66ce1c083b..3f14b8340e4 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -114,11 +114,6 @@ var Dispatcher; break; } break; - case 'profiles': - import('./pages/profiles/index') - .then(callDefault) - .catch(fail); - break; case 'projects': import('./pages/projects') .then(callDefault) diff --git a/app/assets/javascripts/pages/profiles/index.js b/app/assets/javascripts/pages/profiles/index.js new file mode 100644 index 00000000000..5d634915d7a --- /dev/null +++ b/app/assets/javascripts/pages/profiles/index.js @@ -0,0 +1,19 @@ +import '~/profile/gl_crop'; +import Profile from '~/profile/profile'; +import { getPagePath } from '~/lib/utils/common_utils'; + +document.addEventListener('DOMContentLoaded', () => { + $(document).on('input.ssh_key', '#key_key', () => { + const $title = $('#key_title'); + const comment = $(this).val().match(/^\S+ \S+ (.+)\n?$/); + + // Extract the SSH Key title from its comment + if (comment && comment.length > 1) { + $title.val(comment[1]).change(); + } + }); + + if (getPagePath() === 'profiles') { + new Profile(); // eslint-disable-line no-new + } +}); diff --git a/app/assets/javascripts/pages/profiles/index/index.js b/app/assets/javascripts/pages/profiles/index/index.js index 90eed38777a..9bd430f4f11 100644 --- a/app/assets/javascripts/pages/profiles/index/index.js +++ b/app/assets/javascripts/pages/profiles/index/index.js @@ -1,7 +1,7 @@ import NotificationsForm from '../../../notifications_form'; import notificationsDropdown from '../../../notifications_dropdown'; -export default () => { +document.addEventListener('DOMContentLoaded', () => { new NotificationsForm(); // eslint-disable-line no-new notificationsDropdown(); -}; +}); diff --git a/app/assets/javascripts/profile/profile.js b/app/assets/javascripts/profile/profile.js index 930f0fb381e..a811781853b 100644 --- a/app/assets/javascripts/profile/profile.js +++ b/app/assets/javascripts/profile/profile.js @@ -1,103 +1,85 @@ /* eslint-disable comma-dangle, no-unused-vars, class-methods-use-this, quotes, consistent-return, func-names, prefer-arrow-callback, space-before-function-paren, max-len */ import Cookies from 'js-cookie'; -import { getPagePath } from '~/lib/utils/common_utils'; import axios from '~/lib/utils/axios_utils'; import { __ } from '~/locale'; import flash from '../flash'; -((global) => { - class Profile { - constructor({ form } = {}) { - this.onSubmitForm = this.onSubmitForm.bind(this); - this.form = form || $('.edit-user'); - this.newRepoActivated = Cookies.get('new_repo'); - this.setRepoRadio(); - this.bindEvents(); - this.initAvatarGlCrop(); - } - - initAvatarGlCrop() { - const cropOpts = { - filename: '.js-avatar-filename', - previewImage: '.avatar-image .avatar', - modalCrop: '.modal-profile-crop', - pickImageEl: '.js-choose-user-avatar-button', - uploadImageBtn: '.js-upload-user-avatar', - modalCropImg: '.modal-profile-crop-image' - }; - this.avatarGlCrop = $('.js-user-avatar-input').glCrop(cropOpts).data('glcrop'); - } +export default class Profile { + constructor({ form } = {}) { + this.onSubmitForm = this.onSubmitForm.bind(this); + this.form = form || $('.edit-user'); + this.newRepoActivated = Cookies.get('new_repo'); + this.setRepoRadio(); + this.bindEvents(); + this.initAvatarGlCrop(); + } - bindEvents() { - $('.js-preferences-form').on('change.preference', 'input[type=radio]', this.submitForm); - $('input[name="user[multi_file]"]').on('change', this.setNewRepoCookie); - $('#user_notification_email').on('change', this.submitForm); - $('#user_notified_of_own_activity').on('change', this.submitForm); - this.form.on('submit', this.onSubmitForm); - } + initAvatarGlCrop() { + const cropOpts = { + filename: '.js-avatar-filename', + previewImage: '.avatar-image .avatar', + modalCrop: '.modal-profile-crop', + pickImageEl: '.js-choose-user-avatar-button', + uploadImageBtn: '.js-upload-user-avatar', + modalCropImg: '.modal-profile-crop-image' + }; + this.avatarGlCrop = $('.js-user-avatar-input').glCrop(cropOpts).data('glcrop'); + } - submitForm() { - return $(this).parents('form').submit(); - } + bindEvents() { + $('.js-preferences-form').on('change.preference', 'input[type=radio]', this.submitForm); + $('input[name="user[multi_file]"]').on('change', this.setNewRepoCookie); + $('#user_notification_email').on('change', this.submitForm); + $('#user_notified_of_own_activity').on('change', this.submitForm); + this.form.on('submit', this.onSubmitForm); + } - onSubmitForm(e) { - e.preventDefault(); - return this.saveForm(); - } + submitForm() { + return $(this).parents('form').submit(); + } - saveForm() { - const self = this; - const formData = new FormData(this.form[0]); - const avatarBlob = this.avatarGlCrop.getBlob(); + onSubmitForm(e) { + e.preventDefault(); + return this.saveForm(); + } - if (avatarBlob != null) { - formData.append('user[avatar]', avatarBlob, 'avatar.png'); - } + saveForm() { + const self = this; + const formData = new FormData(this.form[0]); + const avatarBlob = this.avatarGlCrop.getBlob(); - axios({ - method: this.form.attr('method'), - url: this.form.attr('action'), - data: formData, - }) - .then(({ data }) => flash(data.message, 'notice')) - .then(() => { - window.scrollTo(0, 0); - // Enable submit button after requests ends - self.form.find(':input[disabled]').enable(); - }) - .catch(error => flash(error.message)); + if (avatarBlob != null) { + formData.append('user[avatar]', avatarBlob, 'avatar.png'); } - setNewRepoCookie() { - if (this.value === 'off') { - Cookies.remove('new_repo'); - } else { - Cookies.set('new_repo', true, { expires_in: 365 }); - } - } + axios({ + method: this.form.attr('method'), + url: this.form.attr('action'), + data: formData, + }) + .then(({ data }) => flash(data.message, 'notice')) + .then(() => { + window.scrollTo(0, 0); + // Enable submit button after requests ends + self.form.find(':input[disabled]').enable(); + }) + .catch(error => flash(error.message)); + } - setRepoRadio() { - const multiEditRadios = $('input[name="user[multi_file]"]'); - if (this.newRepoActivated || this.newRepoActivated === 'true') { - multiEditRadios.filter('[value=on]').prop('checked', true); - } else { - multiEditRadios.filter('[value=off]').prop('checked', true); - } + setNewRepoCookie() { + if (this.value === 'off') { + Cookies.remove('new_repo'); + } else { + Cookies.set('new_repo', true, { expires_in: 365 }); } } - $(function() { - $(document).on('input.ssh_key', '#key_key', function() { - const $title = $('#key_title'); - const comment = $(this).val().match(/^\S+ \S+ (.+)\n?$/); - - // Extract the SSH Key title from its comment - if (comment && comment.length > 1) { - return $title.val(comment[1]).change(); - } - }); - if (getPagePath() === 'profiles') { - return new Profile(); + setRepoRadio() { + const multiEditRadios = $('input[name="user[multi_file]"]'); + if (this.newRepoActivated || this.newRepoActivated === 'true') { + multiEditRadios.filter('[value=on]').prop('checked', true); + } else { + multiEditRadios.filter('[value=off]').prop('checked', true); } - }); -})(window.gl || (window.gl = {})); + } +} diff --git a/app/assets/javascripts/profile/profile_bundle.js b/app/assets/javascripts/profile/profile_bundle.js deleted file mode 100644 index ff35a9bcb83..00000000000 --- a/app/assets/javascripts/profile/profile_bundle.js +++ /dev/null @@ -1,2 +0,0 @@ -import './gl_crop'; -import './profile'; diff --git a/app/views/profiles/_head.html.haml b/app/views/profiles/_head.html.haml deleted file mode 100644 index a8eb66ca13c..00000000000 --- a/app/views/profiles/_head.html.haml +++ /dev/null @@ -1,2 +0,0 @@ -- content_for :page_specific_javascripts do - = webpack_bundle_tag('profile') diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index 0f849f6f8b7..02263095599 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -1,6 +1,5 @@ - page_title "Account" - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' - if current_user.ldap_user? .alert.alert-info diff --git a/app/views/profiles/audit_log.html.haml b/app/views/profiles/audit_log.html.haml index cbea5ca605a..a924369050b 100644 --- a/app/views/profiles/audit_log.html.haml +++ b/app/views/profiles/audit_log.html.haml @@ -1,6 +1,5 @@ - page_title "Authentication log" - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' .row.prepend-top-default .col-lg-4.profile-settings-sidebar diff --git a/app/views/profiles/chat_names/index.html.haml b/app/views/profiles/chat_names/index.html.haml index 8f7121afe02..4b6e419af50 100644 --- a/app/views/profiles/chat_names/index.html.haml +++ b/app/views/profiles/chat_names/index.html.haml @@ -1,6 +1,5 @@ - page_title 'Chat' - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' .row.prepend-top-default .col-lg-4.profile-settings-sidebar diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml index df1df4f5d72..e3c2bd1150e 100644 --- a/app/views/profiles/emails/index.html.haml +++ b/app/views/profiles/emails/index.html.haml @@ -1,6 +1,5 @@ - page_title "Emails" - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' .row.prepend-top-default .col-lg-4.profile-settings-sidebar diff --git a/app/views/profiles/gpg_keys/index.html.haml b/app/views/profiles/gpg_keys/index.html.haml index e44506ec9c9..1d2e41cb437 100644 --- a/app/views/profiles/gpg_keys/index.html.haml +++ b/app/views/profiles/gpg_keys/index.html.haml @@ -1,6 +1,5 @@ - page_title "GPG Keys" - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' .row.prepend-top-default .col-lg-4.profile-settings-sidebar diff --git a/app/views/profiles/keys/index.html.haml b/app/views/profiles/keys/index.html.haml index 5f7b41cf30e..457583cfd35 100644 --- a/app/views/profiles/keys/index.html.haml +++ b/app/views/profiles/keys/index.html.haml @@ -1,6 +1,5 @@ - page_title "SSH Keys" - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' .row.prepend-top-default .col-lg-4.profile-settings-sidebar diff --git a/app/views/profiles/keys/show.html.haml b/app/views/profiles/keys/show.html.haml index 7b7960708c4..28be6172219 100644 --- a/app/views/profiles/keys/show.html.haml +++ b/app/views/profiles/keys/show.html.haml @@ -2,5 +2,4 @@ - breadcrumb_title @key.title - page_title @key.title, "SSH Keys" - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' = render "key_details" diff --git a/app/views/profiles/notifications/show.html.haml b/app/views/profiles/notifications/show.html.haml index 202eccb7bb6..8f099aa6dd7 100644 --- a/app/views/profiles/notifications/show.html.haml +++ b/app/views/profiles/notifications/show.html.haml @@ -1,6 +1,5 @@ - page_title "Notifications" - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' %div - if @user.errors.any? diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml index f445e5a2417..78848542810 100644 --- a/app/views/profiles/personal_access_tokens/index.html.haml +++ b/app/views/profiles/personal_access_tokens/index.html.haml @@ -2,7 +2,6 @@ - page_title "Personal Access Tokens" - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' .row.prepend-top-default .col-lg-4.profile-settings-sidebar diff --git a/app/views/profiles/preferences/show.html.haml b/app/views/profiles/preferences/show.html.haml index 66d1d1e8d44..6aefd97bb96 100644 --- a/app/views/profiles/preferences/show.html.haml +++ b/app/views/profiles/preferences/show.html.haml @@ -1,6 +1,5 @@ - page_title 'Preferences' - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' = form_for @user, url: profile_preferences_path, remote: true, method: :put, html: { class: 'row prepend-top-default js-preferences-form' } do |f| .col-lg-4.application-theme diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml index 110736dc557..e497eab32e0 100644 --- a/app/views/profiles/show.html.haml +++ b/app/views/profiles/show.html.haml @@ -1,6 +1,5 @@ - breadcrumb_title "Edit Profile" - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' = bootstrap_form_for @user, url: profile_path, method: :put, html: { multipart: true, class: 'edit-user prepend-top-default js-quick-submit' }, authenticity_token: true do |f| = form_errors(@user) diff --git a/app/views/profiles/two_factor_auths/show.html.haml b/app/views/profiles/two_factor_auths/show.html.haml index e58cd20402c..8707af36e2e 100644 --- a/app/views/profiles/two_factor_auths/show.html.haml +++ b/app/views/profiles/two_factor_auths/show.html.haml @@ -2,7 +2,6 @@ - add_to_breadcrumbs("Two-Factor Authentication", profile_account_path) - @content_class = "limit-container-width" unless fluid_layout -= render 'profiles/head' - content_for :page_specific_javascripts do - if inject_u2f_api? diff --git a/config/webpack.config.js b/config/webpack.config.js index 94ff39485fb..ad0c8ee51c5 100644 --- a/config/webpack.config.js +++ b/config/webpack.config.js @@ -73,7 +73,6 @@ var config = { pdf_viewer: './blob/pdf_viewer.js', pipelines: './pipelines/pipelines_bundle.js', pipelines_details: './pipelines/pipeline_details_bundle.js', - profile: './profile/profile_bundle.js', project_import_gl: './projects/project_import_gitlab_project.js', protected_branches: './protected_branches', protected_tags: './protected_tags', -- cgit v1.2.1 From ebc2829added6a1ce61070da5f137d87472c5b12 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 23 Feb 2018 17:15:53 +0000 Subject: Fix gpg popover box --- .../tree/components/commit_pipeline_status_component.vue | 2 +- app/assets/stylesheets/pages/commits.scss | 10 +++++----- changelogs/unreleased/43315-gpg-popover.yml | 5 +++++ 3 files changed, 11 insertions(+), 6 deletions(-) create mode 100644 changelogs/unreleased/43315-gpg-popover.yml diff --git a/app/assets/javascripts/projects/tree/components/commit_pipeline_status_component.vue b/app/assets/javascripts/projects/tree/components/commit_pipeline_status_component.vue index 63f20a0041d..c53ce0549f5 100644 --- a/app/assets/javascripts/projects/tree/components/commit_pipeline_status_component.vue +++ b/app/assets/javascripts/projects/tree/components/commit_pipeline_status_component.vue @@ -98,7 +98,7 @@ };