Merge branch 'tc-changelog-check-ee' into 'master'

Check the path of the Changelog

See merge request gitlab-org/gitlab!27588

danger/changelog/Dangerfile

@@ -3,9 +3,9 @@
 
 require 'yaml'
 
-SEE_DOC = "See [the documentation](https://docs.gitlab.com/ce/development/changelog.html)."
+SEE_DOC = "See [the documentation](https://docs.gitlab.com/ee/development/changelog.html)."
 CREATE_CHANGELOG_MESSAGE = <<~MSG
-You can create one with:
+If you want to create a changelog entry for GitLab FOSS, run the following:
 
 ```
 bin/changelog -m %<mr_iid>s "%<mr_title>s"
@@ -20,7 +20,7 @@ bin/changelog --ee -m %<mr_iid>s "%<mr_title>s"
 Note: Merge requests with %<labels>s do not trigger this check.
 MSG
 
-def check_changelog(path)
+def check_changelog_yaml(path)
   yaml = YAML.safe_load(File.read(path))
 
   fail "`title` should be set, in #{gitlab.html_link(path)}! #{SEE_DOC}" if yaml["title"].nil?
@@ -28,8 +28,6 @@ def check_changelog(path)
 
   if yaml["merge_request"].nil? && !helper.security_mr?
     message "Consider setting `merge_request` to #{gitlab.mr_json["iid"]} in #{gitlab.html_link(path)}. #{SEE_DOC}"
-  elsif yaml["merge_request"] != gitlab.mr_json["iid"] && !changelog.ce_port_changelog?(path)
-    fail "Merge request ID was not set to #{gitlab.mr_json["iid"]}! #{SEE_DOC}"
   end
 rescue Psych::SyntaxError, Psych::DisallowedClass, Psych::BadAlias
   # YAML could not be parsed, fail the build.
@@ -38,6 +36,19 @@ rescue StandardError => e
   warn "There was a problem trying to check the Changelog. Exception: #{e.name} - #{e.message}"
 end
 
+def check_changelog_path(path)
+  ee_changes = helper.all_ee_changes.dup
+  ee_changes.delete(path)
+
+  if ee_changes.any? && !changelog.ee_changelog?
+    warn "This MR has a Changelog file outside `ee/`, but code changes in `ee/`. Consider moving the Changelog file into `ee/`."
+  end
+
+  if ee_changes.empty? && changelog.ee_changelog?
+    warn "This MR has a Changelog file in `ee/`, but no code changes in `ee/`. Consider moving the Changelog file outside `ee/`."
+  end
+end
+
 def sanitized_mr_title
   helper.sanitize_mr_title(gitlab.mr_json["title"])
 end
@@ -49,11 +60,10 @@ end
 
 changelog_found = changelog.found
 
-if changelog.needed?
-  if changelog_found
-    check_changelog(changelog_found)
-  else
-    message "**[CHANGELOG missing](https://docs.gitlab.com/ce/development/changelog.html)**: 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.\n\n" +
-      format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: sanitized_mr_title, labels: changelog.presented_no_changelog_labels)
-  end
+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)**: 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.\n\n" +
+          format(CREATE_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], mr_title: sanitized_mr_title, labels: changelog.presented_no_changelog_labels)
 end

lib/gitlab/danger/changelog.rb

@@ -11,7 +11,7 @@ module Gitlab
       end
 
       def found
-        git.added_files.find { |path| path =~ %r{\A(ee/)?(changelogs/unreleased)(-ee)?/} }
+        @found ||= git.added_files.find { |path| path =~ %r{\A(ee/)?(changelogs/unreleased)(-ee)?/} }
       end
 
       def presented_no_changelog_labels
@@ -22,12 +22,8 @@ module Gitlab
         gitlab.mr_json["title"].gsub(/^WIP: */, '').gsub(/`/, '\\\`')
       end
 
-      def ee_changelog?(changelog_path)
-        changelog_path =~ /unreleased-ee/
-      end
-
-      def ce_port_changelog?(changelog_path)
-        helper.ee? && !ee_changelog?(changelog_path)
+      def ee_changelog?
+        found.start_with?('ee/')
       end
 
       private

