Merge branch...

Merge branch '301086-make-danger-fails-when-a-feature-flag-is-removed-and-no-changelog-is-added' into 'master'

danger: Make sure a feature flag removal comes with a changelog [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!53393

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.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
@@ -69,7 +69,7 @@ if changelog_found
   check_changelog_yaml(changelog_found)
   check_changelog_path(changelog_found)
 elsif changelog.required?
-  fail changelog.required_text
+  changelog.required_texts.each { |_, text| fail(text) } # rubocop:disable Lint/UnreachableLoop
 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/helper_spec.rb

@@ -10,13 +10,27 @@ RSpec.describe Tooling::Danger::Helper do
   using RSpec::Parameterized::TableSyntax
   include DangerSpecHelper
 
-  let(:fake_git) { double('fake-git') }
-
   let(:mr_author) { nil }
   let(:fake_gitlab) { double('fake-gitlab', mr_author: mr_author) }
 
   let(:fake_danger) { new_fake_danger.include(described_class) }
 
+  let(:added_files) { %w[added1] }
+  let(:modified_files) { %w[modified1] }
+  let(:deleted_files) { %w[deleted1] }
+  let(:renamed_before_file) { 'renamed_before' }
+  let(:renamed_after_file) { 'renamed_after' }
+  let(:renamed_files) { [{ before: renamed_before_file, after: renamed_after_file }] }
+
+  let(:fake_git) { double('fake-git') }
+
+  before do
+    allow(fake_git).to receive(:added_files) { added_files }
+    allow(fake_git).to receive(:modified_files) { modified_files }
+    allow(fake_git).to receive(:deleted_files) { deleted_files }
+    allow(fake_git).to receive(:renamed_files).at_least(:twice) { renamed_files }
+  end
+
   subject(:helper) { fake_danger.new(git: fake_git, gitlab: fake_gitlab) }
 
   describe '#gitlab_helper' do
@@ -191,15 +205,16 @@ RSpec.describe Tooling::Danger::Helper do
   end
 
   describe '#changes_by_category' do
-    it 'categorizes changed files' do
-      expect(fake_git).to receive(:added_files) { %w[foo foo.md foo.rb foo.js db/migrate/foo lib/gitlab/database/foo.rb qa/foo ee/changelogs/foo.yml] }
-      allow(fake_git).to receive(:modified_files) { [] }
-      allow(fake_git).to receive(:renamed_files) { [] }
+    let(:added_files) { %w[foo foo.md foo.rb foo.js] }
+    let(:modified_files) { %w[db/migrate/foo lib/gitlab/database/foo.rb] }
+    let(:renamed_files) { [{ before: '', after: 'qa/foo' }, { before: '', after: 'ee/changelogs/foo.yml' }] }
 
+    it 'categorizes changed files' do
       expect(helper.changes_by_category).to eq(
         backend: %w[foo.rb],
         database: %w[db/migrate/foo lib/gitlab/database/foo.rb],
         frontend: %w[foo.js],
+        migration: %w[db/migrate/foo],
         none: %w[ee/changelogs/foo.yml foo.md],
         qa: %w[qa/foo],
         unknown: %w[foo]
@@ -207,6 +222,62 @@ RSpec.describe Tooling::Danger::Helper do
     end
   end
 
+  describe 'Tooling::Danger::Helper::Changes', :aggregate_failures do
+    let(:added_files) { %w[db/migrate/foo ee/changelogs/unreleased/foo.yml] }
+
+    describe '#has_category?' do
+      it 'returns true when changes include given category, false otherwise' do
+        changes = helper.changes
+
+        expect(changes.has_category?(:migration)).to eq(true)
+        expect(changes.has_category?(:changelog)).to eq(true)
+        expect(changes.has_category?(:backend)).to eq(false)
+      end
+    end
+
+    describe '#by_category' do
+      it 'returns an array of Change objects' do
+        expect(helper.changes.by_category(:migration)).to all(be_an(described_class::Change))
+      end
+
+      it 'returns an array of Change objects with the given category' do
+        changes = helper.changes
+
+        expect(changes.by_category(:migration).files).to eq(['db/migrate/foo'])
+        expect(changes.by_category(:changelog).files).to eq(['ee/changelogs/unreleased/foo.yml'])
+        expect(changes.by_category(:backend)).to be_empty
+      end
+    end
+
+    describe '#categories' do
+      it 'returns an array of category symbols' do
+        expect(helper.changes.categories).to contain_exactly(:database, :migration, :changelog, :unknown)
+      end
+    end
+
+    describe '#files' do
+      it 'returns an array of files' do
+        expect(helper.changes.files).to include(*added_files)
+      end
+    end
+  end
+
+  describe '#changes' do
+    it 'returns an array of Change objects' do
+      expect(helper.changes).to all(be_an(described_class::Change))
+    end
+
+    it 'groups changes by change type' do
+      changes = helper.changes
+
+      expect(changes.added.files).to eq(added_files)
+      expect(changes.modified.files).to eq(modified_files)
+      expect(changes.deleted.files).to eq(deleted_files)
+      expect(changes.renamed_before.files).to eq([renamed_before_file])
+      expect(changes.renamed_after.files).to eq([renamed_after_file])
+    end
+  end
+
   describe '#categories_for_file' do
     before do
       allow(fake_git).to receive(:diff_for_file).with('usage_data.rb') { double(:diff, patch: "+ count(User.active)") }
@@ -304,12 +375,10 @@ RSpec.describe Tooling::Danger::Helper do
 
       'db/schema.rb'                                              | [:database]
       'db/structure.sql'                                          | [:database]
-      'db/migrate/foo'                                            | [:database]
-      'db/post_migrate/foo'                                       | [:database]
-      'ee/db/migrate/foo'                                         | [:database]
-      'ee/db/post_migrate/foo'                                    | [:database]
-      'ee/db/geo/migrate/foo'                                     | [:database]
-      'ee/db/geo/post_migrate/foo'                                | [:database]
+      'db/migrate/foo'                                            | [:database, :migration]
+      'db/post_migrate/foo'                                       | [:database, :migration]
+      'ee/db/geo/migrate/foo'                                     | [:database, :migration]
+      'ee/db/geo/post_migrate/foo'                                | [:database, :migration]
       'app/models/project_authorization.rb'                       | [:database]
       'app/services/users/refresh_authorized_projects_service.rb' | [:database]
       'lib/gitlab/background_migration.rb'                        | [:database]
@@ -400,6 +469,22 @@ RSpec.describe Tooling::Danger::Helper do
     end
   end
 
+  describe '#mr_iid' do
+    it 'returns "" when `gitlab_helper` is unavailable' do
+      expect(helper).to receive(:gitlab_helper).and_return(nil)
+
+      expect(helper.mr_iid).to eq('')
+    end
+
+    it 'returns the MR IID when `gitlab_helper` is available' do
+      mr_iid = '1234'
+      expect(fake_gitlab).to receive(:mr_json)
+        .and_return('iid' => mr_iid)
+
+      expect(helper.mr_iid).to eq(mr_iid)
+    end
+  end
+
   describe '#mr_title' do
     it 'returns "" when `gitlab_helper` is unavailable' do
       expect(helper).to receive(:gitlab_helper).and_return(nil)
@@ -432,6 +517,22 @@ RSpec.describe Tooling::Danger::Helper do
     end
   end
 
+  describe '#mr_labels' do
+    it 'returns "" when `gitlab_helper` is unavailable' do
+      expect(helper).to receive(:gitlab_helper).and_return(nil)
+
+      expect(helper.mr_labels).to eq([])
+    end
+
+    it 'returns the MR labels when `gitlab_helper` is available' do
+      mr_labels = %w[foo bar baz]
+      expect(fake_gitlab).to receive(:mr_labels)
+        .and_return(mr_labels)
+
+      expect(helper.mr_labels).to eq(mr_labels)
+    end
+  end
+
   describe '#mr_target_branch' do
     it 'returns "" when `gitlab_helper` is unavailable' do
       expect(helper).to receive(:gitlab_helper).and_return(nil)

tooling/danger/changelog.rb

@@ -30,25 +30,35 @@ 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_reasons
+        [].tap do |reasons|
+          reasons << :db_changes if helper.changes.added.has_category?(:migration)
+          reasons << :feature_flag_removed if helper.changes.deleted.has_category?(:feature_flag)
+        end
+      end
+
       def required?
-        git.added_files.any? { |path| path =~ %r{\Adb/(migrate|post_migrate)/} }
+        required_reasons.any?
       end
-      alias_method :db_changes?, :required?
 
       def optional?
         categories_need_changelog? && without_no_changelog_label?
       end
 
       def found
-        @found ||= git.added_files.find { |path| path =~ %r{\A(ee/)?(changelogs/unreleased)(-ee)?/} }
+        @found ||= helper.changes.added.by_category(:changelog).files.first
       end
 
       def ee_changelog?
@@ -57,35 +67,34 @@ module Tooling
 
       def modified_text
         CHANGELOG_MODIFIED_URL_TEXT +
-          format(OPTIONAL_CHANGELOG_MESSAGE, mr_iid: mr_iid, mr_title: sanitized_mr_title)
+          format(OPTIONAL_CHANGELOG_MESSAGE, mr_iid: helper.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
+        required_reasons.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: helper.mr_iid, mr_title: sanitized_mr_title)
+        end
       end
 
       def optional_text
         CHANGELOG_MISSING_URL_TEXT +
-          format(OPTIONAL_CHANGELOG_MESSAGE, mr_iid: mr_iid, mr_title: sanitized_mr_title)
+          format(OPTIONAL_CHANGELOG_MESSAGE, mr_iid: helper.mr_iid, mr_title: sanitized_mr_title)
       end
 
       private
 
-      def mr_iid
-        gitlab.mr_json["iid"]
-      end
-
       def sanitized_mr_title
-        TitleLinting.sanitize_mr_title(gitlab.mr_json["title"])
+        TitleLinting.sanitize_mr_title(helper.mr_title)
       end
 
       def categories_need_changelog?
-        (helper.changes_by_category.keys - NO_CHANGELOG_CATEGORIES).any?
+        (helper.changes.categories - NO_CHANGELOG_CATEGORIES).any?
       end
 
       def without_no_changelog_label?
-        (gitlab.mr_labels & NO_CHANGELOG_LABELS).empty?
+        (helper.mr_labels & NO_CHANGELOG_LABELS).empty?
       end
     end
   end

tooling/danger/helper.rb

 # frozen_string_literal: true
 
+require 'delegate'
+
 require_relative 'teammate'
 require_relative 'title_linting'
 
@@ -86,13 +88,84 @@ module Tooling
         end
       end
 
-      # @return [Hash<String,Array<String>>]
+      Change = Struct.new(:file, :change_type, :category)
+
+      class Changes < ::SimpleDelegator
+        def added
+          select_by_change_type(:added)
+        end
+
+        def modified
+          select_by_change_type(:modified)
+        end
+
+        def deleted
+          select_by_change_type(:deleted)
+        end
+
+        def renamed_before
+          select_by_change_type(:renamed_before)
+        end
+
+        def renamed_after
+          select_by_change_type(:renamed_after)
+        end
+
+        def has_category?(category)
+          any? { |change| change.category == category }
+        end
+
+        def by_category(category)
+          Changes.new(select { |change| change.category == category })
+        end
+
+        def categories
+          map(&:category).uniq
+        end
+
+        def files
+          map(&:file)
+        end
+
+        private
+
+        def select_by_change_type(change_type)
+          Changes.new(select { |change| change.change_type == change_type })
+        end
+      end
+
+      # @return [Hash<Symbol,Array<String>>]
       def changes_by_category
         all_changed_files.each_with_object(Hash.new { |h, k| h[k] = [] }) do |file, hash|
           categories_for_file(file).each { |category| hash[category] << file }
         end
       end
 
+      # @return [Changes]
+      def changes
+        Changes.new([]).tap do |changes|
+          git.added_files.each do |file|
+            categories_for_file(file).each { |category| changes << Change.new(file, :added, category) }
+          end
+
+          git.modified_files.each do |file|
+            categories_for_file(file).each { |category| changes << Change.new(file, :modified, category) }
+          end
+
+          git.deleted_files.each do |file|
+            categories_for_file(file).each { |category| changes << Change.new(file, :deleted, category) }
+          end
+
+          git.renamed_files.map { |x| x[:before] }.each do |file|
+            categories_for_file(file).each { |category| changes << Change.new(file, :renamed_before, category) }
+          end
+
+          git.renamed_files.map { |x| x[:after] }.each do |file|
+            categories_for_file(file).each { |category| changes << Change.new(file, :renamed_after, category) }
+          end
+        end
+      end
+
       # Determines the categories a file is in, e.g., `[:frontend]`, `[:backend]`, or  `%i[frontend engineering_productivity]`
       # using filename regex and specific change regex if given.
       #
@@ -130,6 +203,10 @@ module Tooling
       CATEGORIES = {
         [%r{usage_data\.rb}, %r{^(\+|-).*\s+(count|distinct_count|estimate_batch_distinct_count)\(.*\)(.*)$}] => [:database, :backend],
 
+        %r{\A(ee/)?config/feature_flags/} => :feature_flag,
+
+        %r{\A(ee/)?(changelogs/unreleased)(-ee)?/} => :changelog,
+
         %r{\Adoc/.*(\.(md|png|gif|jpg))\z} => :docs,
         %r{\A(CONTRIBUTING|LICENSE|MAINTENANCE|PHILOSOPHY|PROCESS|README)(\.md)?\z} => :docs,
 
@@ -159,6 +236,7 @@ module Tooling
           \.gitlab/ci/frontend\.gitlab-ci\.yml
         )\z}x => %i[frontend engineering_productivity],
 
+        %r{\A(ee/)?db/(geo/)?(migrate|post_migrate)/} => [:database, :migration],
         %r{\A(ee/)?db/(?!fixtures)[^/]+} => :database,
         %r{\A(ee/)?lib/gitlab/(database|background_migration|sql|github_import)(/|\.rb)} => :database,
         %r{\A(app/models/project_authorization|app/services/users/refresh_authorized_projects_service)(/|\.rb)} => :database,
@@ -214,6 +292,12 @@ module Tooling
         usernames.map { |u| Tooling::Danger::Teammate.new('username' => u) }
       end
 
+      def mr_iid
+        return '' unless gitlab_helper
+
+        gitlab_helper.mr_json['iid']
+      end
+
       def mr_title
         return '' unless gitlab_helper
 
@@ -226,6 +310,12 @@ module Tooling
         gitlab_helper.mr_json['web_url']
       end
 
+      def mr_labels
+        return [] unless gitlab_helper
+
+        gitlab_helper.mr_labels
+      end
+
       def mr_target_branch
         return '' unless gitlab_helper
 
@@ -257,10 +347,9 @@ module Tooling
       end
 
       def mr_has_labels?(*labels)
-        return false unless gitlab_helper
-
         labels = labels.flatten.uniq
-        (labels & gitlab_helper.mr_labels) == labels
+
+        (labels & mr_labels) == labels
       end
 
       def labels_list(labels, sep: ', ')