diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index 456fedc157e6d9d12e9ece971141057272ffe2ff..0e9ac56955836c7438379535aecc5e20cb2764ec 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -210,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: @@ -270,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 @@ -285,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 @@ -297,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.