Resolves pending comments from reviewtc-database-review-process

- Addresses documentation feedback
- Refactor Danger methods
- Touches a file to activate Danger. Will remove after testing

7 files changed, 74 insertions, 51 deletions

diff --git a/.gitlab/CODEOWNERS b/.gitlab/CODEOWNERS
index 197365c868a..13c8b4a8458 100644
--- a/.gitlab/CODEOWNERS
+++ b/.gitlab/CODEOWNERS
@@ -9,11 +9,12 @@
 app/assets/ @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann @kushalpandya @pslaughter
 *.scss @annabeldunstone @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann @kushalpandya @pslaughter
 
-# Someone from the database team should review changes in `db/`
+# Maintainers from the Database team should review changes in `db/`
 db/ @abrandl @NikolayS
 lib/gitlab/background_migration/ @abrandl @NikolayS
 lib/gitlab/database/ @abrandl @NikolayS
 lib/gitlab/sql/ @abrandl @NikolayS
+/ee/db/ @abrandl @NikolayS
 
 # Feature specific owners
 /ee/lib/gitlab/code_owners/ @reprazent
diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile
index a6356925315..3550cb7eabf 100644
--- a/danger/database/Dangerfile
+++ b/danger/database/Dangerfile
@@ -46,8 +46,11 @@ if gitlab.mr_labels.include?('database') || db_paths_to_review.any?
   markdown(DB_MESSAGE)
   markdown(DB_FILES_MESSAGE + helper.markdown_list(db_paths_to_review)) if db_paths_to_review.any?
 
-  labels = ['database']
-  labels << 'database::review pending' unless gitlab.mr_labels.any? { |label| label.start_with?('database::') }
+  database_labels = helper.missing_database_labels(gitlab.mr_labels)
 
-  helper.add_labels(labels) unless labels.empty?
+  if database_labels.any?
+    gitlab.api.update_merge_request(gitlab.mr_json['project_id'],
+                                        gitlab.mr_json['iid'],
+                                        labels: (gitlab.mr_labels + database_labels).join(','))
+  end
 end
diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile
index 675e1774780..19a5076778c 100644
--- a/danger/roulette/Dangerfile
+++ b/danger/roulette/Dangerfile
@@ -62,7 +62,7 @@ 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')
+categories << :database if gitlab.mr_labels.include?('database') && !categories.include?(:database)
 
 # Single codebase MRs are reviewed using a slightly different process, so we
 # disable the review roulette for such MRs.
diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md
index 2582bc10722..17328762c5b 100644
--- a/doc/development/contributing/merge_request_workflow.md
+++ b/doc/development/contributing/merge_request_workflow.md
@@ -104,7 +104,7 @@ from the [core team](https://about.gitlab.com/community/core-team/) or one of th
 [merge request coaches](https://about.gitlab.com/team/). When having your code reviewed
 and when reviewing merge requests, please keep the [code review guidelines](../code_review.md)
 in mind. And if your code also makes changes to the database, or does expensive queries,
-check the [database review guidelines](../database_review.md)
+check the [database review guidelines](../database_review.md).
 
 ### Keep it simple
 
diff --git a/doc/development/database_review.md b/doc/development/database_review.md
index e5caefca4ab..1413c2f69fb 100644
--- a/doc/development/database_review.md
+++ b/doc/development/database_review.md
@@ -8,14 +8,14 @@ practices for code review in general.
 
 A database review is required for:
 
-* Changes that touch the database schema or perform data migrations,
+- Changes that touch the database schema or perform data migrations,
   including files in:
-  * `db/`
-  * `lib/gitlab/background_migration/`
-* Changes to the database tooling, e.g.:
-  * migration or ActiveRecord helpers in `lib/gitlab/database/`
-  * load balancing
-* Changes that produce SQL queries that are beyond the obvious. It is
+  - `db/`
+  - `lib/gitlab/background_migration/`
+- Changes to the database tooling, e.g.:
+  - migration or ActiveRecord helpers in `lib/gitlab/database/`
+  - load balancing
+- Changes that produce SQL queries that are beyond the obvious. It is
   generally up to the author of a merge request to decide whether or
   not complex queries are being introduced and if they require a
   database review.
@@ -34,25 +34,25 @@ for review.
 
 A Merge Request author's role is to:
 
-* Decide whether a database review is needed.
-* If database review is needed, add the ~database label.
-* Use the [database changes](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/.gitlab/merge_request_templates/Database%20changes.md)
+- Decide whether a database review is needed.
+- If database review is needed, add the ~database label.
+- 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", approve it, and
+- Perform a first-pass review on the MR and suggest improvements to the author.
+- Once satisfied, relabel the MR with ~"database::reviewed", approve it, and
   reassign MR to the database **maintainer** suggested by Reviewer
   Roulette.
 
 A database **maintainer**'s role is to:
 
-* Perform the final database review on the MR.
-* Discuss further improvements or other relevant changes with the
+- Perform the final database review on the MR.
+- Discuss further improvements or other relevant changes with the
   database reviewer and the MR author.
-* Finally approve the MR and relabel the MR with ~"database::approved"
-* Merge the MR if no other approvals are pending or pass it on to
+- Finally approve the MR and relabel the MR with ~"database::approved"
+- Merge the MR if no other approvals are pending or pass it on to
   other maintainers as required (frontend, backend, docs).
 
 ### Distributing review workload
@@ -70,32 +70,32 @@ make sure you have applied the ~database label and rerun the
 
 ### How to review for database
 
-* Check migrations
-  * Review relational modeling and design choices
-  * Review migrations follow [database migration style guide](migration_style_guide.md),
+- Check migrations
+  - Review relational modeling and design choices
+  - 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
+    - [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](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.
-* Check background migrations
-  * Review queries (for example, make sure batch sizes are fine)
-  * Establish a time estimate for execution
-* Query performance
-  * Check for any obviously complex queries and queries the author specifically
+  - 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.
+- Check background migrations
+  - Review queries (for example, make sure batch sizes are fine)
+  - 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
+  - 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](understanding_explain_plans.md) and suggest improvements
+  - For given queries, review parameters regarding data distribution
+  - [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
+  - 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 ee28b4f7e95..c0a12318990 100644
--- a/lib/gitlab/danger/helper.rb
+++ b/lib/gitlab/danger/helper.rb
@@ -143,17 +143,20 @@ module Gitlab
         usernames.map { |u| Gitlab::Danger::Teammate.new('username' => u) }
       end
 
-      def add_labels(labels)
-        new_labels = labels - gitlab.mr_labels
-
-        return if new_labels.empty?
+      def missing_database_labels(current_mr_labels)
+        labels = if has_database_scoped_labels?(current_mr_labels)
+                   ['database']
+                 else
+                   ['database', 'database::review pending']
+                 end
+
+        labels - current_mr_labels
+      end
 
-        gitlab.api.update_merge_request(gitlab.mr_json['project_id'],
-                                        gitlab.mr_json['iid'],
-                                        labels: (gitlab.mr_labels + new_labels).join(','))
+      private
 
-        message "Automatically applied the labels: " +
-          new_labels.map { |label| %Q[~"#{label}"] }.join(' ')
+      def has_database_scoped_labels?(current_mr_labels)
+        current_mr_labels.any? { |label| label.start_with?('database::') }
       end
     end
   end
diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb
index 09f72e25982..f11f68ab3c2 100644
--- a/spec/lib/gitlab/danger/helper_spec.rb
+++ b/spec/lib/gitlab/danger/helper_spec.rb
@@ -238,4 +238,20 @@ describe Gitlab::Danger::Helper do
       expect(teammates.map(&:username)).to eq(usernames)
     end
   end
+
+  describe '#missing_database_labels' do
+    subject { helper.missing_database_labels(current_mr_labels) }
+
+    context 'when current merge request has ~database::review pending' do
+      let(:current_mr_labels) { ['database::review pending', 'feature'] }
+
+      it { is_expected.to match_array(['database']) }
+    end
+
+    context 'when current merge request does not have ~database::review pending' do
+      let(:current_mr_labels) { ['feature'] }
+
+      it { is_expected.to match_array(['database', 'database::review pending']) }
+    end
+  end
 end