diff options
Diffstat (limited to 'doc/development/contributing')
-rw-r--r-- | doc/development/contributing/design.md | 4 | ||||
-rw-r--r-- | doc/development/contributing/index.md | 32 | ||||
-rw-r--r-- | doc/development/contributing/merge_request_workflow.md | 6 | ||||
-rw-r--r-- | doc/development/contributing/verify/index.md | 68 |
4 files changed, 78 insertions, 32 deletions
diff --git a/doc/development/contributing/design.md b/doc/development/contributing/design.md index 7f5c800216a..ce013a9254b 100644 --- a/doc/development/contributing/design.md +++ b/doc/development/contributing/design.md @@ -54,7 +54,7 @@ Check visual design properties using your browser's _elements inspector_ ([Chrom - Use recommended [colors](https://design.gitlab.com/product-foundations/colors/) and [typography](https://design.gitlab.com/product-foundations/type-fundamentals/). - Follow [layout guidelines](https://design.gitlab.com/layout/grid/). -- Use existing [icons](http://gitlab-org.gitlab.io/gitlab-svgs/) and [illustrations](http://gitlab-org.gitlab.io/gitlab-svgs/illustrations/) +- Use existing [icons](https://gitlab-org.gitlab.io/gitlab-svgs/) and [illustrations](https://gitlab-org.gitlab.io/gitlab-svgs/illustrations/) or propose new ones according to [iconography](https://design.gitlab.com/product-foundations/iconography/) and [illustration](https://design.gitlab.com/product-foundations/illustration/) guidelines. @@ -98,7 +98,7 @@ Check accessibility using your browser's _accessibility inspector_ ([Chrome](htt When the design is ready, _before_ starting its implementation: -- Share design specifications in the related issue, preferably through a [Figma link](https://help.figma.com/hc/en-us/articles/360040531773-Share-Files-with-anyone-using-Link-Sharing#Copy_links) +- Share design specifications in the related issue, preferably through a [Figma link](https://help.figma.com/hc/en-us/articles/360040531773-Share-Files-with-anyone-using-Link-Sharing#copy-link) link or [GitLab Designs feature](../../user/project/issues/design_management.md). See [when you should use each tool](https://about.gitlab.com/handbook/engineering/ux/product-designer/#deliver). - Document user flow and states (for example, using [Mermaid flowcharts in Markdown](../../user/markdown.md#mermaid)). diff --git a/doc/development/contributing/index.md b/doc/development/contributing/index.md index 8a4b06840a4..182d00d52ab 100644 --- a/doc/development/contributing/index.md +++ b/doc/development/contributing/index.md @@ -37,7 +37,7 @@ Report suspected security vulnerabilities by following the [disclosure process on the GitLab.com website](https://about.gitlab.com/security/disclosure/). WARNING: -Do **NOT** create publicly viewable issues for suspected security vulnerabilities. +Do **not** create publicly viewable issues for suspected security vulnerabilities. ## Code of conduct @@ -128,9 +128,12 @@ The general flow of contributing to GitLab is: 1. In the merge request's description: - Ensure you provide complete and accurate information. - Review the provided checklist. -1. Assign the merge request (if possible) to, or [mention](../../user/discussions/index.md#mentions), - one of the [code owners](../../user/project/code_owners.md) for the relevant project, - and explain that you are ready for review. +1. Once you're ready, mark your MR as ready for review with `@gitlab-bot ready`. + - This will add the `~"workflow::ready for review"` label, and then automatically assign a merge request coach as reviewer. + - If you know a relevant reviewer (for example, someone that was involved a related issue), you can also + assign them directly with `@gitlab-bot ready @username`. + +#### Review process When you submit code to GitLab, we really want it to get merged! However, we always review submissions carefully, and this takes time. Code submissions will usually be reviewed by two @@ -139,7 +142,11 @@ submissions carefully, and this takes time. Code submissions will usually be rev - A [reviewer](../code_review.md#the-responsibility-of-the-reviewer). - A [maintainer](../code_review.md#the-responsibility-of-the-maintainer). -Keep the following in mind when submitting merge requests: +After review, the reviewer could ask the author to update the merge request. In that case, the reviewer would set the `~"workflow::in dev"` label. +Once the merge request has been updated and set as ready for review again (for example, with `@gitlab-bot ready`), they will review the code again. +This process may repeat any number of times before merge, to help make the contribution the best it can be. + +Lastly, keep the following in mind when submitting merge requests: - When reviewers are reading through a merge request they may request guidance from other reviewers. @@ -154,13 +161,11 @@ Keep the following in mind when submitting merge requests: [approval](../../user/project/merge_requests/approvals/index.md) of merge requests, the maintainer may require [approvals from certain reviewers](../code_review.md#approval-guidelines) before merging a merge request. -- After review, the author may be asked to update the merge request. Once the merge request has been - updated and reassigned to the reviewer, they will review the code again. This process may repeat - any number of times before merge, to help make the contribution the best it can be. +- Sometimes a maintainer may choose to close a merge request. They will fully disclose why it will not + be merged, as well as some guidance. The maintainers will be open to discussion about how to change + the code so it can be approved and merged in the future. -Sometimes a maintainer may choose to close a merge request. They will fully disclose why it will not -be merged, as well as some guidance. The maintainers will be open to discussion about how to change -the code so it can be approved and merged in the future. +#### Getting attention on your merge request GitLab will do its best to review community contributions as quickly as possible. Specially appointed developers review community contributions daily. Look at the @@ -170,8 +175,9 @@ written some front-end code, you should mention the frontend merge request coach your code has multiple disciplines, you may mention multiple merge request coaches. GitLab receives a lot of community contributions. If your code has not been reviewed within two -working days of its initial submission, feel free to mention all merge request coaches with -`@gitlab-org/coaches` to get their attention. +working days of its initial submission, you can ask for help with `@gitlab-bot help`. + +#### Addition of external libraries When submitting code to GitLab, you may feel that your contribution requires the aid of an external library. If your code includes an external library, please provide a link to the library, as well as diff --git a/doc/development/contributing/merge_request_workflow.md b/doc/development/contributing/merge_request_workflow.md index ee1ed744cd4..eff1d2e671d 100644 --- a/doc/development/contributing/merge_request_workflow.md +++ b/doc/development/contributing/merge_request_workflow.md @@ -281,7 +281,7 @@ requirements. 1. The change is tested in a review app where possible and if appropriate. 1. The new feature does not degrade the user experience of the product. 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. An agreed-upon [rollout plan](https://about.gitlab.com/handbook/engineering/development/processes/rollout-plans/). +1. An agreed-upon [rollout plan](https://about.gitlab.com/handbook/engineering/development/processes/rollout-plans/). 1. Merged by a project maintainer. ### Production use @@ -292,7 +292,7 @@ requirements. 1. If there is a performance risk in the change, I have analyzed the performance of the system before and after the change. 1. *If the merge request uses feature flags, per-project or per-group enablement, and a staged rollout:* - Confirmed to be working on GitLab projects. - - Confirmed to be working at each stage for all projects added. + - Confirmed to be working at each stage for all projects added. 1. Added to the [release post](https://about.gitlab.com/handbook/marketing/blog/release-posts/), if relevant. 1. Added to [the website](https://gitlab.com/gitlab-com/www-gitlab-com/blob/master/data/features.yml), if relevant. @@ -322,7 +322,7 @@ issue) that are incremental improvements, such as: 1. Unprioritized bug fixes (for example, [Banner alerting of project move is showing up everywhere](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/18985)) 1. Documentation improvements -1. Rubocop or Code Quality improvements +1. RuboCop or Code Quality improvements Tag a merge request with ~"Stuff that should Just Work" to track work in this area. diff --git a/doc/development/contributing/verify/index.md b/doc/development/contributing/verify/index.md index 01aacffd00f..09b206d59aa 100644 --- a/doc/development/contributing/verify/index.md +++ b/doc/development/contributing/verify/index.md @@ -53,7 +53,7 @@ and they serve us and our users well. Some examples of these principles are that - The feedback delivered by GitLab CI/CD and data produced by the platform should be accurate. If a job fails and we notify a user that it was successful, it can have severe negative consequences. - Feedback needs to be available when a user needs it and data can not disappear unexpectedly when engineers need it. -- It all doesn’t matter if the platform is not secure and we +- It all doesn't matter if the platform is not secure and we are leaking credentials or secrets. - When a user provides a set of preconditions in a form of CI/CD configuration, the result should be deterministic each time a pipeline runs, because otherwise the platform might not be trustworthy. - If it is fast, simple to use and has a great UX it will serve our users well. @@ -134,7 +134,7 @@ applied to many other technical implementations. GitLab is a DevOps platform. We popularize DevOps because it helps companies be more efficient and achieve better results. One important component of DevOps culture is to take ownership over features and code that you are -building. It is very difficult to do that when you don’t know how your features +building. It is very difficult to do that when you don't know how your features perform and behave in the production environment. This is why we want to make our features and code observable. It @@ -164,15 +164,29 @@ data from the database, file system, or object storage, you should get an extra of eyes on your changes. When you are defining a new retention policy, you should double check with PMs and EMs. +### Get your design reviewed + +When you are designing a subsystem for pipeline processing and transitioning +CI/CD statuses, request an additional opinion on the design from a Verify maintainer (`@gitlab-org/maintainers/cicd-verify`) +as early as possible and hold others accountable for doing the same. Having your +design reviewed by a Verify maintainer helps to identify any blind spots you might +have overlooked as early as possible and possibly leads to a better solution. + +By having the design reviewed before any development work is started, it also helps to +make merge request review more efficient. You would be less likely to encounter +significantly differing opinions or change requests during the maintainer review +if the design has been reviewed by a Verify maintainer. As a result, the merge request +could be merged sooner. + ### Get your changes reviewed -When your merge request is ready for reviews you must assign -reviewers and then maintainers. Depending on the complexity of a change, you -might want to involve the people that know the most about the codebase area you are -changing. We do have many domain experts in Verify and it is absolutely acceptable to -ask them to review your code when you are not certain if a reviewer or -maintainer assigned by the Reviewer Roulette has enough context about the -change. +When your merge request is ready for reviews you must assign reviewers and then +maintainers. Depending on the complexity of a change, you might want to involve +the people that know the most about the codebase area you are changing. We do +have many domain experts and maintainers in Verify and it is absolutely +acceptable to ask them to review your code when you are not certain if a +reviewer or maintainer assigned by the Reviewer Roulette has enough context +about the change. The reviewer roulette offers useful suggestions, but as assigning the right reviewers is important it should not be done automatically every time. It might @@ -181,9 +195,19 @@ updating, because their feedback might be limited to code style and syntax. Depending on the complexity and impact of a change, assigning the right people to review your changes might be very important. -If you don’t know who to assign, consult `git blame` or ask in the `#verify` +If you don't know who to assign, consult `git blame` or ask in the `#s_verify` Slack channel (GitLab team members only). +There are two kinds of changes / merge requests that require additional +attention from reviews and an additional reviewer: + +1. Merge requests changing code around pipelines / stages / builds statuses. +1. Merge requests changing code around authentication / security features. + +In both cases engineers are expected to request a review from a maintainer and +a domain expert. If maintainer is the domain expert, involving another person +is recommended. + ### Incremental rollouts After your merge request is merged by a maintainer, it is time to release it to @@ -220,7 +244,7 @@ scenario relating to a software being built by one of our [early customers](http That would be quite an undesirable outcome of a small bug in GitLab CI/CD status processing. Please take extra care when you are working on CI/CD statuses, -we don’t want to implode our Universe! +we don't want to implode our Universe! This is an extreme and unlikely scenario, but presenting data that is not accurate can potentially cause a myriad of problems through the @@ -230,6 +254,22 @@ can have disastrous consequences. GitLab CI/CD is being used by companies building medical, aviation, and automotive software. Continuous Integration is a mission critical part of software engineering. -When you are working on a subsystem for pipeline processing and transitioning -CI/CD statuses, request an additional opinion on the design from a domain expert -as early as possible and hold others accountable for doing the same. +### Definition of Done + +In Verify, we follow our Development team's [Definition of Done](../merge_request_workflow.md#definition-of-done). +We also want to keep things efficient and [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) when we answer questions +and solve problems for our users. + +For any issue that is resolved because the solution is supported with existing `.gitlab-ci.yml` syntax, +create a project in the [`ci-sample-projects`](https://gitlab.com/gitlab-org/ci-sample-projects) group +that demonstrates the solution. + +The project must have: + +- A simple title. +- A clear description. +- A `README.md` with: + - A link to the resolved issue. You should also direct users to collaborate in the + resolved issue if any questions arise. + - A link to any relevant documentation. + - A detailed explanation of what the example is doing. |