diff options
| -rw-r--r-- | doc/development/code_review.md | 47 |
1 files changed, 47 insertions, 0 deletions
diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 4ed89146072..5f732ee7ad6 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -133,6 +133,53 @@ 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: + 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 version, 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]. |