lib/gitlab/danger/helper.rb

@@ -34,6 +34,10 @@ module Gitlab
           .sort
       end
 
+      def all_ee_changes
+        all_changed_files.grep(%r{\Aee/})
+      end
+
       def ee?
         # Support former project name for `dev` and support local Danger run
         %w[gitlab gitlab-ee].include?(ENV['CI_PROJECT_NAME']) || Dir.exist?('../../ee')

spec/lib/gitlab/danger/changelog_spec.rb

@@ -76,10 +76,10 @@ describe Gitlab::Danger::Changelog do
 
     context 'added files contain a changelog' do
       [
-        'changelogs/unreleased/entry.md',
-        'ee/changelogs/unreleased/entry.md',
-        'changelogs/unreleased-ee/entry.md',
-        'ee/changelogs/unreleased-ee/entry.md'
+        'changelogs/unreleased/entry.yml',
+        'ee/changelogs/unreleased/entry.yml',
+        'changelogs/unreleased-ee/entry.yml',
+        'ee/changelogs/unreleased-ee/entry.yml'
       ].each do |file_path|
         let(:added_files) { [file_path] }
 
@@ -107,46 +107,22 @@ describe Gitlab::Danger::Changelog do
   end
 
   describe '#ee_changelog?' do
-    context 'is ee changelog' do
-      [
-        'changelogs/unreleased-ee/entry.md',
-        'ee/changelogs/unreleased-ee/entry.md'
-      ].each do |file_path|
-        subject { changelog.ee_changelog?(file_path) }
+    subject { changelog.ee_changelog? }
 
-        it { is_expected.to be_truthy }
-      end
+    before do
+      allow(changelog).to receive(:found).and_return(file_path)
     end
 
-    context 'is not ee changelog' do
-      [
-        'changelogs/unreleased/entry.md',
-        'ee/changelogs/unreleased/entry.md'
-      ].each do |file_path|
-        subject { changelog.ee_changelog?(file_path) }
-
-        it { is_expected.to be_falsy }
-      end
-    end
-  end
+    context 'is ee changelog' do
+      let(:file_path) { 'ee/changelogs/unreleased/entry.yml' }
 
-  describe '#ce_port_changelog?' do
-    where(:helper_ee?, :file_path, :expected) do
-      true  | 'changelogs/unreleased-ee/entry.md'    | false
-      true  | 'ee/changelogs/unreleased-ee/entry.md' | false
-      false | 'changelogs/unreleased-ee/entry.md'    | false
-      false | 'ee/changelogs/unreleased-ee/entry.md' | false
-      true  | 'changelogs/unreleased/entry.md'       | true
-      true  | 'ee/changelogs/unreleased/entry.md'    | true
-      false | 'changelogs/unreleased/entry.md'       | false
-      false | 'ee/changelogs/unreleased/entry.md'    | false
+      it { is_expected.to be_truthy }
     end
 
-    with_them do
-      let(:ee?) { helper_ee? }
-      subject { changelog.ce_port_changelog?(file_path) }
+    context 'is not ee changelog' do
+      let(:file_path) { 'changelogs/unreleased/entry.yml' }
 
-      it { is_expected.to eq(expected) }
+      it { is_expected.to be_falsy }
     end
   end
 end

spec/lib/gitlab/danger/helper_spec.rb

@@ -76,6 +76,16 @@ describe Gitlab::Danger::Helper do
     end
   end
 
+  describe '#all_ee_changes' do
+    subject { helper.all_ee_changes }
+
+    it 'returns all changed files starting with ee/' do
+      expect(helper).to receive(:all_changed_files).and_return(%w[fr/ee/beer.rb ee/wine.rb ee/lib/ido.rb ee.k])
+
+      is_expected.to match_array(%w[ee/wine.rb ee/lib/ido.rb])
+    end
+  end
+
   describe '#ee?' do
     subject { helper.ee? }