From 21f7559d1c66725b6472ea085552dac781d66434 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Thu, 24 Oct 2024 10:59:53 +1100
Subject: [PATCH] Remove Danger warning for finalizing migration required stop
Since https://gitlab.com/gitlab-org/gitlab/-/merge_requests/165146 we
now have a CI job that enforces a required stop has passed before we can
finalize a background migration.
Additionally these danger warnings are quite noisy. For example all the
discussions I had to resolve in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/169518 .
This note does also mention "Check the timings" and theoretically there
may be a case to wait for 2 required stops for any migration that is
very slow but I don't believe we do this in practice. For now I think
we've settled on the following requirements before finalizing:
1. It is finished on GitLab.com
2. We've passed a required stop
And we already have adequate enforcement for these so I don't think this
warning is really helping us anymore.
---
danger/plugins/required_stops.rb | 10 ---
danger/required_stops/Dangerfile | 3 -
spec/tooling/danger/required_stops_spec.rb | 96 ----------------------
tooling/danger/required_stops.rb | 40 ---------
4 files changed, 149 deletions(-)
delete mode 100644 danger/plugins/required_stops.rb
delete mode 100644 danger/required_stops/Dangerfile
delete mode 100644 spec/tooling/danger/required_stops_spec.rb
delete mode 100644 tooling/danger/required_stops.rb
diff --git a/danger/plugins/required_stops.rb b/danger/plugins/required_stops.rb
deleted file mode 100644
index 7c845201d4dd4..0000000000000
--- a/danger/plugins/required_stops.rb
+++ /dev/null
@@ -1,10 +0,0 @@
-# frozen_string_literal: true
-
-require_relative '../../tooling/danger/required_stops'
-
-module Danger
- class RequiredStops < ::Danger::Plugin
- # Put the helper code somewhere it can be tested
- include Tooling::Danger::RequiredStops
- end
-end
diff --git a/danger/required_stops/Dangerfile b/danger/required_stops/Dangerfile
deleted file mode 100644
index 227ee0aa18501..0000000000000
--- a/danger/required_stops/Dangerfile
+++ /dev/null
@@ -1,3 +0,0 @@
-# frozen_string_literal: true
-
-required_stops.add_comment_for_finalized_migrations
diff --git a/spec/tooling/danger/required_stops_spec.rb b/spec/tooling/danger/required_stops_spec.rb
deleted file mode 100644
index 1b811166d1397..0000000000000
--- a/spec/tooling/danger/required_stops_spec.rb
+++ /dev/null
@@ -1,96 +0,0 @@
-# frozen_string_literal: true
-
-require 'fast_spec_helper'
-require 'gitlab/dangerfiles/spec_helper'
-
-require_relative '../../../tooling/danger/required_stops'
-require_relative '../../../tooling/danger/project_helper'
-
-RSpec.describe Tooling::Danger::RequiredStops, feature_category: :tooling do
- include_context "with dangerfile"
-
- let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) }
- let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) }
- let(:warning_comment) { described_class::WARNING_COMMENT.chomp }
-
- subject(:required_stops) { fake_danger.new(helper: fake_helper) }
-
- before do
- allow(required_stops).to receive(:project_helper).and_return(fake_project_helper)
- end
-
- describe '#add_comment_for_finalized_migrations' do
- let(:file_lines) { file_diff.map { |line| line.delete_prefix('+').delete_prefix('-') } }
-
- before do
- allow(required_stops.project_helper).to receive(:file_lines).and_return(file_lines)
- allow(required_stops.helper).to receive(:all_changed_files).and_return([filename])
- allow(required_stops.helper).to receive(:changed_lines).with(filename).and_return(file_diff)
- end
-
- shared_examples "adds comment to added migration finalizations" do
- context 'when model has a newly added migration finalization' do
- let(:file_diff) do
- [
- "+ def up",
- "+ finalize_background_migration(MIGRATION)",
- "+ end",
- "+ def up",
- "+ finalize_background_migration('MyMigration')",
- "+ end",
- "+ def up",
- "+ ensure_batched_background_migration_is_finished(",
- "+ end",
- "+ def up",
- "+ ensure_batched_background_migration_is_finished('MyMigration')",
- "+ end",
- "+ def up",
- "+ finalize_batched_background_migration(",
- "+ end",
- "+ def up",
- "+ finalize_batched_background_migration('MyMigration')",
- "+ end"
- ]
- end
-
- it 'adds comment at the correct line' do
- matching_line_numbers = [2, 5, 8, 11, 14, 17]
- matching_line_numbers.each do |line_number|
- expect(required_stops).to receive(:markdown).with("\n#{warning_comment}", file: filename, line: line_number)
- end
-
- required_stops.add_comment_for_finalized_migrations
- end
- end
-
- context 'when model does not have migration finalization statement' do
- let(:file_diff) do
- [
- "+ queue_batched_background_migration(",
- "- ensure_batched_background_migration_is_finished("
- ]
- end
-
- it 'does not add comment' do
- expect(required_stops).not_to receive(:markdown)
-
- required_stops.add_comment_for_finalized_migrations
- end
- end
- end
-
- context 'when model has a newly added migration finalization' do
- context 'with regular migration' do
- let(:filename) { 'db/migrate/my_migration.rb' }
-
- include_examples 'adds comment to added migration finalizations'
- end
-
- context 'with post migration' do
- let(:filename) { 'db/post_migrate/my_migration.rb' }
-
- include_examples 'adds comment to added migration finalizations'
- end
- end
- end
-end
diff --git a/tooling/danger/required_stops.rb b/tooling/danger/required_stops.rb
deleted file mode 100644
index 6d933088e4468..0000000000000
--- a/tooling/danger/required_stops.rb
+++ /dev/null
@@ -1,40 +0,0 @@
-# frozen_string_literal: true
-
-require_relative 'suggestor'
-
-module Tooling
- module Danger
- module RequiredStops
- include ::Tooling::Danger::Suggestor
-
- MIGRATION_FILES_REGEX = %r{^db/(post_)?migrate}
-
- MIGRATION_FINALIZE_METHODS = %w[finalize_background_migration ensure_batched_background_migration_is_finished
- finalize_batched_background_migration].freeze
- MIGRATION_FINALIZE_REGEX = /^\+\s*(.*\.)?(#{MIGRATION_FINALIZE_METHODS.join('|')})[( ]/
-
- DOC_URL = "https://docs.gitlab.com/ee/development/database/required_stops.html"
- WARNING_COMMENT = <<~COMMENT.freeze
- Finalizing data migration might be time consuming and require a [required stop](#{DOC_URL}).
- Check the timings of the underlying data migration.
- Make sure the background migration was added before the last required stop.
- COMMENT
-
- def add_comment_for_finalized_migrations
- migration_files.each do |filename|
- add_suggestion(
- filename: filename,
- regex: MIGRATION_FINALIZE_REGEX,
- comment_text: WARNING_COMMENT
- )
- end
- end
-
- private
-
- def migration_files
- helper.all_changed_files.grep(MIGRATION_FILES_REGEX)
- end
- end
- end
-end
--
GitLab