diff options
7 files changed, 202 insertions, 1 deletions
diff --git a/app/models/release.rb b/app/models/release.rb index 5ef3ff1bc6c..b770f3934ef 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -26,12 +26,13 @@ class Release < ApplicationRecord before_create :set_released_at validates :project, :tag, presence: true + validates :author_id, presence: true, if: :validate_release_with_author? + validates :tag, uniqueness: { scope: :project_id } validates :description, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT }, if: :description_changed? validates_associated :milestone_releases, message: -> (_, obj) { obj[:value].map(&:errors).map(&:full_messages).join(",") } validates :links, nested_attributes_duplicates: { scope: :release, child_attributes: %i[name url filepath] } - validates :author_id, presence: true, on: :create, if: :validate_release_with_author? scope :sorted, -> { order(released_at: :desc) } scope :preloaded, -> { diff --git a/db/post_migrate/20221215151822_schedule_backfill_releases_author_id.rb b/db/post_migrate/20221215151822_schedule_backfill_releases_author_id.rb new file mode 100644 index 00000000000..4d8343ca2dd --- /dev/null +++ b/db/post_migrate/20221215151822_schedule_backfill_releases_author_id.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +class ScheduleBackfillReleasesAuthorId < Gitlab::Database::Migration[2.1] + MIGRATION = 'BackfillReleasesAuthorId' + JOB_DELAY_INTERVAL = 2.minutes + GHOST_USER_TYPE = 5 + + restrict_gitlab_migration gitlab_schema: :gitlab_main + + class User < MigrationRecord + self.table_name = 'users' + end + + class Release < MigrationRecord + self.table_name = 'releases' + end + + def up + unless release_with_empty_author_exists? + say "There are no releases with empty author_id, so skipping migration #{self.class.name}" + return + end + + create_ghost_user if ghost_user_id.nil? + + queue_batched_background_migration( + MIGRATION, + :releases, + :id, + ghost_user_id, + job_interval: JOB_DELAY_INTERVAL + ) + end + + def down + delete_batched_background_migration(MIGRATION, :releases, :id, [ghost_user_id]) + end + + private + + def ghost_user_id + User.find_by(user_type: GHOST_USER_TYPE)&.id + end + + def create_ghost_user + user = User.new + user.name = 'Ghost User' + user.username = 'ghost' + user.email = 'ghost@example.com' + user.user_type = GHOST_USER_TYPE + user.projects_limit = 100000 + + user.save! + end + + def release_with_empty_author_exists? + Release.exists?(author_id: nil) + end +end diff --git a/db/schema_migrations/20221215151822 b/db/schema_migrations/20221215151822 new file mode 100644 index 00000000000..bef6ccd1711 --- /dev/null +++ b/db/schema_migrations/20221215151822 @@ -0,0 +1 @@ +6d5872c6c5e0a7bc9bd52eeac7cbbd49bbe41210dd5596078acf088ac8eec1bd
\ No newline at end of file diff --git a/lib/gitlab/background_migration/backfill_releases_author_id.rb b/lib/gitlab/background_migration/backfill_releases_author_id.rb new file mode 100644 index 00000000000..62af0b42e5d --- /dev/null +++ b/lib/gitlab/background_migration/backfill_releases_author_id.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Gitlab + module BackgroundMigration + # Backfills releases with empty release authors. + # More details on: + # 1) https://gitlab.com/groups/gitlab-org/-/epics/8375 + # 2) https://gitlab.com/gitlab-org/gitlab/-/issues/367522#note_1156503600 + class BackfillReleasesAuthorId < BatchedMigrationJob + operation_name :backfill_releases_author_id + job_arguments :ghost_user_id + + scope_to ->(relation) { relation.where(author_id: nil) } + + def perform + each_sub_batch do |sub_batch| + sub_batch.update_all(author_id: ghost_user_id) + end + end + end + end +end diff --git a/spec/lib/gitlab/background_migration/backfill_releases_author_id_spec.rb b/spec/lib/gitlab/background_migration/backfill_releases_author_id_spec.rb new file mode 100644 index 00000000000..d8ad10849f2 --- /dev/null +++ b/spec/lib/gitlab/background_migration/backfill_releases_author_id_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::BackgroundMigration::BackfillReleasesAuthorId, + :migration, schema: 20221215151822, feature_category: :release_orchestration do + let(:releases_table) { table(:releases) } + let(:user_table) { table(:users) } + let(:date_time) { DateTime.now } + + let!(:test_user) { user_table.create!(name: 'test', email: 'test@example.com', username: 'test', projects_limit: 10) } + let!(:ghost_user) do + user_table.create!(name: 'ghost', email: 'ghost@example.com', + username: 'ghost', user_type: User::USER_TYPES['ghost'], projects_limit: 100000) + end + + let(:migration) do + described_class.new(start_id: 1, end_id: 100, + batch_table: :releases, batch_column: :id, + sub_batch_size: 10, pause_ms: 0, + job_arguments: [ghost_user.id], + connection: ApplicationRecord.connection) + end + + subject(:perform_migration) { migration.perform } + + before do + releases_table.create!(tag: 'tag1', name: 'tag1', + released_at: (date_time - 1.minute), author_id: test_user.id) + releases_table.create!(tag: 'tag2', name: 'tag2', + released_at: (date_time - 2.minutes), author_id: test_user.id) + releases_table.new(tag: 'tag3', name: 'tag3', + released_at: (date_time - 3.minutes), author_id: nil).save!(validate: false) + releases_table.new(tag: 'tag4', name: 'tag4', + released_at: (date_time - 4.minutes), author_id: nil).save!(validate: false) + releases_table.new(tag: 'tag5', name: 'tag5', + released_at: (date_time - 5.minutes), author_id: nil).save!(validate: false) + releases_table.create!(tag: 'tag6', name: 'tag6', + released_at: (date_time - 6.minutes), author_id: test_user.id) + releases_table.new(tag: 'tag7', name: 'tag7', + released_at: (date_time - 7.minutes), author_id: nil).save!(validate: false) + end + + it 'backfills `author_id` for the selected records', :aggregate_failures do + expect(releases_table.where(author_id: ghost_user.id).count).to eq 0 + expect(releases_table.where(author_id: nil).count).to eq 4 + + perform_migration + + expect(releases_table.where(author_id: ghost_user.id).count).to eq 4 + expect(releases_table.where(author_id: ghost_user.id).pluck(:name)).to include('tag3', 'tag4', 'tag5', 'tag7') + expect(releases_table.where(author_id: test_user.id).count).to eq 3 + expect(releases_table.where(author_id: test_user.id).pluck(:name)).to include('tag1', 'tag2', 'tag6') + end + + it 'tracks timings of queries' do + expect(migration.batch_metrics.timings).to be_empty + + expect { perform_migration }.to change { migration.batch_metrics.timings } + end +end diff --git a/spec/migrations/20221215151822_schedule_backfill_releases_author_id_spec.rb b/spec/migrations/20221215151822_schedule_backfill_releases_author_id_spec.rb new file mode 100644 index 00000000000..d7aa53ec35b --- /dev/null +++ b/spec/migrations/20221215151822_schedule_backfill_releases_author_id_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration! + +RSpec.describe ScheduleBackfillReleasesAuthorId, feature_category: :release_orchestration do + context 'when there are releases without author' do + let(:releases_table) { table(:releases) } + let(:user_table) { table(:users) } + let(:date_time) { DateTime.now } + let!(:batched_migration) { described_class::MIGRATION } + let!(:test_user) do + user_table.create!(name: 'test', + email: 'test@example.com', + username: 'test', + projects_limit: 10) + end + + before do + releases_table.create!(tag: 'tag1', name: 'tag1', + released_at: (date_time - 1.minute), author_id: test_user.id) + releases_table.create!(tag: 'tag2', name: 'tag2', + released_at: (date_time - 2.minutes), author_id: test_user.id) + releases_table.new(tag: 'tag3', name: 'tag3', + released_at: (date_time - 3.minutes), author_id: nil).save!(validate: false) + releases_table.new(tag: 'tag4', name: 'tag4', + released_at: (date_time - 4.minutes), author_id: nil).save!(validate: false) + end + + it 'schedules a new batched migration' do + reversible_migration do |migration| + migration.before -> { + expect(batched_migration).not_to have_scheduled_batched_migration + } + + migration.after -> { + expect(batched_migration).to have_scheduled_batched_migration( + table_name: :releases, + column_name: :id, + interval: described_class::JOB_DELAY_INTERVAL, + job_arguments: [User.find_by(user_type: :ghost)&.id] + ) + } + end + end + end + + context 'when there are no releases without author' do + it 'does not schedule batched migration' do + expect(described_class.new.up).not_to have_scheduled_batched_migration + end + end +end diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb index 180a76ff593..5ed4eb7d233 100644 --- a/spec/models/release_spec.rb +++ b/spec/models/release_spec.rb @@ -86,6 +86,10 @@ RSpec.describe Release do context 'when updating existing release without author' do let(:release) { create(:release, :legacy) } + before do + stub_feature_flags(validate_release_with_author: false) + end + it 'updates successfully' do release.description += 'Update' |