Add latest changes from gitlab-org/gitlab@13-1-stable-ee

6 files changed, 112 insertions, 81 deletions

diff --git a/danger/bundle_size/Dangerfile b/danger/bundle_size/Dangerfile
new file mode 100644
index 00000000000..a7102cd0e38
--- /dev/null
+++ b/danger/bundle_size/Dangerfile
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+analysis_result = "./bundle-size-review/analysis.json"
+markdown_result = "./bundle-size-review/comparison.md"
+
+# Executing the webpack-entry-point-analyser
+# We would like to do that in the CI file directly,
+# but unfortunately the head_commit SHA is not available
+# as a CI variable due to our merge into master simulation
+analyze_cmd = [
+  "webpack-entry-point-analyser",
+  "--from-file ./webpack-report/stats.json",
+  "--json #{analysis_result}",
+  " --sha #{gitlab&.head_commit}"
+].join(" ")
+
+# execute analysis
+`#{analyze_cmd}`
+
+# We are executing the comparison by comparing the start_sha
+# to the current pipeline result. The start_sha is the commit
+# from master that was merged into for the merged pipeline.
+comparison_cmd = [
+  "webpack-compare-reports",
+  "--job #{ENV["CI_JOB_ID"]}",
+  "--to-file #{analysis_result}",
+  "--html ./bundle-size-review/comparison.html",
+  "--markdown #{markdown_result}"
+].join(" ")
+
+# execute comparison
+`#{comparison_cmd}`
+
+comment = `cat #{markdown_result}`
+
+markdown(<<~MARKDOWN)
+    #{comment}
+MARKDOWN
diff --git a/danger/changelog/Dangerfile b/danger/changelog/Dangerfile
index 7b8a096639d..aa7a81555a3 100644
--- a/danger/changelog/Dangerfile
+++ b/danger/changelog/Dangerfile
@@ -3,7 +3,7 @@
 
 require 'yaml'
 
-SEE_DOC = "See [the documentation](https://docs.gitlab.com/ee/development/changelog.html)."
+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:
 
