Add latest changes from gitlab-org/gitlab@16-7-stable-eev16.7.0-rc42

6 files changed, 121 insertions, 45 deletions

diff --git a/danger/documentation/Dangerfile b/danger/documentation/Dangerfile
index 78f8c87a528..f7fa0d9fcf2 100644
--- a/danger/documentation/Dangerfile
+++ b/danger/documentation/Dangerfile
@@ -8,45 +8,85 @@ def doc_path_to_url(path)
   path.sub("doc/", "https://docs.gitlab.com/ee/").sub("index.md", "").sub(".md", "/")
 end
 
-DOCUMENTATION_UPDATE_MISSING = <<~MSG
-~"feature::addition" and ~"feature::enhancement" merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the [Technical Writer counterpart](https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments).
+docs_paths_to_review = helper.changes_by_category[:docs]
 
-For more information, see:
+# Some docs do not need a review from a Technical Writer.
+# In these cases, we'll output a message specific to the section.
+sections_with_no_tw_review = {
+  'doc/solutions' => [],
+  'doc/development' => []
+}.freeze
 
-- The Handbook page on [merge request types](https://about.gitlab.com/handbook/engineering/metrics/#work-type-classification).
-- The [definition of done](https://docs.gitlab.com/ee/development/contributing/merge_request_workflow.html#definition-of-done) documentation.
-MSG
+# One exception to the exceptions above: Technical Writing docs should get a TW review.
+TW_DOCS_PATH = 'doc/development/documentation'
 
-docs_paths_to_review = helper.changes_by_category[:docs]
+docs_paths_to_review.reject! do |doc|
+  section_with_no_tw_review = sections_with_no_tw_review.keys.find { |skip_path| doc.start_with?(skip_path) && !doc.start_with?(TW_DOCS_PATH) }
+  next unless section_with_no_tw_review
 
-# Some docs do not need a review from a technical writer
-SKIP_TW_REVIEW_PATHS = ['doc/solutions'].freeze
+  sections_with_no_tw_review[section_with_no_tw_review] << doc
 
-docs_paths_to_review.delete_if do |item|
-  SKIP_TW_REVIEW_PATHS.any? { |skip_path| item.start_with?(skip_path) }
+  true
 end
 
-# Documentation should be updated for feature::addition and feature::enhancement
-if docs_paths_to_review.empty?
-  warn(DOCUMENTATION_UPDATE_MISSING) if feature_mr?
+SOLUTIONS_LABELS = %w[Solutions].freeze
+DEVELOPMENT_LABELS = ['docs::improvement', 'development guidelines'].freeze
 
-  return
+def add_labels(labels)
+  helper.labels_to_add.concat(%w[documentation type::maintenance maintenance::refactor] + labels)
 end
 
-message 'This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is **recommended**. Reviews can happen after you merge.'
+SOLUTIONS_MESSAGE = <<~MSG
+This MR contains docs in the /solutions directory and should be reviewed by a Solutions Architect approver. You do not need tech writer review.
+MSG
+
+DEVELOPMENT_MESSAGE = <<~MSG
+This MR contains docs in the /development directory. Any Maintainer, other than the author, can merge. You do not need tech writer review.
+MSG
 
-return unless helper.ci?
+# For regular pages, prompt for a TW review
+DOCS_UPDATE_SHORT_MESSAGE = <<~MSG
+This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is **recommended**. Reviews can happen after you merge.
+MSG
 
-markdown(<<~MARKDOWN)
-  ## Documentation review
+DOCS_UPDATE_LONG_MESSAGE = <<~MSG.freeze
+## Documentation review
 
-  The following files require a review from a technical writer:
+The following files require a review from a technical writer:
 
-  * #{docs_paths_to_review.map { |path| "`#{path}` ([Link to current live version](#{doc_path_to_url(path)}))" }.join("\n* ")}
+* #{docs_paths_to_review.map { |path| "`#{path}` ([Link to current live version](#{doc_path_to_url(path)}))" }.join("\n* ")}
 
-  The review does not need to block merging this merge request. See the:
+The review does not need to block merging this merge request. See the:
 
-  - [Metadata for the `*.md` files](https://docs.gitlab.com/ee/development/documentation/#metadata) that you've changed. The first few lines of each `*.md` file identify the stage and group most closely associated with your docs change.
-  - The [Technical Writer assigned](https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments) for that stage and group.
-  - [Documentation workflows](https://docs.gitlab.com/ee/development/documentation/workflow.html) for information on when to assign a merge request for review.
-MARKDOWN
+- [Metadata for the `*.md` files](https://docs.gitlab.com/ee/development/documentation/#metadata) that you've changed. The first few lines of each `*.md` file identify the stage and group most closely associated with your docs change.
+- The [Technical Writer assigned](https://handbook.gitlab.com/handbook/product/ux/technical-writing/#assignments) for that stage and group.
+- [Documentation workflows](https://docs.gitlab.com/ee/development/documentation/workflow.html) for information on when to assign a merge request for review.
+MSG
+
+# Documentation should be updated for feature::addition and feature::enhancement
+DOCUMENTATION_UPDATE_MISSING = <<~MSG
+~"feature::addition" and ~"feature::enhancement" merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the [Technical Writer counterpart](https://handbook.gitlab.com/handbook/product/ux/technical-writing/#assignments).
+
+For more information, see:
+
+- The Handbook page on [merge request types](https://about.gitlab.com/handbook/engineering/metrics/#work-type-classification).
+- The [definition of done](https://docs.gitlab.com/ee/development/contributing/merge_request_workflow.html#definition-of-done) documentation.
+MSG
+
+# Output messages
+warn(DOCUMENTATION_UPDATE_MISSING) if docs_paths_to_review.empty? && feature_mr?
+
+if sections_with_no_tw_review["doc/solutions"].any?
+  add_labels(SOLUTIONS_LABELS)
+  message(SOLUTIONS_MESSAGE)
+end
+
+if sections_with_no_tw_review["doc/development"].any?
+  add_labels(DEVELOPMENT_LABELS)
+  message(DEVELOPMENT_MESSAGE)
+end
+
+unless docs_paths_to_review.empty?
+  message(DOCS_UPDATE_SHORT_MESSAGE)
+  markdown(DOCS_UPDATE_LONG_MESSAGE)
+end
diff --git a/danger/plugins/todos.rb b/danger/plugins/todos.rb
index b31f147f2af..06b76321f95 100644
--- a/danger/plugins/todos.rb
+++ b/danger/plugins/todos.rb
@@ -5,7 +5,11 @@ require_relative '../../tooling/danger/outdated_todo'
 module Danger
   class Todos < ::Danger::Plugin
     def check_outdated_todos(filenames)
-      Tooling::Danger::OutdatedTodo.new(filenames, context: self).check
+      Tooling::Danger::OutdatedTodo.new(filenames, context: self, allow_fail: from_lefthook?).check
+    end
+
+    def from_lefthook?
+      %w[1 true].include?(ENV['FROM_LEFTHOOK'])
     end
   end
 end
diff --git a/danger/qa_selector/Dangerfile b/danger/qa_selector/Dangerfile
index c8450bffe27..98417e79782 100644
--- a/danger/qa_selector/Dangerfile
+++ b/danger/qa_selector/Dangerfile
@@ -7,39 +7,48 @@ data_testids = /testid/
 deprecated_qa_selectors = /(?=qa_selector|data-qa-selector)|(?!.*\bdata-qa-)(?=class=.*qa-.*|class: .*qa-.*)/
 
 def filter_changed_lines(files, pattern)
-  lines = []
+  files_with_lines = {}
   files.each do |file|
     next if file.start_with?('spec/', 'ee/spec/', 'qa/')
 
     testid_changed_lines = helper.changed_lines(file).select { |line| line =~ pattern }
     next unless testid_changed_lines.any?
 
-    lines += ["file `#{file}`:", testid_changed_lines]
+    files_with_lines[file] = testid_changed_lines
   end
-  lines
+
+  files_with_lines
 end
 
 changed_code_files = helper.changed_files(/\.(vue|haml|js|rb)$/)
 
 return if changed_code_files.empty?
 
-lines_with_testids = filter_changed_lines(changed_code_files, data_testids)
-
+lines_with_testids  = filter_changed_lines(changed_code_files, data_testids)
 deprecated_qa_class = filter_changed_lines(changed_code_files, deprecated_qa_selectors)
 
-return if (lines_with_testids + deprecated_qa_class).empty?
-
-markdown(<<~MARKDOWN)
-  ## Testid Selectors
-
-MARKDOWN
+return if lines_with_testids.empty? && deprecated_qa_class.empty?
 
 if lines_with_testids.any?
   markdown(<<~MARKDOWN)
-    The following changed lines in this MR contain testid selectors:
+    ### `testid` selectors
+
+    The following changed lines in this MR contain `testid` selectors:
 
-    * #{lines_with_testids.join("\n* ")}
+  MARKDOWN
+
+  lines_with_testids.each do |file, lines|
+    markdown(<<~MARKDOWN)
+      #### `#{file}`
+
+      ```shell
+      #{lines.join("\n")}
+      ```
+
+    MARKDOWN
+  end
 
+  markdown(<<~MARKDOWN)
     If the `e2e:package-and-test` job in the `qa` stage has run automatically, please ensure the tests are passing.
     If the job has not run, please start the `trigger-omnibus-and-follow-up-e2e` job in the `qa` stage and ensure the tests in `follow-up-e2e:package-and-test-ee` pipeline are passing.
 
@@ -54,12 +63,27 @@ end
 
 if deprecated_qa_class.any?
   markdown(<<~MARKDOWN)
-    ### Deprecated data-qa-selector
+    ### Deprecated `data-qa-selector` selectors
+
+  MARKDOWN
+
+  markdown(<<~MARKDOWN)
+    The following lines in this MR contain deprecated `data-qa-selector` selectors:
 
-    The following lines in this MR contain deprecated data-qa-selector selectors:
+  MARKDOWN
+
+  deprecated_qa_class.each do |file, lines|
+    markdown(<<~MARKDOWN)
+      #### `#{file}`
 
-    * #{deprecated_qa_class.join("\n* ")}
+      ```shell
+      #{lines.join("\n")}
+      ```
 
+    MARKDOWN
+  end
+
+  markdown(<<~MARKDOWN)
     Please ensure all deprecated data-qa-selector attributes are replaced with data-testid attributes in accordance with our [Testing Guide](https://docs.gitlab.com/ee/development/testing_guide/end_to_end/page_objects.html#data-testid-vs-data-qa-selector).
 
   MARKDOWN
diff --git a/danger/rubocop/Dangerfile b/danger/rubocop/Dangerfile
index 41131241691..b43a1042bd5 100644
--- a/danger/rubocop/Dangerfile
+++ b/danger/rubocop/Dangerfile
@@ -1,5 +1,9 @@
 # frozen_string_literal: true
 
+# This is noisy for draft MRs, so let's ignore this cop in draft mode since we have
+# rubocop watching this as well.
+return if helper.draft_mr?
+
 # Danger should not comment when inline disables are added in the following files.
 no_suggestions_for_extensions = %w[.md]
 
diff --git a/danger/saas_feature/Dangerfile b/danger/saas_feature/Dangerfile
index 38ca87fb5fd..159201f8904 100644
--- a/danger/saas_feature/Dangerfile
+++ b/danger/saas_feature/Dangerfile
@@ -13,7 +13,11 @@ SUGGEST_COMMENT
 def check_yaml(saas_feature)
   mr_group_label = helper.group_label
 
-  message_for_missing_group!(saas_feature: saas_feature, mr_group_label: mr_group_label) if saas_feature.group.nil?
+  if saas_feature.group.nil?
+    message_for_missing_group!(saas_feature: saas_feature, mr_group_label: mr_group_label)
+  else
+    message_for_group!(saas_feature: saas_feature, mr_group_label: mr_group_label)
+  end
 rescue Psych::Exception
   # YAML could not be parsed, fail the build.
   fail "#{helper.html_link(saas_feature.path)} isn't valid YAML! #{SEE_DOC.capitalize}"
diff --git a/danger/stable_branch_patch/Dangerfile b/danger/stable_branch_patch/Dangerfile
index a4b557c1eaa..e5635f3ae26 100644
--- a/danger/stable_branch_patch/Dangerfile
+++ b/danger/stable_branch_patch/Dangerfile
@@ -7,7 +7,7 @@ if stable_branch.encourage_package_and_qa_execution?
   **@#{helper.mr_author}, the `package-and-test` job must complete before merging this merge request.***
 
   If there are failures on the `package-and-test` pipeline, ping your team's associated Software Engineer in Test (SET) to confirm
-  the failures are unrelated to the merge request. If there's no SET assigned, ask for assistance on the `#quality` Slack channel.
+  the failures are unrelated to the merge request. If there's no SET assigned, ask for assistance on the `#test-platform` Slack channel.
   MARKDOWN
 end