danger: Make sure a feature flag removal comes with a changelog

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

@@ -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

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.

spec/tooling/danger/changelog_spec.rb

@@ -7,7 +7,8 @@ require_relative '../../../tooling/danger/changelog'
 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 @@ RSpec.describe Tooling::Danger::Changelog do
   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 @@ RSpec.describe Tooling::Danger::Changelog do
       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 @@ RSpec.describe Tooling::Danger::Changelog do
     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

tooling/danger/changelog.rb

@@ -30,18 +30,28 @@ module Tooling
       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 @@ module Tooling
           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