@@ -20,14 +20,29 @@ 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
+merge_request: %<mr_iid>s
+```
+
+#{SEE_DOC}
+SUGGEST_COMMENT
+
 def check_changelog_yaml(path)
-  yaml = YAML.safe_load(File.read(path))
+  raw_file = File.read(path)
+  yaml = YAML.safe_load(raw_file)
 
   fail "`title` should be set, in #{gitlab.html_link(path)}! #{SEE_DOC}" if yaml["title"].nil?
   fail "`type` should be set, in #{gitlab.html_link(path)}! #{SEE_DOC}" if yaml["type"].nil?
 
   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}"
+    mr_line = raw_file.lines.find_index("merge_request:\n")
+
+    if mr_line
+      markdown(format(SUGGEST_MR_COMMENT, mr_iid: gitlab.mr_json["iid"]), file: path, line: mr_line.succ)
+    else
+      message "Consider setting `merge_request` to #{gitlab.mr_json["iid"]} in #{gitlab.html_link(path)}. #{SEE_DOC}"
+    end
   elsif yaml["merge_request"] != gitlab.mr_json["iid"] && !helper.security_mr?
     fail "Merge request ID was not set to #{gitlab.mr_json["iid"]}! #{SEE_DOC}"
   end
diff --git a/danger/commit_messages/Dangerfile b/danger/commit_messages/Dangerfile
index 59d42082de9..174fc10eef3 100644
--- a/danger/commit_messages/Dangerfile
+++ b/danger/commit_messages/Dangerfile
@@ -6,6 +6,16 @@ COMMIT_MESSAGE_GUIDELINES = "https://docs.gitlab.com/ee/development/contributing
 MORE_INFO = "For more information, take a look at our [Commit message guidelines](#{COMMIT_MESSAGE_GUIDELINES})."
 THE_DANGER_JOB_TEXT = "the `danger-review` job"
 MAX_COMMITS_COUNT = 10
+MAX_COMMITS_COUNT_EXCEEDED_MESSAGE = <<~MSG
+This merge request includes more than %<max_commits_count>d commits. Each commit should meet the following criteria:
+
+1. Have a well-written commit message.
+1. Has all tests passing when used on its own (e.g. when using git checkout SHA).
+1. Can be reverted on its own without also requiring the revert of commit that came before it.
+1. Is small enough that it can be reviewed in isolation in under 30 minutes or so.
+
+If this merge request contains commits that do not meet this criteria and/or contains intermediate work, please rebase these commits into a smaller number of commits or split this merge request into multiple smaller merge requests.
+MSG
 
 def gitlab_danger
   @gitlab_danger ||= GitlabDanger.new(helper.gitlab_helper)
@@ -94,11 +104,7 @@ def lint_commits(commits)
   warn_or_fail_commits(failed_commit_linters, default_to_fail: !squash_mr?)
 
   if count_non_fixup_commits(commit_linters) > MAX_COMMITS_COUNT
-    level = squash_mr? ? :warn : :fail
-    self.__send__(level, # rubocop:disable GitlabSecurity/PublicSend
-      "This merge request includes more than #{MAX_COMMITS_COUNT} commits. " \
-        'Please rebase these commits into a smaller number of commits or split ' \
-        'this merge request into multiple smaller merge requests.')
+    self.warn(format(MAX_COMMITS_COUNT_EXCEEDED_MESSAGE, max_commits_count: MAX_COMMITS_COUNT))
   end
 
   if squash_mr?
diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile
index d73f6bf4f10..54559af4066 100644
--- a/danger/roulette/Dangerfile
+++ b/danger/roulette/Dangerfile
@@ -36,36 +36,22 @@ Please consider creating a merge request to
 for them.
 MARKDOWN
 
-NO_REVIEWER = 'No reviewer available'.freeze
-NO_MAINTAINER = 'No maintainer available'.freeze
+OPTIONAL_REVIEW_TEMPLATE = "%{role} review is optional for %{category}".freeze
+NOT_AVAILABLE_TEMPLATE = 'No %{role} available'.freeze
 
-Spin = Struct.new(:reviewer, :maintainer)
-
-def spin_role_for_category(team, role, project, category)
-  team.select do |member|
-    member.public_send("#{role}?", project, category, gitlab.mr_labels) # rubocop:disable GitlabSecurity/PublicSend
+def note_for_category_role(spin, role)
+  if spin.optional_role == role
+    return OPTIONAL_REVIEW_TEMPLATE % { role: role.capitalize, category: helper.label_for_category(spin.category) }
   end
-end
-
-def spin_for_category(team, project, category, branch_name)
-  reviewers, traintainers, maintainers =
-    %i[reviewer traintainer maintainer].map do |role|
-      spin_role_for_category(team, role, project, category)
-    end
 
-  # TODO: take CODEOWNERS into account?
-  # https://gitlab.com/gitlab-org/gitlab/issues/26723
-
-  # Make traintainers have triple the chance to be picked as a reviewer
-  random = roulette.new_random(branch_name)
-  reviewer = roulette.spin_for_person(reviewers + traintainers + traintainers, random: random)
-  maintainer = roulette.spin_for_person(maintainers, random: random)
-
-  Spin.new(reviewer, maintainer)
+  spin.public_send(role)&.markdown_name || NOT_AVAILABLE_TEMPLATE % { role: role } # rubocop:disable GitlabSecurity/PublicSend
 end
 
-def markdown_row_for_category(category, reviewer, maintainer)
-  "| #{helper.label_for_category(category)} | #{reviewer&.markdown_name || NO_REVIEWER} | #{maintainer&.markdown_name || NO_MAINTAINER} |"
+def markdown_row_for_spin(spin)
+  reviewer_note = note_for_category_role(spin, :reviewer)
+  maintainer_note = note_for_category_role(spin, :maintainer)
+
+  "| #{helper.label_for_category(spin.category)} | #{reviewer_note} | #{maintainer_note} |"
 end
 
 changes = helper.changes_by_category
@@ -78,43 +64,13 @@ categories = changes.keys - [:unknown]
 categories << :database if gitlab.mr_labels.include?('database') && !categories.include?(:database)
 
 if changes.any?
-  # Strip leading and trailing CE/EE markers
-  canonical_branch_name =
-    roulette.canonical_branch_name(gitlab.mr_json['source_branch'])
-
-  team =
-    begin
-      roulette.project_team(helper.project_name)
-    rescue => err
-      warn("Reviewer roulette failed to load team data: #{err.message}")
-      []
-    end
-
   project = helper.project_name
-  unknown = changes.fetch(:unknown, [])
-  spin_per_category = categories.each_with_object({}) do |category, memo|
-    memo[category] = spin_for_category(team, project, category, canonical_branch_name)
-  end
+  branch_name = gitlab.mr_json['source_branch']
 
-  rows = spin_per_category.map do |category, spin|
-    reviewer = spin.reviewer
-    maintainer = spin.maintainer
-
-    case category
-    when :test
-      if reviewer.nil?
-        # Fetch an already picked backend reviewer, or pick one otherwise
-        reviewer = spin_per_category[:backend]&.reviewer || spin_for_category(team, project, :backend, canonical_branch_name).reviewer
-      end
-    when :engineering_productivity
-      if maintainer.nil?
-        # Fetch an already picked backend maintainer, or pick one otherwise
-        maintainer = spin_per_category[:backend]&.maintainer || spin_for_category(team, project, :backend, canonical_branch_name).maintainer
-      end
-    end
-
-    markdown_row_for_category(category, reviewer, maintainer)
-  end
+  roulette_spins = roulette.spin(project, categories, branch_name)
+  rows = roulette_spins.map { |spin| markdown_row_for_spin(spin) }
+
+  unknown = changes.fetch(:unknown, [])
 
   markdown(MESSAGE)
   markdown(CATEGORY_TABLE_HEADER + rows.join("\n")) unless rows.empty?
diff --git a/danger/specs/Dangerfile b/danger/specs/Dangerfile
index 7803fad8472..784ce75fef8 100644
--- a/danger/specs/Dangerfile
+++ b/danger/specs/Dangerfile
@@ -1,16 +1,35 @@
 # frozen_string_literal: true
 
 NO_SPECS_LABELS = %w[backstage documentation QA].freeze
-NO_NEW_SPEC_MESSAGE = <<~MSG.freeze
+NO_NEW_SPEC_MESSAGE = <<~MSG
 You've made some app changes, but didn't add any tests.
 That's OK as long as you're refactoring existing code,
 but please consider adding any of the %<labels>s labels.
 MSG
+EE_CHANGE_WITH_FOSS_SPEC_CHANGE_MESSAGE = <<~MSG
+You've made some EE-specific changes, but only made changes to FOSS tests.
+This could be a sign that you're testing an EE-specific behavior in a FOSS test.
 
-has_app_changes = !helper.all_changed_files.grep(%r{\A(ee/)?(app|lib|db/(geo/)?(post_)?migrate)/}).empty?
-has_spec_changes = !helper.all_changed_files.grep(%r{\A(ee/)?spec/}).empty?
+Please make sure the spec files pass in AS-IF-FOSS mode either:
+
+1. Locally with `FOSS_ONLY=1 bin/rspec -- %<spec_files>s`.
+1. In the MR pipeline by verifying that the `rspec foss-impact` job has passed.
+1. In the MR pipelines by including `RUN AS-IF-FOSS` in the MR title (you can do it with the ``/title %<mr_title>s [RUN AS-IF-FOSS]`` quick action) and start a new MR pipeline.
+
+MSG
+
+has_app_changes = helper.all_changed_files.grep(%r{\A(app|lib|db/(geo/)?(post_)?migrate)/}).any?
+has_ee_app_changes = helper.all_changed_files.grep(%r{\Aee/(app|lib|db/(geo/)?(post_)?migrate)/}).any?
+spec_changes = helper.all_changed_files.grep(%r{\Aspec/})
+has_spec_changes = spec_changes.any?
+has_ee_spec_changes = helper.all_changed_files.grep(%r{\Aee/spec/}).any?
 new_specs_needed = (gitlab.mr_labels & NO_SPECS_LABELS).empty?
 
-if has_app_changes && !has_spec_changes && new_specs_needed
+if (has_app_changes || has_ee_app_changes) && !(has_spec_changes || has_ee_spec_changes) && new_specs_needed
   warn format(NO_NEW_SPEC_MESSAGE, labels: helper.labels_list(NO_SPECS_LABELS)), sticky: false
 end
+
+# The only changes outside `ee/` are in `spec/`
+if has_ee_app_changes && has_spec_changes && !(has_app_changes || has_ee_spec_changes)
+  warn format(EE_CHANGE_WITH_FOSS_SPEC_CHANGE_MESSAGE, spec_files: spec_changes.join(" "), mr_title: gitlab.mr_json['title']), sticky: false
+end
diff --git a/danger/telemetry/Dangerfile b/danger/telemetry/Dangerfile
index c18a15fcb03..b749bd3b80b 100644
--- a/danger/telemetry/Dangerfile
+++ b/danger/telemetry/Dangerfile
@@ -1,12 +1,11 @@
 # frozen_string_literal: true
 
 TELEMETRY_CHANGED_FILES_MESSAGE = <<~MSG
-This merge request includes changes for which a review from the Data team and Telemetry team is recommended.
-Please reach out to @gitlab-org/growth/telemetry/engineers group for a review.
-MSG
+For the following files, a review from the [Data team and Telemetry team](https://gitlab.com/groups/gitlab-org/growth/telemetry/engineers/-/group_members?with_inherited_permissions=exclude) is recommended
+Please check the ~telemetry [guide](https://docs.gitlab.com/ee/development/telemetry/usage_ping.html) and reach out to @gitlab-org/growth/telemetry/engineers group for a review.
+
+%<changed_files>s
 
-USAGE_DATA_FILES_MESSAGE = <<~MSG
-For the following files, a review from the [Data team and Telemetry team](https://gitlab.com/groups/gitlab-org/growth/telemetry/engineers/-/group_members?with_inherited_permissions=exclude) is recommended:
 MSG
 
 tracking_files = [
@@ -16,7 +15,7 @@ tracking_files = [
   'spec/helpers/tracking_helper_spec.rb',
   'app/assets/javascripts/tracking.js',
   'spec/frontend/tracking_spec.js'
-  ]
+]
 
 usage_data_changed_files = git.modified_files.grep(%r{usage_data})
 snowplow_events_changed_files = git.modified_files & tracking_files
@@ -24,9 +23,7 @@ snowplow_events_changed_files = git.modified_files & tracking_files
 changed_files = (usage_data_changed_files + snowplow_events_changed_files)
 
 if changed_files.any?
-  warn format(TELEMETRY_CHANGED_FILES_MESSAGE)
-
-  markdown(USAGE_DATA_FILES_MESSAGE + helper.markdown_list(changed_files))
+  warn format(TELEMETRY_CHANGED_FILES_MESSAGE, changed_files: helper.markdown_list(changed_files))
 
   telemetry_labels = ['telemetry']
   telemetry_labels << 'telemetry::review pending' unless helper.mr_has_labels?('telemetry::reviewed')