diff options
Diffstat (limited to 'doc/development/contributing/merge_request_workflow.md')
| -rw-r--r-- | doc/development/contributing/merge_request_workflow.md | 104 |
1 files changed, 62 insertions, 42 deletions
diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index faa1642d50a..0e9ac569558 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -35,12 +35,23 @@ and see the [Development section](../../index.md) for the required guidelines. ## Merge request guidelines for contributors -If you find an issue, please submit a merge request with a fix or improvement, if -you can, and include tests. If you don't know how to fix the issue but can write a test -that exposes the issue, we will accept that as well. In general, bug fixes that -include a regression test are merged quickly, while new features without proper -tests might be slower to receive feedback. The workflow to make a merge -request is as follows: +If you find an issue, please submit a merge request with a fix or improvement, +if you can, and include tests. + +If the change is non-trivial, we encourage you to +start a discussion with [a product manager or a member of the team](https://about.gitlab.com/handbook/product/categories/). +You can do +this by tagging them in an MR before submitting the code for review. Talking +to team members can be helpful when making design decisions. Communicating the +intent behind your changes can also help expedite merge request reviews. + +If +you don't know how to fix the issue but can write a test that exposes the +issue, we will accept that as well. In general, bug fixes that include a +regression test are merged quickly. New features without proper tests +might be slower to receive feedback. + +To create a merge request: 1. [Fork](../../user/project/repository/forking_workflow.md) the project into your personal namespace (or group) on GitLab.com. @@ -199,48 +210,27 @@ To reach the definition of done, the merge request must create no regressions an - Verified as working in production on GitLab.com. - Verified as working for self-managed instances. -If a regression occurs, we prefer you revert the change. We break the definition of done into two phases: [MR Merge](#mr-merge) and [Production use](#production-use). +If a regression occurs, we prefer you revert the change. Your contribution is *incomplete* until you have made sure it meets all of these requirements. -### MR Merge +### Functionality -1. Clear title and description explaining the relevancy of the contribution. 1. Working and clean code that is commented where needed. 1. The change is evaluated to [limit the impact of far-reaching work](https://about.gitlab.com/handbook/engineering/development/#reducing-the-impact-of-far-reaching-work). -1. Testing: - - - [Unit, integration, and system tests](../testing_guide/index.md) that all pass - on the CI server. - - Peer member testing is optional but recommended when the risk of a change is high. - This includes when the changes are [far-reaching](https://about.gitlab.com/handbook/engineering/development/#reducing-the-impact-of-far-reaching-work) - or are for [components critical for security](../code_review.md#security). - - Description includes any steps or setup required to ensure reviewers can view the changes you've made (for example, include any information about feature flags). - - Regressions and bugs are covered with tests that reduce the risk of the issue happening - again. - - For tests that use Capybara, read - [how to write reliable, asynchronous integration tests](https://thoughtbot.com/blog/write-reliable-asynchronous-integration-tests-with-capybara). - - [Black-box tests/end-to-end tests](../testing_guide/testing_levels.md#black-box-tests-at-the-system-level-aka-end-to-end-tests) - added if required. Please contact [the quality team](https://about.gitlab.com/handbook/engineering/quality/#teams) - with any questions. - - The change is tested in a review app where possible and if appropriate. -1. In case of UI changes: - - - Use available components from the GitLab Design System, - [Pajamas](https://design.gitlab.com/). - - The MR must include *Before* and *After* screenshots if UI changes are made. - - If the MR changes CSS classes, please include the list of affected pages, which - can be found by running `grep css-class ./app -R`. +1. [Performance guidelines](../merge_request_performance_guidelines.md) have been followed. +1. [Secure coding guidelines](https://gitlab.com/gitlab-com/gl-security/security-guidelines) have been followed. +1. [Application and rate limit guidelines](../merge_request_application_and_rate_limit_guidelines.md) have been followed. +1. [Documented](../documentation/index.md) in the `/doc` directory. 1. If your MR touches code that executes shell commands, reads or opens files, or handles paths to files on disk, make sure it adheres to the [shell command guidelines](../shell_commands.md) 1. [Code changes should include observability instrumentation](../code_review.md#observability-instrumentation). 1. If your code needs to handle file storage, see the [uploads documentation](../uploads/index.md). -1. If your merge request adds one or more migrations: - - Make sure to execute all migrations on a fresh database before the MR is reviewed. - If the review leads to large changes in the MR, execute the migrations again - after the review is complete. - - Write tests for more complex migrations. +1. If your merge request adds one or more migrations, make sure to execute all migrations on a fresh database + before the MR is reviewed. + If the review leads to large changes in the MR, execute the migrations again + after the review is complete. 1. If your merge request adds new validations to existing models, to make sure the data processing is backwards compatible: @@ -259,12 +249,37 @@ requirements. [self-managed instances](../../subscriptions/self_managed/index.md), so keep that in mind for any data implications with your merge request. +### Testing + +1. [Unit, integration, and system tests](../testing_guide/index.md) that all pass + on the CI server. +1. Peer member testing is optional but recommended when the risk of a change is high. + This includes when the changes are [far-reaching](https://about.gitlab.com/handbook/engineering/development/#reducing-the-impact-of-far-reaching-work) + or are for [components critical for security](../code_review.md#security). +1. Regressions and bugs are covered with tests that reduce the risk of the issue happening + again. +1. For tests that use Capybara, read + [how to write reliable, asynchronous integration tests](https://thoughtbot.com/blog/write-reliable-asynchronous-integration-tests-with-capybara). +1. [Black-box tests/end-to-end tests](../testing_guide/testing_levels.md#black-box-tests-at-the-system-level-aka-end-to-end-tests) + added if required. Please contact [the quality team](https://about.gitlab.com/handbook/engineering/quality/#teams) + with any questions. +1. The change is tested in a review app where possible and if appropriate. 1. Code affected by a feature flag is covered by [automated tests with the feature flag enabled and disabled](../feature_flags/index.md#feature-flags-in-tests), or both states are tested as part of peer member testing or as part of the rollout plan. -1. [Performance guidelines](../merge_request_performance_guidelines.md) have been followed. -1. [Secure coding guidelines](https://gitlab.com/gitlab-com/gl-security/security-guidelines) have been followed. -1. [Application and rate limit guidelines](../merge_request_application_and_rate_limit_guidelines.md) have been followed. -1. [Documented](../documentation/index.md) in the `/doc` directory. +1. If your merge request adds one or more migrations, write tests for more complex migrations. + +### UI changes + +1. Use available components from the GitLab Design System, + [Pajamas](https://design.gitlab.com/). +1. The MR must include *Before* and *After* screenshots if UI changes are made. +1. If the MR changes CSS classes, please include the list of affected pages, which + can be found by running `grep css-class ./app -R`. + +### Description of changes + +1. Clear title and description explaining the relevancy of the contribution. +1. Description includes any steps or setup required to ensure reviewers can view the changes you've made (for example, include any information about feature flags). 1. [Changelog entry added](../changelog.md), if necessary. 1. If your merge request introduces changes that require additional steps when installing GitLab from source, add them to `doc/install/installation.md` in @@ -274,10 +289,13 @@ requirements. `doc/update/upgrading_from_source.md` in the same merge request. If these instructions are specific to a version, add them to the "Version specific upgrading instructions" section. -1. Reviewed by relevant reviewers, and all concerns are addressed for Availability, Regressions, and Security. Documentation reviews should take place as soon as possible, but they should not block a merge request. + +### Approval + 1. The [MR acceptance checklist](../code_review.md#acceptance-checklist) has been checked as confirmed in the MR. 1. Create an issue in the [infrastructure issue tracker](https://gitlab.com/gitlab-com/gl-infra/reliability/-/issues) to inform the Infrastructure department when your contribution is changing default settings or introduces a new setting, if relevant. 1. An agreed-upon [rollout plan](https://about.gitlab.com/handbook/engineering/development/processes/rollout-plans/). +1. Reviewed by relevant reviewers, and all concerns are addressed for Availability, Regressions, and Security. Documentation reviews should take place as soon as possible, but they should not block a merge request. 1. Your merge request has at least 1 approval, but depending on your changes you might need additional approvals. Refer to the [Approval guidelines](../code_review.md#approval-guidelines). - You don't have to select any specific approvers, but you can if you really want @@ -286,6 +304,8 @@ requirements. ### Production use +The following items are checked after the merge request has been merged: + 1. Confirmed to be working in staging before implementing the change in production, where possible. 1. Confirmed to be working in the production with no new [Sentry](https://about.gitlab.com/handbook/engineering/monitoring/#sentry) errors after the contribution is deployed. 1. Confirmed that the [rollout plan](https://about.gitlab.com/handbook/engineering/development/processes/rollout-plans/) has been completed. |