From 20083ff6bb0b57b42e77f32fe933dd30626bfee5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Thu, 4 Feb 2021 18:04:48 +0100
Subject: [PATCH] danger: Make sure a feature flag removal comes with a
changelog
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This would address the use-case where a feature flag is removed with no
changelog, creating potential confusion for users.
Signed-off-by: Rémy Coutable <remy@rymai.me>
---
danger/changelog/Dangerfile | 8 +-
doc/development/changelog.md | 3 +-
spec/tooling/danger/changelog_spec.rb | 120 ++++++++++++++++++++++----
tooling/danger/changelog.rb | 27 ++++--
4 files changed, 130 insertions(+), 28 deletions(-)
diff --git a/danger/changelog/Dangerfile b/danger/changelog/Dangerfile
index c8474157fa56a..32c0277480cb7 100644
--- a/danger/changelog/Dangerfile
+++ b/danger/changelog/Dangerfile
@@ -46,7 +46,7 @@ def check_changelog_path(path)
ee_changes = helper.all_ee_changes.dup
ee_changes.delete(path)
- if ee_changes.any? && !changelog.ee_changelog? && !changelog.db_changes?
+ if ee_changes.any? && !changelog.ee_changelog? && !changelog.required?
warn "This MR has a Changelog file outside `ee/`, but code changes in `ee/`. Consider moving the Changelog file into `ee/`."
end
@@ -54,7 +54,7 @@ def check_changelog_path(path)
warn "This MR has a Changelog file in `ee/`, but no code changes in `ee/`. Consider moving the Changelog file outside `ee/`."
end
- if ee_changes.any? && changelog.ee_changelog? && changelog.db_changes?
+ if ee_changes.any? && changelog.ee_changelog? && changelog.required_reasons(feature_flag_helper: feature_flag).include?(:db_changes)
warn "This MR has a Changelog file inside `ee/`, but there are database changes which [requires](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry) the Changelog placement to be outside of `ee/`. Consider moving the Changelog file outside `ee/`."
end
end
@@ -68,8 +68,8 @@ changelog_found = changelog.found
if changelog_found
check_changelog_yaml(changelog_found)
check_changelog_path(changelog_found)
-elsif changelog.required?
- fail changelog.required_text
+elsif changelog.required?(feature_flag_helper: feature_flag)
+ changelog.required_texts(feature_flag_helper: feature_flag).each { |_, text| fail(text) }
elsif changelog.optional?
message changelog.optional_text
end
diff --git a/doc/development/changelog.md b/doc/development/changelog.md
index f2c8aa4db62fa..44be6e9548b11 100644
--- a/doc/development/changelog.md
+++ b/doc/development/changelog.md
@@ -57,8 +57,7 @@ the `author` field. GitLab team members **should not**.
- Any change behind an enabled feature flag **should** have a changelog entry.
- Any change that adds new usage data metrics and changes that needs to be documented in Product Intelligence [Event Dictionary](https://about.gitlab.com/handbook/product/product-intelligence-guide/#event-dictionary) **should** have a changelog entry.
- A change that adds snowplow events **should** have a changelog entry -
-- A change that [removes a feature flag](feature_flags/development.md) **should** have a changelog entry -
- only if the feature flag did not default to true already.
+- A change that [removes a feature flag](feature_flags/development.md) **must** have a changelog entry.
- A fix for a regression introduced and then fixed in the same release (i.e.,
fixing a bug introduced during a monthly release candidate) **should not**
have a changelog entry.
diff --git a/spec/tooling/danger/changelog_spec.rb b/spec/tooling/danger/changelog_spec.rb
index c0eca67ce92da..4b7ef9cf79e88 100644
--- a/spec/tooling/danger/changelog_spec.rb
+++ b/spec/tooling/danger/changelog_spec.rb
@@ -7,7 +7,8 @@
RSpec.describe Tooling::Danger::Changelog do
include DangerSpecHelper
- let(:added_files) { nil }
+ let(:added_files) { [] }
+ let(:removed_feature_flag_files) { [] }
let(:fake_git) { double('fake-git', added_files: added_files) }
let(:mr_labels) { nil }
@@ -19,10 +20,62 @@
let(:ee?) { false }
let(:fake_helper) { double('fake-helper', changes_by_category: changes_by_category, sanitize_mr_title: sanitize_mr_title, ee?: ee?) }
+ let(:fake_feature_flag) { double('feature-flag', feature_flag_files: removed_feature_flag_files) }
let(:fake_danger) { new_fake_danger.include(described_class) }
subject(:changelog) { fake_danger.new(git: fake_git, gitlab: fake_gitlab, helper: fake_helper) }
+ describe '#required_reasons' do
+ subject { changelog.required_reasons }
+
+ [
+ 'db/migrate/20200000000000_new_migration.rb',
+ 'db/post_migrate/20200000000000_new_migration.rb'
+ ].each do |file_path|
+ context "added files contain a migration (#{file_path})" do
+ let(:added_files) { [file_path] }
+
+ it { is_expected.to include(:db_changes) }
+ end
+ end
+
+ [
+ 'config/feature_flags/foo.yml',
+ 'ee/config/feature_flags/foo.yml'
+ ].each do |file_path|
+ context "removed files contains a feature flag (#{file_path})" do
+ let(:removed_feature_flag_files) { [file_path] }
+
+ context 'when no feature_flag_helper is given' do
+ it { is_expected.to be_empty }
+ end
+
+ context 'when a feature_flag_helper is given' do
+ subject { changelog.required_reasons(feature_flag_helper: fake_feature_flag) }
+
+ it { is_expected.to include(:feature_flag_removed) }
+ end
+ end
+ end
+
+ [
+ 'app/models/model.rb',
+ 'app/assets/javascripts/file.js'
+ ].each do |file_path|
+ context "added files do not contain a migration (#{file_path})" do
+ let(:added_files) { [file_path] }
+
+ it { is_expected.to be_empty }
+ end
+ end
+
+ context "removed files do not contain a feature flag" do
+ let(:removed_feature_flag_files) { [] }
+
+ it { is_expected.to be_empty }
+ end
+ end
+
describe '#required?' do
subject { changelog.required? }
@@ -37,6 +90,25 @@
end
end
+ [
+ 'config/feature_flags/foo.yml',
+ 'ee/config/feature_flags/foo.yml'
+ ].each do |file_path|
+ context "removed files contains a feature flag (#{file_path})" do
+ let(:removed_feature_flag_files) { [file_path] }
+
+ context 'when no feature_flag_helper is given' do
+ it { is_expected.to be_falsey }
+ end
+
+ context 'when a feature_flag_helper is given' do
+ subject { changelog.required?(feature_flag_helper: fake_feature_flag) }
+
+ it { is_expected.to be_truthy }
+ end
+ end
+ end
+
context 'added files do not contain a migration' do
[
'app/models/model.rb',
@@ -174,28 +246,46 @@
end
end
- describe '#required_text' do
+ describe '#required_texts' do
+ let(:sanitize_mr_title) { 'Fake Title' }
let(:mr_json) { { "iid" => 1234, "title" => sanitize_mr_title } }
- subject { changelog.required_text }
-
- context "when title is not changed from sanitization", :aggregate_failures do
- let(:sanitize_mr_title) { 'Fake Title' }
+ subject { changelog.required_texts }
+ shared_examples 'changelog required text' do |key|
specify do
- expect(subject).to include('CHANGELOG missing')
- expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
- expect(subject).not_to include('--ee')
+ expect(subject).to have_key(key)
+ expect(subject[key]).to include('CHANGELOG missing')
+ expect(subject[key]).to include('bin/changelog -m 1234 "Fake Title"')
+ expect(subject[key]).not_to include('--ee')
end
end
- context "when title needs sanitization", :aggregate_failures do
- let(:sanitize_mr_title) { 'DRAFT: Fake Title' }
+ context 'with a new migration file' do
+ let(:added_files) { ['db/migrate/20200000000000_new_migration.rb'] }
- specify do
- expect(subject).to include('CHANGELOG missing')
- expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
- expect(subject).not_to include('--ee')
+ context "when title is not changed from sanitization", :aggregate_failures do
+ it_behaves_like 'changelog required text', :db_changes
+ end
+
+ context "when title needs sanitization", :aggregate_failures do
+ let(:sanitize_mr_title) { 'DRAFT: Fake Title' }
+
+ it_behaves_like 'changelog required text', :db_changes
+ end
+ end
+
+ context 'with a removed feature flag file' do
+ let(:removed_feature_flag_files) { ['config/feature_flags/foo.yml'] }
+
+ context 'when no feature_flag_helper is given' do
+ it { is_expected.to be_empty }
+ end
+
+ context 'when a feature_flag_helper is given' do
+ subject { changelog.required_texts(feature_flag_helper: fake_feature_flag) }
+
+ it_behaves_like 'changelog required text', :feature_flag_removed
end
end
end
diff --git a/tooling/danger/changelog.rb b/tooling/danger/changelog.rb
index f7f505f51a639..30bdfd9f9ae1d 100644
--- a/tooling/danger/changelog.rb
+++ b/tooling/danger/changelog.rb
@@ -30,18 +30,28 @@ module Changelog
If this merge request [doesn't need a CHANGELOG entry](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry), feel free to ignore this message.
MSG
+ REQUIRED_CHANGELOG_REASONS = {
+ db_changes: 'introduces a database migration',
+ feature_flag_removed: 'removes a feature flag'
+ }.freeze
REQUIRED_CHANGELOG_MESSAGE = <<~MSG
To create a changelog entry, run the following:
#{CREATE_CHANGELOG_COMMAND}
- This merge request requires a changelog entry because it [introduces a database migration](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry).
+ This merge request requires a changelog entry because it [%<reason>s](https://docs.gitlab.com/ee/development/changelog.html#what-warrants-a-changelog-entry).
MSG
- def required?
- git.added_files.any? { |path| path =~ %r{\Adb/(migrate|post_migrate)/} }
+ def required_reasons(feature_flag_helper: nil)
+ [].tap do |reasons|
+ reasons << :db_changes if git.added_files.any? { |path| path =~ %r{\Adb/(migrate|post_migrate)/} }
+ reasons << :feature_flag_removed if feature_flag_helper&.respond_to?(:feature_flag_files) && feature_flag_helper.feature_flag_files(change_type: :deleted).any?
+ end
+ end
+
+ def required?(feature_flag_helper: nil)
+ required_reasons(feature_flag_helper: feature_flag_helper).any?
end
- alias_method :db_changes?, :required?
def optional?
categories_need_changelog? && without_no_changelog_label?
@@ -60,9 +70,12 @@ def modified_text
format(OPTIONAL_CHANGELOG_MESSAGE, mr_iid: mr_iid, mr_title: sanitized_mr_title)
end
- def required_text
- CHANGELOG_MISSING_URL_TEXT +
- format(REQUIRED_CHANGELOG_MESSAGE, mr_iid: mr_iid, mr_title: sanitized_mr_title)
+ def required_texts(feature_flag_helper: nil)
+ required_reasons(feature_flag_helper: feature_flag_helper).each_with_object({}) do |required_reason, memo|
+ memo[required_reason] =
+ CHANGELOG_MISSING_URL_TEXT +
+ format(REQUIRED_CHANGELOG_MESSAGE, reason: REQUIRED_CHANGELOG_REASONS.fetch(required_reason), mr_iid: mr_iid, mr_title: sanitized_mr_title)
+ end
end
def optional_text
--
GitLab