diff --git a/danger/analytics_instrumentation/Dangerfile b/danger/analytics_instrumentation/Dangerfile
index 6b2139adab94d94a817fa126d35ed4458e16a7c7..4e57697c92ba362c19ce30ec7f64931b03da0912 100644
--- a/danger/analytics_instrumentation/Dangerfile
+++ b/danger/analytics_instrumentation/Dangerfile
@@ -9,3 +9,5 @@ analytics_instrumentation.check_affected_scopes!
analytics_instrumentation.check_usage_data_insertions!
analytics_instrumentation.check_deprecated_data_sources!
+
+analytics_instrumentation.check_removed_metric_fields!
diff --git a/spec/tooling/danger/analytics_instrumentation_spec.rb b/spec/tooling/danger/analytics_instrumentation_spec.rb
index 79c75b2e89cde9d8666fd9275b3c9466f1cc5c59..68eee4b24d1eb7895695f1d2e6e283a9455f8a26 100644
--- a/spec/tooling/danger/analytics_instrumentation_spec.rb
+++ b/spec/tooling/danger/analytics_instrumentation_spec.rb
@@ -12,6 +12,7 @@
subject(:analytics_instrumentation) { fake_danger.new(helper: fake_helper) }
let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) }
+ let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) }
let(:previous_label_to_add) { 'label_to_add' }
let(:labels_to_add) { [previous_label_to_add] }
let(:ci_env) { true }
@@ -233,8 +234,6 @@
end
describe '#check_deprecated_data_sources!' do
- let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) }
-
subject(:check_data_source) { analytics_instrumentation.check_deprecated_data_sources! }
before do
@@ -291,4 +290,138 @@
end
end
end
+
+ describe '#check_removed_metric_fields!' do
+ let(:file_lines) do
+ file_diff.map { |line| line.delete_prefix('+') }
+ end
+
+ let(:filename) { 'config/metrics/new_metric.yml' }
+ let(:mr_url) { 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/1' }
+ let(:milestone) { '17.0' }
+
+ before do
+ allow(fake_project_helper).to receive(:file_lines).with(filename).and_return(file_lines)
+ allow(fake_helper).to receive(:modified_files).and_return([filename])
+ allow(fake_helper).to receive(:changed_lines).with(filename).and_return(file_diff)
+ allow(fake_helper).to receive(:mr_web_url).and_return(mr_url)
+ allow(fake_helper).to receive(:mr_milestone).and_return(milestone)
+ allow(analytics_instrumentation).to receive(:project_helper).and_return(fake_project_helper)
+ end
+
+ context 'when metric was removed' do
+ context 'and removed_by_url is missing' do
+ let(:file_diff) do
+ [
+ "+---",
+ "+status: removed",
+ "+milestone_removed: #{milestone}"
+ ]
+ end
+
+ it 'adds suggestions' do
+ template = <<~SUGGEST_COMMENT
+ ```suggestion
+ status: removed
+ removed_by_url: %<mr_url>s
+ ```
+ SUGGEST_COMMENT
+
+ expected_format = format(template, mr_url: mr_url)
+
+ expect(analytics_instrumentation).to receive(:markdown).with(expected_format, file: filename, line: 2)
+
+ analytics_instrumentation.check_removed_metric_fields!
+ end
+ end
+
+ context 'and milestone_removed is missing' do
+ let(:file_diff) do
+ [
+ "+---",
+ "+status: removed",
+ "+removed_by_url: #{mr_url}"
+ ]
+ end
+
+ context 'when milestone is set for the MR' do
+ it 'adds suggestions' do
+ template = <<~SUGGEST_COMMENT
+ ```suggestion
+ status: removed
+ milestone_removed: %<milestone>s
+ ```
+ SUGGEST_COMMENT
+
+ expected_format = format(template, milestone: milestone)
+
+ expect(analytics_instrumentation).to receive(:markdown).with(expected_format, file: filename, line: 2)
+
+ analytics_instrumentation.check_removed_metric_fields!
+ end
+ end
+
+ context 'when milestone is not set for the MR' do
+ let(:milestone) { nil }
+
+ it 'adds suggestions with placeholder text and a comment' do
+ template = <<~SUGGEST_COMMENT
+ ```suggestion
+ status: removed
+ milestone_removed: [PLEASE SET MILESTONE]
+ ```
+ SUGGEST_COMMENT
+
+ expected_format = format("#{template}\nPlease set the `milestone_removed` value manually")
+
+ expect(analytics_instrumentation).to receive(:markdown).with(expected_format, file: filename, line: 2)
+
+ analytics_instrumentation.check_removed_metric_fields!
+ end
+ end
+ end
+
+ context 'and both removed_by_url and milestone_removed are missing' do
+ let(:file_diff) do
+ [
+ "+---",
+ "+status: removed"
+ ]
+ end
+
+ it 'adds suggestions' do
+ template = <<~SUGGEST_COMMENT
+ ```suggestion
+ status: removed
+ removed_by_url: %<mr_url>s
+ milestone_removed: %<milestone>s
+ ```
+ SUGGEST_COMMENT
+
+ expected_format = format(template, mr_url: mr_url, milestone: milestone)
+
+ expect(analytics_instrumentation).to receive(:markdown).with(expected_format, file: filename, line: 2)
+
+ analytics_instrumentation.check_removed_metric_fields!
+ end
+ end
+ end
+
+ context 'when metric was not removed' do
+ let(:file_diff) do
+ [
+ "+---",
+ "+status: active",
+ "+removed_by_url: #{mr_url}",
+ "+milestone_removed: #{milestone}"
+ ]
+ end
+
+ it 'does not add suggestions' do
+ expect(analytics_instrumentation).not_to receive(:markdown)
+
+ analytics_instrumentation.check_removed_metric_fields!
+ end
+ end
+ end
end
diff --git a/tooling/danger/analytics_instrumentation.rb b/tooling/danger/analytics_instrumentation.rb
index 47078f1cb314bbc2cdae5701f5e09a6391af1ca9..9b5ece1a1511e861d1c58d44f9c287688c7bdb5d 100644
--- a/tooling/danger/analytics_instrumentation.rb
+++ b/tooling/danger/analytics_instrumentation.rb
@@ -39,6 +39,8 @@ module AnalyticsInstrumentation
REVIEW_LABEL
].freeze
+ STATUS_REMOVED_REGEX = /^\+?status: removed\s?$/
+
def check!
analytics_instrumentation_paths_to_review = helper.changes.by_category(:analytics_instrumentation).files
@@ -77,6 +79,45 @@ def check_deprecated_data_sources!
end
end
+ def check_removed_metric_fields!
+ modified_config_files.each do |filename|
+ affected = false
+ removed_url = false
+ removed_milestone = false
+ helper.changed_lines(filename).each do |mod_line, _i|
+ affected = true if mod_line == '+status: removed'
+ removed_url = true if /^\+removed_by_url:\s?\w+/.match?(mod_line)
+ removed_milestone = true if /^\+milestone_removed:\s?\w+/.match?(mod_line)
+ end
+
+ next unless affected
+
+ milestone = helper.mr_milestone || '[PLEASE SET MILESTONE]'
+ comment_text = helper.mr_milestone ? nil : "Please set the `milestone_removed` value manually"
+
+ replacement = "status: removed\n"
+ if !removed_url && !removed_milestone
+ replacement += "removed_by_url: #{helper.mr_web_url}\nmilestone_removed: #{milestone}"
+ elsif !removed_url
+ replacement += "removed_by_url: #{helper.mr_web_url}"
+ comment_text = nil
+ elsif !removed_milestone
+ replacement += "milestone_removed: #{milestone}"
+ end
+
+ add_suggestion(
+ filename: filename,
+ regex: STATUS_REMOVED_REGEX,
+ replacement: replacement,
+ comment_text: comment_text
+ )
+ end
+ end
+
+ def modified_config_files
+ helper.modified_files.select { |f| f.include?('config/metrics') && f.end_with?('yml') }
+ end
+
private
def convert_to_table(items)