summaryrefslogtreecommitdiff
path: root/doc/development
diff options
context:
space:
mode:
authorMayra Cabrera <mcabrera@gitlab.com>2019-07-17 10:54:52 -0500
committerMayra Cabrera <mcabrera@gitlab.com>2019-07-19 09:24:30 -0500
commitd8315b9866a496a47871c77f232aa686c42345ed (patch)
tree6d00487c33411b5e32cb1503597c8c2c6f26edf7 /doc/development
parent0acbdf9c1c03648ee6aac7b771af62d345b21d64 (diff)
downloadgitlab-ce-tc-database-review-process.tar.gz
Resolves pending comments from reviewtc-database-review-process
- Addresses documentation feedback - Refactor Danger methods - Touches a file to activate Danger. Will remove after testing
Diffstat (limited to 'doc/development')
-rw-r--r--doc/development/contributing/merge_request_workflow.md2
-rw-r--r--doc/development/database_review.md72
2 files changed, 37 insertions, 37 deletions
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.