Further improve database review processdatabase-review-docs

Based on the comments, we're dropping the use of a "Database Review"
issue, and I'm improve Danger more to get database reviewers suggested
by roulette.

7 files changed, 75 insertions, 70 deletions

diff --git a/.gitlab/merge_request_templates/Database changes.md b/.gitlab/merge_request_templates/Database changes.md
index 3f457174492..2077997a0cb 100644
--- a/.gitlab/merge_request_templates/Database changes.md
+++ b/.gitlab/merge_request_templates/Database changes.md
@@ -39,20 +39,11 @@ When adding tables:
 
 - [ ] Ordered columns based on the [Ordering Table Columns](https://docs.gitlab.com/ee/development/ordering_table_columns.html#ordering-table-columns) guidelines
 - [ ] Added foreign keys to any columns pointing to data in other tables
-- [ ] Added indexes for fields that are used in statements such as WHERE, ORDER BY, GROUP BY, and JOINs
+- [ ] Added indexes for fields that are used in statements such as `WHERE`, `ORDER BY`, `GROUP BY`, and `JOIN`s
 
 When removing columns, tables, indexes or other structures:
 
 - [ ] Removed these in a post-deployment migration
 - [ ] Made sure the application no longer uses (or ignores) these structures
 
-## General checklist
-
-- [ ] [Changelog entry](https://docs.gitlab.com/ee/development/changelog.html) added, if necessary
-- [ ] [Documentation created/updated](https://docs.gitlab.com/ee/development/documentation/)
-- [ ] [Tests added for this feature/bug](https://docs.gitlab.com/ee/development/testing_guide/index.html)
-- [ ] Conforms to the [code review guidelines](https://docs.gitlab.com/ee/development/code_review.html)
-- [ ] Conforms to the [merge request performance guidelines](https://docs.gitlab.com/ee/development/merge_request_performance_guidelines.html)
-- [ ] Conforms to the [style guides](https://docs.gitlab.com/ee/development/contributing/style_guides.html)
-
-/label ~database
+/label ~database ~"database::review pending"
diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile
index 083e95b8da7..f3dc841c7f0 100644
--- a/danger/database/Dangerfile
+++ b/danger/database/Dangerfile
@@ -8,6 +8,20 @@ updated too (unless the migration isn't changing the DB schema
 and isn't the most recent one).
 MSG
 
+DB_MESSAGE = <<~MSG
+This merge request requires a database review. To make sure these
+changes are reviewed, take the following steps:
+
+1. Add labels ~database and ~"database::review pending" to your merge request.
+1. Use the [Database%20changes checklist](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab/merge_request_templates/Database%20changes.md)
+   template or add the appropriate items to the MR description.
+1. Assign and mention the database reviewer suggested by Reviewer Roulette.
+MSG
+
+DB_FILES_MESSAGE = <<~MSG
+The following files require a review from the Database team:
+MSG
+
 non_geo_db_schema_updated = !git.modified_files.grep(%r{\Adb/schema\.rb}).empty?
 geo_db_schema_updated = !git.modified_files.grep(%r{\Aee/db/geo/schema\.rb}).empty?
 
@@ -24,27 +38,18 @@ end
 
 db_paths_to_review = helper.changes_by_category[:database]
 
-unless db_paths_to_review.empty?
+if gitlab.mr_labels.include?('database') || db_paths_to_review.any?
   message 'This merge request adds or changes files that require a ' \
-    'review from the Database team.'
-
-  markdown(<<~MARKDOWN.strip)
-## Database Review
+    'review from the [Database team](https://gitlab.com/groups/gl-database/-/group_members).'
 
-The following files require a review from the Database team:
-
-* #{db_paths_to_review.map { |path| "`#{path}`" }.join("\n* ")}
-
-To make sure these changes are reviewed, take the following steps:
-
-1. Edit your merge request, and add `gl-database` to the list of Group
-   approvers.
-1. Mention `@gl-database` in a separate comment, and explain what needs to be
-   reviewed by the team. Please don't mention the team until your changes are
-   ready for review.
-  MARKDOWN
+  markdown(DB_MESSAGE)
+  markdown(DB_FILES_MESSAGE + helper.markdown_list(db_paths_to_review)) if db_paths_to_review.any?
 
   unless gitlab.mr_labels.include?('database')
     warn 'This merge request is missing the ~database label.'
   end
+
+  unless gitlab.mr_labels.any? { |label| label.start_with?('database::') }
+    warn 'This merge request is missing the ~"database::review pending" label.'
+  end
 end
diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile
index 6718e218233..675e1774780 100644
--- a/danger/roulette/Dangerfile
+++ b/danger/roulette/Dangerfile
@@ -55,22 +55,15 @@ def spin_for_category(team, project, category, branch_name)
   "| #{helper.label_for_category(category)} | #{reviewer&.markdown_name || NO_REVIEWER} | #{maintainer&.markdown_name || NO_MAINTAINER} |"
 end
 
-def build_list(items)
-  list = items.map { |filename| "* `#{filename}`" }.join("\n")
-
-  if items.size > 10
-    "\n<details>\n\n#{list}\n\n</details>"
-  else
-    list
-  end
-end
-
 changes = helper.changes_by_category
 
 # Ignore any files that are known but uncategorized. Prompt for any unknown files
 changes.delete(:none)
 categories = changes.keys - [:unknown]
 
+# Ensure to spin for database reviewer/maintainer when ~database is applied (e.g. to review SQL queries)
+categories |= [:database] if gitlab.mr_labels.include?('database')
+
 # Single codebase MRs are reviewed using a slightly different process, so we
 # disable the review roulette for such MRs.
 # CSS Clean up MRs are reviewed using a slightly different process, so we
@@ -95,5 +88,5 @@ if changes.any? && !gitlab.mr_labels.include?('single codebase') && !gitlab.mr_l
 
   markdown(MESSAGE)
   markdown(CATEGORY_TABLE_HEADER + rows.join("\n")) unless rows.empty?
-  markdown(UNKNOWN_FILES_MESSAGE + build_list(unknown)) unless unknown.empty?
+  markdown(UNKNOWN_FILES_MESSAGE + helper.markdown_list(unknown)) unless unknown.empty?
 end
diff --git a/doc/development/code_review.md b/doc/development/code_review.md
index 6123f9f845a..51911cb7d13 100644
--- a/doc/development/code_review.md
+++ b/doc/development/code_review.md
@@ -62,6 +62,7 @@ from teams other than your own.
       **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_backend)**.
    1. If your merge request includes database migrations or changes to expensive queries [^2], it must be
       **approved by a [database maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_database)**.
+      Read the [database review guidelines](database_review.md) for more details.
    1. If your merge request includes frontend changes [^1], it must be
       **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_frontend)**.
    1. If your merge request includes UX changes [^1], it must be
diff --git a/doc/development/database_review.md b/doc/development/database_review.md
index 1fb5ff2e76e..80fff5a129e 100644
--- a/doc/development/database_review.md
+++ b/doc/development/database_review.md
@@ -34,56 +34,47 @@ for review.
 
 A Merge Request author's role is to:
 
-* Decide whether a database review is needed and apply ~database and ~"database::review pending" if so.
+* Decide whether a database review is needed.
+* If database review is needed, apply ~database and ~"database::review pending" labels.
+* Use the [database changes](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab/merge_request_templates/Database%20changes.md)
+  merge request template, or include the appropriate items in the MR description.
 
 A database **reviewer**'s role is to:
 
 * Perform a first-pass review on the MR and suggest improvements to the author.
-* Once satisfied, relabel the MR with ~"database::reviewed" and reassign MR to a database **maintainer**.
+* Once satisfied, relabel the MR with ~"database::reviewed" and
+  reassign MR to the database **maintainer** suggested by Reviewer
+  Roulette.
 
-A database **maintainer**'s role is to
+A database **maintainer**'s role is to:
 
-* Perform another database review of the MR.
+* Perform the final database review on the MR.
 * Discuss further improvements or other relevant changes with the database reviewer and the MR author.
-* Finally relabel the MR with ~"database::approved"
-* And if requested, approve or merge the MR, or pass it on to other maintainers as required (frontend, backend, docs).
+* Finally Approve the MR relabel the MR with ~"database::approved"
+* And if requested, approve and merge the MR, or pass it on to other maintainers as required (frontend, backend, docs).
 
 ### Distributing review workload
 
-Ideally, review workload is distributed using our review roulette
+Review workload is distributed using [reviewer roulette](code_review.md#reviewer-roulette)
 ([example](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/25181#note_147551725)).
 The MR author should then co-assign the suggested database
 **reviewer**. When they give their sign-off, they will hand over to
 the suggested database **maintainer**.
 
-However, some changes are not yet automatically detected to need a
-database review. For those changes, MR authors typically mention
-`@gl-database` in order to get a review.
-
-Until there are [Merge boards](https://gitlab.com/gitlab-org/gitlab-ce/issues/35336),
-a list of pending database reviews is maintained in an issue titled
-"Database Reviews" in the [infrastructure tracker](https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues?scope=all&utf8=%E2%9C%93&state=opened&label_name[]=Database&search=Database+Reviews)
-([example](https://gitlab.com/gitlab-com/gl-infra/infrastructure/issues/6998)).
-Note the issue rolls over after two weeks and a new issue is created.
-
-The Database Reviews issue contains a list of pending database
-reviews. Database **reviewers** and **maintainers** should work from
-the list of pending database reviews and
-
-1. Pick an MR from the list that doesn't have a person attached to it.
-1. Edit the description of the issue and put your handle next the MR.
-1. Proceed with reviewing the MR in question.
-1. Once review has been completed - check the box.
+If reviewer roulette didn't suggest a database reviewer & maintainer,
+make sure you have applied the ~database label and rerun the
+`danger-review` CI job, or pick someone from the
+[`@gl-database` team](https://gitlab.com/groups/gl-database/-/group_members).
 
 ### How to review for database
 
 * Check migrations
   * Review relational modeling and design choices
-  * Review migrations follow [database migration style guide](https://docs.gitlab.com/ee/development/migration_style_guide.html), for example
-    * [Check ordering of columns](https://docs.gitlab.com/ee/development/ordering_table_columns.html)
-    * [Check indexes are present for foreign keys](https://docs.gitlab.com/ee/development/migration_style_guide.html#adding-foreign-key-constraints)
+  * Review migrations follow [database migration style guide](migration_style_guide.md), for example
+    * [Check ordering of columns](ordering_table_columns.md)
+    * [Check indexes are present for foreign keys](migration_style_guide.md#adding-foreign-key-constraints)
   * Ensure that migrations execute in a transaction or only contain concurrent index/foreign key helpers (with transactions disabled)
-  * Check consistency with `db/schema.rb` and that migrations are [reversible](https://docs.gitlab.com/ee/development/migration_style_guide.html#reversibility)
+  * Check consistency with `db/schema.rb` and that migrations are [reversible](migration_style_guide.md#reversibility)
   * For data migrations, establish a time estimate for execution
 * Check post deploy migration
   * Make sure we can expect post deploy migrations to finish within 1 hour max.
@@ -92,8 +83,8 @@ the list of pending database reviews and
   * Establish a time estimate for execution
 * Query performance
   * Check for any obviously complex queries and queries the author specifically points out for review (if any)
-  * If not present yet, ask the author to provide SQL queries and query plans (e.g. by using [chatops](https://docs.gitlab.com/ee/development/understanding_explain_plans.html) or direct database access)
+  * If not present yet, ask the author to provide SQL queries and query plans (e.g. by using [chatops](understanding_explain_plans.md#chatops) or direct database access)
   * For given queries, review parameters regarding data distribution
-  * [Check query plans](https://docs.gitlab.com/ee/development/understanding_explain_plans.html) and suggest improvements to queries (changing the query, schema or adding indexes and similar)
+  * [Check query plans](understanding_explain_plans.md) and suggest improvements to queries (changing the query, schema or adding indexes and similar)
   * General guideline is for queries to come in below 100ms execution time
   * If queries rely on prior migrations that are not present yet on production (eg indexes, columns), you can use a [one-off instance from the restore pipeline](https://ops.gitlab.net/gitlab-com/gl-infra/gitlab-restore/postgres-gprd) in order to establish a proper testing environment.
diff --git a/lib/gitlab/danger/helper.rb b/lib/gitlab/danger/helper.rb
index 0fc145534bf..f297cff93c2 100644
--- a/lib/gitlab/danger/helper.rb
+++ b/lib/gitlab/danger/helper.rb
@@ -46,6 +46,16 @@ module Gitlab
         ee? ? 'gitlab-ee' : 'gitlab-ce'
       end
 
+      def markdown_list(items)
+        list = items.map { |item| "* `#{item}`" }.join("\n")
+
+        if items.size > 10
+          "\n<details>\n\n#{list}\n\n</details>\n"
+        else
+          list
+        end
+      end
+
       # @return [Hash<String,Array<String>>]
       def changes_by_category
         all_changed_files.each_with_object(Hash.new { |h, k| h[k] = [] }) do |file, hash|
diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb
index 92d90ac2fef..09f72e25982 100644
--- a/spec/lib/gitlab/danger/helper_spec.rb
+++ b/spec/lib/gitlab/danger/helper_spec.rb
@@ -85,6 +85,20 @@ describe Gitlab::Danger::Helper do
     end
   end
 
+  describe '#markdown_list' do
+    it 'creates a markdown list of items' do
+      items = %w[a b]
+
+      expect(helper.markdown_list(items)).to eq("* `a`\n* `b`")
+    end
+
+    it 'wraps items in <details> when there are more than 10 items' do
+      items = ('a'..'k').to_a
+
+      expect(helper.markdown_list(items)).to match(%r{<details>[^<]+</details>})
+    end
+  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/foo lib/gitlab/database/foo.rb qa/foo ee/changelogs/foo.yml] }