diff options
Diffstat (limited to 'doc/development/contributing')
| -rw-r--r-- | doc/development/contributing/index.md | 12 | ||||
| -rw-r--r-- | doc/development/contributing/merge_request_workflow.md | 104 | ||||
| -rw-r--r-- | doc/development/contributing/style_guides.md | 37 |
3 files changed, 106 insertions, 47 deletions
diff --git a/doc/development/contributing/index.md b/doc/development/contributing/index.md index 6999ffe810e..2ff51e765a3 100644 --- a/doc/development/contributing/index.md +++ b/doc/development/contributing/index.md @@ -99,10 +99,14 @@ If you would like to contribute to GitLab: could speed them up. - Consult the [Contribution Flow](#contribution-flow) section to learn the process. -If you have any questions or need help visit [Getting Help](https://about.gitlab.com/get-help/) to -learn how to communicate with GitLab. We have a [Gitter channel for contributors](https://gitter.im/gitlab/contributors), -however we favor -[asynchronous communication](https://about.gitlab.com/handbook/communication/#internal-communication) over real time communication. +### Communication channels + +If you have any questions or need help, visit [Getting Help](https://about.gitlab.com/get-help/) to learn how to +communicate with the GitLab community. GitLab prefers [asynchronous communication](https://about.gitlab.com/handbook/communication/#internal-communication) over real-time communication. + +We do encourage you to connect and hang out with us. GitLab has a Gitter room dedicated for [contributors](https://gitter.im/gitlab/contributors), which is bridged with our +internal Slack. We actively monitor this channel. There is also a community-run [Discord server](https://discord.gg/S4cwz9sR8u) where you can +find other contributors in the `#contributors` channel. Thanks for your contribution! 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. diff --git a/doc/development/contributing/style_guides.md b/doc/development/contributing/style_guides.md index 7a4ebbdbadf..2e696cf517b 100644 --- a/doc/development/contributing/style_guides.md +++ b/doc/development/contributing/style_guides.md @@ -146,13 +146,20 @@ reduces the aforementioned [bike-shedding](https://en.wiktionary.org/wiki/bikesh To that end, we encourage creation of new RuboCop rules in the codebase. -We currently maintain Cops across several Ruby code bases, and not all of them are +We maintain Cops across several Ruby code bases, and not all of them are specific to the GitLab application. When creating a new cop that could be applied to multiple applications, we encourage you to add it to our [GitLab Styles](https://gitlab.com/gitlab-org/gitlab-styles) gem. If the Cop targets rules that only apply to the main GitLab application, it should be added to [GitLab](https://gitlab.com/gitlab-org/gitlab) instead. +#### RuboCop node pattern + +When creating [node patterns](https://docs.rubocop.org/rubocop-ast/node_pattern.html) to match +Ruby's AST, you can use [`scripts/rubocop-parse`](https://gitlab.com/gitlab-org/gitlab/-/blob/master/scripts/rubocop-parse) +to display the AST of a Ruby expression, in order to help you create the matcher. +See also [!97024](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/97024). + ### Resolving RuboCop exceptions When the number of RuboCop exceptions exceed the default [`exclude-limit` of 15](https://docs.rubocop.org/rubocop/1.2/usage/basic_usage.html#command-line-flags), @@ -222,6 +229,34 @@ We're following [Ciro Santilli's Markdown Style Guide](https://cirosantilli.com/ See the dedicated [Documentation Style Guide](../documentation/styleguide/index.md). +### Guidelines for good practices + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/36576/) in GitLab 13.2 as GitLab Development documentation. + +*Good practice* examples demonstrate encouraged ways of writing code while +comparing with examples of practices to avoid. These examples are labeled as +*Bad* or *Good*. In GitLab development guidelines, when presenting the cases, +it's recommended to follow a *first-bad-then-good* strategy. First demonstrate +the *Bad* practice (how things *could* be done, which is often still working +code), and then how things *should* be done better, using a *Good* example. This +is typically an improved example of the same code. + +Consider the following guidelines when offering examples: + +- First, offer the *Bad* example, and then the *Good* one. +- When only one bad case and one good case is given, use the same code block. +- When more than one bad case or one good case is offered, use separated code + blocks for each. With many examples being presented, a clear separation helps + the reader to go directly to the good part. Consider offering an explanation + (for example, a comment, or a link to a resource) on why something is bad + practice. +- Better and best cases can be considered part of the good cases' code block. + In the same code block, precede each with comments: `# Better` and `# Best`. + +Although the bad-then-good approach is acceptable for the GitLab development +guidelines, do not use it for user documentation. For user documentation, use +*Do* and *Don't*. For examples, see the [Pajamas Design System](https://design.gitlab.com/content/punctuation/). + ## Python See the dedicated [Python Development Guidelines](../python_guide/index.md). |