Merge branch 'fail-danger-on-missing-migration-changelog' into 'master'

Fail Danger if a migration is added without a changelog

See merge request gitlab-org/gitlab!35290

danger/changelog/Dangerfile

@@ -4,21 +4,6 @@
 require 'yaml'
 
 SEE_DOC = "See the [changelog documentation](https://docs.gitlab.com/ee/development/changelog.html)."
-CREATE_CHANGELOG_MESSAGE = <<~MSG
-If you want to create a changelog entry for GitLab FOSS, run the following:
-
-```
-bin/changelog -m %<mr_iid>s "%<mr_title>s"
-```
-
-If you want to create a changelog entry for GitLab EE, run the following instead:
-
-```
-bin/changelog --ee -m %<mr_iid>s "%<mr_title>s"
-```
-
-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
 
 SUGGEST_MR_COMMENT = <<~SUGGEST_COMMENT
 ```suggestion
@@ -70,13 +55,8 @@ def check_changelog_path(path)
   end
 end
 
-def sanitized_mr_title
-  helper.sanitize_mr_title(gitlab.mr_json["title"])
-end
-
 if git.modified_files.include?("CHANGELOG.md")
-  fail "**CHANGELOG.md was edited.** Please remove the additions and create a CHANGELOG entry.\n\n" +
-    format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: sanitized_mr_title)
+  fail changelog.modified_text
 end
 
 changelog_found = changelog.found
@@ -84,7 +64,8 @@ changelog_found = changelog.found
 if changelog_found
   check_changelog_yaml(changelog_found)
   check_changelog_path(changelog_found)
-elsif changelog.needed?
-  message "**[CHANGELOG missing](https://docs.gitlab.com/ee/development/changelog.html)**:\n\n" +
-          format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: sanitized_mr_title)
+elsif changelog.required?
+  fail changelog.required_text
+elsif changelog.optional?
+  message changelog.optional_text
 end

doc/development/changelog.md

@@ -30,7 +30,8 @@ the `author` field. GitLab team members **should not**.
 ## What warrants a changelog entry?
 
 - Any change that introduces a database migration, whether it's regular, post,
-  or data migration, **must** have a changelog entry.
+  or data migration, **must** have a changelog entry, even if it is behind a
+  disabled feature flag.
 - [Security fixes](https://gitlab.com/gitlab-org/release/docs/blob/master/general/security/developer.md)
   **must** have a changelog entry, without `merge_request` value
   and with `type` set to `security`.
@@ -43,8 +44,8 @@ the `author` field. GitLab team members **should not**.
   a changelog entry regardless of these guidelines if the contributor wants one.
   Example: "Fixed a typo on the search results page."
 - Any docs-only changes **should not** have a changelog entry.
-- Any change behind a feature flag **should not** have a changelog entry - unless
-  the feature flag has been defaulted to true.
+- Any change behind a disabled feature flag **should not** have a changelog entry.
+- Any change behind an enabled feature flag **should** 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 fix for a regression introduced and then fixed in the same release (i.e.,

lib/gitlab/danger/changelog.rb

@@ -11,8 +11,36 @@ module Gitlab
         'meta'
       ].freeze
       NO_CHANGELOG_CATEGORIES = %i[docs none].freeze
+      CREATE_CHANGELOG_COMMAND = 'bin/changelog -m %<mr_iid>s "%<mr_title>s"'
+      CREATE_EE_CHANGELOG_COMMAND = 'bin/changelog --ee -m %<mr_iid>s "%<mr_title>s"'
+      CHANGELOG_MODIFIED_URL_TEXT = "**CHANGELOG.md was edited.** Please remove the additions and create a CHANGELOG entry.\n\n"
+      CHANGELOG_MISSING_URL_TEXT = "**[CHANGELOG missing](https://docs.gitlab.com/ee/development/changelog.html)**:\n\n"
 
-      def needed?
+      OPTIONAL_CHANGELOG_MESSAGE = <<~MSG
+      If you want to create a changelog entry for GitLab FOSS, run the following:
+
+          #{CREATE_CHANGELOG_COMMAND}
+
+      If you want to create a changelog entry for GitLab EE, run the following instead:
+
+          #{CREATE_EE_CHANGELOG_COMMAND}
+
+      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_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).
+      MSG
+
+      def required?
+        git.added_files.any? { |path| path =~ %r{\Adb/(migrate|post_migrate)/} }
+      end
+
+      def optional?
         categories_need_changelog? && without_no_changelog_label?
       end
 
@@ -20,16 +48,35 @@ module Gitlab
         @found ||= git.added_files.find { |path| path =~ %r{\A(ee/)?(changelogs/unreleased)(-ee)?/} }
       end
 
-      def sanitized_mr_title
-        gitlab.mr_json["title"].gsub(/^WIP: */, '').gsub(/`/, '\\\`')
-      end
-
       def ee_changelog?
         found.start_with?('ee/')
       end
 
