summaryrefslogtreecommitdiff
path: root/doc/development
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2017-07-24 15:17:20 +0000
committerRémy Coutable <remy@rymai.me>2017-07-24 15:17:20 +0000
commitfe10d1a1d9780cdceb73f6610f98a9707db81352 (patch)
tree2e2f221a8909fc1b62d496cbc78eaec8a8ae0c9c /doc/development
parentc2c8d7ffabaeb6514f7bf15fb5330c8402d80333 (diff)
parente597fa613d4c68857f73e07533fadee66df5d012 (diff)
downloadgitlab-ce-fe10d1a1d9780cdceb73f6610f98a9707db81352.tar.gz
Merge branch 'enhance-backend-review-guide' into 'master'
Add GitLab-specific concerns to code review guide Closes #35099 See merge request !12999
Diffstat (limited to 'doc/development')
-rw-r--r--doc/development/code_review.md49
1 files changed, 49 insertions, 0 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md
index 4ed89146072..e3f37616757 100644
--- a/doc/development/code_review.md
+++ b/doc/development/code_review.md
@@ -133,6 +133,55 @@ reviewee.
tomorrow. When you are not able to find the right balance, ask other people
about their opinion.
+### GitLab-specific concerns
+
+GitLab is used in a lot of places. Many users use
+our [Omnibus packages](https://about.gitlab.com/installation/), but some use
+the [Docker images](https://docs.gitlab.com/omnibus/docker/), some are
+[installed from source](https://docs.gitlab.com/ce/install/installation.html),
+and there are other installation methods available. GitLab.com itself is a large
+Enterprise Edition instance. This has some implications:
+
+1. **Query changes** should be tested to ensure that they don't result in worse
+ performance at the scale of GitLab.com:
+ 1. Generating large quantities of data locally can help.
+ 2. Asking for query plans from GitLab.com is the most reliable way to validate
+ these.
+2. **Database migrations** must be:
+ 1. Reversible.
+ 2. Performant at the scale of GitLab.com - ask a maintainer to test the
+ migration on the staging environment if you aren't sure.
+ 3. Categorised correctly:
+ - Regular migrations run before the new code is running on the instance.
+ - [Post-deployment migrations](post_deployment_migrations.md) run _after_
+ the new code is deployed, when the instance is configured to do that.
+ - [Background migrations](background_migrations.md) run in Sidekiq, and
+ should only be done for migrations that would take an extreme amount of
+ time at GitLab.com scale.
+3. **Sidekiq workers**
+ [cannot change in a backwards-incompatible way](sidekiq_style_guide.md#removing-or-renaming-queues):
+ 1. Sidekiq queues are not drained before a deploy happens, so there will be
+ workers in the queue from the previous version of GitLab.
+ 2. If you need to change a method signature, try to do so across two releases,
+ and accept both the old and new arguments in the first of those.
+ 3. Similarly, if you need to remove a worker, stop it from being scheduled in
+ one release, then remove it in the next. This will allow existing jobs to
+ execute.
+ 4. Don't forget, not every instance will upgrade to every intermediate version
+ (some people may go from X.1.0 to X.10.0, or even try bigger upgrades!), so
+ try to be liberal in accepting the old format if it is cheap to do so.
+4. **Cached values** may persist across releases. If you are changing the type a
+ cached value returns (say, from a string or nil to an array), change the
+ cache key at the same time.
+5. **Settings** should be added as a
+ [last resort](https://about.gitlab.com/handbook/product/#convention-over-configuration).
+ If you're adding a new setting in `gitlab.yml`:
+ 1. Try to avoid that, and add to `ApplicationSetting` instead.
+ 2. Ensure that it is also
+ [added to Omnibus](https://docs.gitlab.com/omnibus/settings/gitlab.yml.html#adding-a-new-setting-to-gitlab-yml).
+6. **Filesystem access** can be slow, so try to avoid
+ [shared files](shared_files.md) when an alternative solution is available.
+
### Credits
Largely based on the [thoughtbot code review guide].