+      def modified_text
+        CHANGELOG_MODIFIED_URL_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)
+      end
+
+      def optional_text
+        CHANGELOG_MISSING_URL_TEXT +
+          format(OPTIONAL_CHANGELOG_MESSAGE, mr_iid: mr_iid, mr_title: sanitized_mr_title)
+      end
+
       private
 
+      def mr_iid
+        gitlab.mr_json["iid"]
+      end
+
+      def sanitized_mr_title
+        helper.sanitize_mr_title(gitlab.mr_json["title"])
+      end
+
       def categories_need_changelog?
         (helper.changes_by_category.keys - NO_CHANGELOG_CATEGORIES).any?
       end

spec/lib/gitlab/danger/changelog_spec.rb

@@ -16,20 +16,47 @@ RSpec.describe Gitlab::Danger::Changelog do
   let(:fake_gitlab) { double('fake-gitlab', mr_labels: mr_labels, mr_json: mr_json) }
 
   let(:changes_by_category) { nil }
+  let(:sanitize_mr_title) { nil }
   let(:ee?) { false }
-  let(:fake_helper) { double('fake-helper', changes_by_category: changes_by_category, ee?: ee?) }
+  let(:fake_helper) { double('fake-helper', changes_by_category: changes_by_category, sanitize_mr_title: sanitize_mr_title, ee?: ee?) }
 
   let(:fake_danger) { new_fake_danger.include(described_class) }
 
   subject(:changelog) { fake_danger.new(git: fake_git, gitlab: fake_gitlab, helper: fake_helper) }
 
-  describe '#needed?' do
+  describe '#required?' do
+    subject { changelog.required? }
+
+    context 'added files contain a migration' do
+      [
+        'db/migrate/20200000000000_new_migration.rb',
+        'db/post_migrate/20200000000000_new_migration.rb'
+      ].each do |file_path|
+        let(:added_files) { [file_path] }
+
+        it { is_expected.to be_truthy }
+      end
+    end
+
+    context 'added files do not contain a migration' do
+      [
+        'app/models/model.rb',
+        'app/assets/javascripts/file.js'
+      ].each do |file_path|
+        let(:added_files) { [file_path] }
+
+        it { is_expected.to be_falsey }
+      end
+    end
+  end
+
+  describe '#optional?' do
     let(:category_with_changelog) { :backend }
     let(:label_with_changelog) { 'frontend' }
     let(:category_without_changelog) { Gitlab::Danger::Changelog::NO_CHANGELOG_CATEGORIES.first }
     let(:label_without_changelog) { Gitlab::Danger::Changelog::NO_CHANGELOG_LABELS.first }
 
-    subject { changelog.needed? }
+    subject { changelog.optional? }
 
     context 'when MR contains only categories requiring no changelog' do
       let(:changes_by_category) { { category_without_changelog => nil } }
@@ -121,4 +148,43 @@ RSpec.describe Gitlab::Danger::Changelog do
       it { is_expected.to be_falsy }
     end
   end
+
+  describe '#modified_text' do
+    let(:sanitize_mr_title) { 'Fake Title' }
+    let(:mr_json) { { "iid" => 1234, "title" => sanitize_mr_title } }
+
+    subject { changelog.modified_text }
+
+    it do
+      expect(subject).to include('CHANGELOG.md was edited')
+      expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
+      expect(subject).to include('bin/changelog --ee -m 1234 "Fake Title"')
+    end
+  end
+
+  describe '#required_text' do
+    let(:sanitize_mr_title) { 'Fake Title' }
+    let(:mr_json) { { "iid" => 1234, "title" => sanitize_mr_title } }
+
+    subject { changelog.required_text }
+
+    it do
+      expect(subject).to include('CHANGELOG missing')
+      expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
+      expect(subject).not_to include('--ee')
+    end
+  end
+
+  describe 'optional_text' do
+    let(:sanitize_mr_title) { 'Fake Title' }
+    let(:mr_json) { { "iid" => 1234, "title" => sanitize_mr_title } }
+
+    subject { changelog.optional_text }
+
+    it do
+      expect(subject).to include('CHANGELOG missing')
+      expect(subject).to include('bin/changelog -m 1234 "Fake Title"')
+      expect(subject).to include('bin/changelog --ee -m 1234 "Fake Title"')
+    end
+  end
 end