From a1e267dc7534acc425ac510a7446d4a83db7c0b6 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 9 Oct 2018 17:20:29 +0000 Subject: [PATCH 1/4] Document the role of the maintainer --- doc/development/code_review.md | 69 ++++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 16 deletions(-) diff --git a/doc/development/code_review.md b/doc/development/code_review.md index edf0b6f46df..6833154b2be 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -4,31 +4,25 @@ There are a few rules to get your merge request accepted: -1. Your merge request should only be **merged by a [maintainer][team]**. - 1. If your merge request includes only backend changes [^1], it must be +1. Your merge request can only be **merged by a [maintainer][team]**. + 1. If your merge request includes backend changes [^1], it must be **approved by a [backend maintainer][projects]**. - 1. If your merge request includes only frontend changes [^1], it must be + 1. If your merge request includes frontend changes [^1], it must be **approved by a [frontend maintainer][projects]**. - 1. If your merge request includes UX changes [^1], it must - be **approved by a [UX team member][team]**. + 1. If your merge request includes UX changes [^1], it must be + **approved by a [UX team member][team]**. 1. If your merge request includes adding a new JavaScript library [^1], it must be **approved by a [frontend lead][team]**. 1. If your merge request includes adding a new UI/UX paradigm [^1], it must be **approved by a [UX lead][team]**. - 1. If your merge request includes frontend and backend changes [^1], it must - be **approved by a [frontend and a backend maintainer][projects]**. - 1. If your merge request includes UX and frontend changes [^1], it must - be **approved by a [UX team member and a frontend maintainer][team]**. - 1. If your merge request includes UX, frontend and backend changes [^1], it must - be **approved by a [UX team member, a frontend and a backend maintainer][team]**. - 1. If your merge request includes a new dependency or a filesystem change, it must - be *approved by a [Distribution team member][team]*. See how to work with the [Distribution team for more details.](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) -1. To lower the amount of merge requests maintainers need to review, you can + 1. If your merge request includes a new dependency or a filesystem change, it must be + **approved by a [Distribution team member][team]**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) for more details. + 1. If more than one of the items above apply to your merge request, it must be + **approved by all listed people**. The last of the people to approve can then merge it. +1. To lower the amount of merge requests maintainers need to review, you are encouraged ask or assign any [reviewers][projects] for a first review. 1. If you need some guidance (e.g. it's your first merge request), feel free to ask one of the [Merge request coaches][team]. - 1. It is recommended that you assign a maintainer that is from a different team than your own. - This ensures that all code across GitLab is consistent and can be easily understood by all contributors. 1. Keep in mind that maintainers are also going to perform a final code review. The ideal scenario is that the reviewer has already addressed any concerns @@ -37,6 +31,49 @@ There are a few rules to get your merge request accepted: For more guidance, see [CONTRIBUTING.md](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md). +### The role of the maintainer + +Maintainers are responsible for the overall health, quality, and consistency of +the GitLab codebase, across domains and product areas. Consequently, their reviews +will focus primarily on things like overall architecture, code organization, +separation of concerns, tests, DRYness, consistency, readability, etc. + +Their job is explicitly _not_ to review the solution itself. By the time a merge +request makes it to a maintainer, they should be able to assume that it actually +solves the problem it was meant to solve, that it does so in the most appropriate +way, that it satisfies all requirements, and that there are no remaining bugs, +logical problems, or uncovered edge cases. + +The responsibility to find the best solution and implement it lies with the +merge request author, and they should be confident of the chosen solution, +implementation, and everything else that makes up the merge request, before +they ask a maintainer for final review, approval, and merge. + +To reach this level of confidence, an author is expected to involve other people +in the investigation and implementation processes as appropriate. They may want +to reach out to domain experts to discuss different solutions or get an +implementation reviewed, to product managers and UX designers to clear up +confusion or verify that the end result matches what they had in mind, or to +database specialists to get input on the data model or specific queries. + +They are also strongly encouraged to get their code reviewed by any other developer +as soon as there is any code to review, to get a second opinion on the chosen +solution and implementation and an extra pair of eyes looking for bugs, +logic problems, or uncovered edge cases, and to ease the job of the maintainer. + +Of course, a maintainer will also make note of any issues with the solution or +implementation they may find, but in general will assume that the author is the +expert on the issue at hand, and that they made their choices with good reason. + +Since a maintainer's job does not depend on their domain-specific knowledge beyond +knowledge of the overall GitLab codebase, they can review merge requests from any +team and in any product area. + +Authors are recommended to assign merge requests to maintainers from other teams +than their own, to ensure that all code across GitLab is consistent and can be +easily understood by all contributors, from both inside and outside the company, +without requiring team-specific expertise. + ## Best practices This guide contains advice and best practices for performing code review, and From eb0ded1d909fe5bb53294dc8f9b48a2fdee45fb9 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 10 Oct 2018 10:48:14 +0000 Subject: [PATCH 2/4] Rewrite guidance on getting your merge request reviewed, approved, and merged --- doc/development/code_review.md | 66 ++++++++++++++++------------------ 1 file changed, 31 insertions(+), 35 deletions(-) diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 6833154b2be..d18bb51eb5d 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -1,14 +1,31 @@ # Code Review Guidelines +This guide contains advice and best practices for performing code review, and +having your code reviewed. + +All merge requests for GitLab CE and EE, whether written by a GitLab team member +or a volunteer contributor, must go through a code review process to ensure the +code is effective, understandable, and maintainable. + ## Getting your merge request reviewed, approved, and merged -There are a few rules to get your merge request accepted: +You are strongly encouraged to get your code **reviewed** by a +[reviewer](https://about.gitlab.com/handbook/engineering/#reviewer) as soon as +there is any code to review, to get a second opinion on the chosen solution and +implementation, and an extra pair of eyes looking for bugs, logic problems, or +uncovered edge cases. The reviewer can be from a different team, but it is often +useful to pick someone who knows the domain well. + +If you need some guidance (e.g. it's your first merge request), feel free to ask +one of the [Merge request coaches][team]. + +Depending on the areas your merge request touches, it must be **approved** by one +or more [maintainers](https://about.gitlab.com/handbook/engineering/#maintainer): -1. Your merge request can only be **merged by a [maintainer][team]**. 1. If your merge request includes backend changes [^1], it must be - **approved by a [backend maintainer][projects]**. + **approved by a [backend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_backend)**. 1. If your merge request includes frontend changes [^1], it must be - **approved by a [frontend maintainer][projects]**. + **approved by a [frontend maintainer](https://about.gitlab.com/handbook/engineering/projects/#gitlab-ce_maintainers_frontend)**. 1. If your merge request includes UX changes [^1], it must be **approved by a [UX team member][team]**. 1. If your merge request includes adding a new JavaScript library [^1], it must be @@ -17,19 +34,14 @@ There are a few rules to get your merge request accepted: **approved by a [UX lead][team]**. 1. If your merge request includes a new dependency or a filesystem change, it must be **approved by a [Distribution team member][team]**. See how to work with the [Distribution team](https://about.gitlab.com/handbook/engineering/dev-backend/distribution/) for more details. - 1. If more than one of the items above apply to your merge request, it must be - **approved by all listed people**. The last of the people to approve can then merge it. -1. To lower the amount of merge requests maintainers need to review, you are encouraged - ask or assign any [reviewers][projects] for a first review. - 1. If you need some guidance (e.g. it's your first merge request), feel free - to ask one of the [Merge request coaches][team]. - -1. Keep in mind that maintainers are also going to perform a final code review. - The ideal scenario is that the reviewer has already addressed any concerns - the maintainer would have found, and the maintainer only has to perform the - merge, but be prepared for further review comments. -For more guidance, see [CONTRIBUTING.md](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md). +Getting your merge request **merged** also requires a maintainer. If it requires +more than one approval, the last maintainer to review and approve it will also merge it. + +Keep in mind that maintainers are also going to perform a final code review. +The ideal scenario is that the reviewer has already identified any concerns +the maintainer would have found, and the maintainer only has to perform the +merge, but be prepared for further review comments. ### The role of the maintainer @@ -53,13 +65,9 @@ To reach this level of confidence, an author is expected to involve other people in the investigation and implementation processes as appropriate. They may want to reach out to domain experts to discuss different solutions or get an implementation reviewed, to product managers and UX designers to clear up -confusion or verify that the end result matches what they had in mind, or to -database specialists to get input on the data model or specific queries. - -They are also strongly encouraged to get their code reviewed by any other developer -as soon as there is any code to review, to get a second opinion on the chosen -solution and implementation and an extra pair of eyes looking for bugs, -logic problems, or uncovered edge cases, and to ease the job of the maintainer. +confusion or verify that the end result matches what they had in mind, to +database specialists to get input on the data model or specific queries, +or to any other developer to get a code review. Of course, a maintainer will also make note of any issues with the solution or implementation they may find, but in general will assume that the author is the @@ -76,18 +84,6 @@ without requiring team-specific expertise. ## Best practices -This guide contains advice and best practices for performing code review, and -having your code reviewed. - -All merge requests for GitLab CE and EE, whether written by a GitLab team member -or a volunteer contributor, must go through a code review process to ensure the -code is effective, understandable, and maintainable. - -Any developer can, and is encouraged to, perform code review on merge requests -of colleagues and contributors. However, the final decision to accept a merge -request is up to one the project's maintainers, denoted on the -[engineering projects][projects]. - ### Everyone - Accept that many programming decisions are opinions. Discuss tradeoffs, which From 90056ed25b547537b02b29715e6153d3aab4cc79 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 16 Oct 2018 22:31:27 +0000 Subject: [PATCH 3/4] Clarify responsibilities of MR author and maintainer based on feedback. --- doc/development/code_review.md | 85 +++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 37 deletions(-) diff --git a/doc/development/code_review.md b/doc/development/code_review.md index d18bb51eb5d..fac31fe8e8a 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -14,7 +14,8 @@ You are strongly encouraged to get your code **reviewed** by a there is any code to review, to get a second opinion on the chosen solution and implementation, and an extra pair of eyes looking for bugs, logic problems, or uncovered edge cases. The reviewer can be from a different team, but it is often -useful to pick someone who knows the domain well. +useful to pick someone who knows the domain well. You can read more about the +importance of involving reviewer(s) in the section on the responsibility of the author below. If you need some guidance (e.g. it's your first merge request), feel free to ask one of the [Merge request coaches][team]. @@ -38,49 +39,59 @@ or more [maintainers](https://about.gitlab.com/handbook/engineering/#maintainer) Getting your merge request **merged** also requires a maintainer. If it requires more than one approval, the last maintainer to review and approve it will also merge it. -Keep in mind that maintainers are also going to perform a final code review. -The ideal scenario is that the reviewer has already identified any concerns -the maintainer would have found, and the maintainer only has to perform the -merge, but be prepared for further review comments. +As described in the section on the responsibility of the maintainer below, you +are recommended to get your merge request approved and merged by maintainer(s) +from other teams than your own. -### The role of the maintainer - -Maintainers are responsible for the overall health, quality, and consistency of -the GitLab codebase, across domains and product areas. Consequently, their reviews -will focus primarily on things like overall architecture, code organization, -separation of concerns, tests, DRYness, consistency, readability, etc. - -Their job is explicitly _not_ to review the solution itself. By the time a merge -request makes it to a maintainer, they should be able to assume that it actually -solves the problem it was meant to solve, that it does so in the most appropriate -way, that it satisfies all requirements, and that there are no remaining bugs, -logical problems, or uncovered edge cases. +### The responsibility of the merge request author The responsibility to find the best solution and implement it lies with the -merge request author, and they should be confident of the chosen solution, -implementation, and everything else that makes up the merge request, before -they ask a maintainer for final review, approval, and merge. +merge request author. -To reach this level of confidence, an author is expected to involve other people -in the investigation and implementation processes as appropriate. They may want -to reach out to domain experts to discuss different solutions or get an -implementation reviewed, to product managers and UX designers to clear up -confusion or verify that the end result matches what they had in mind, to -database specialists to get input on the data model or specific queries, -or to any other developer to get a code review. +Before assigning a merge request to maintainer for approval and merge, they +should be confident that it actually solves the problem it was meant to solve, +that it does so in the most appropriate way, that it satisfies all requirements, +and that there are no remaining bugs, logical problems, or uncovered edge cases. +The merge request should also have a completed task list in its description and +a passing CI pipeline to avoid unnecessary back and forth. -Of course, a maintainer will also make note of any issues with the solution or -implementation they may find, but in general will assume that the author is the -expert on the issue at hand, and that they made their choices with good reason. +To reach the required level of confidence in their solution, an author is expected +to involve other people in the investigation and implementation processes as +appropriate: -Since a maintainer's job does not depend on their domain-specific knowledge beyond -knowledge of the overall GitLab codebase, they can review merge requests from any -team and in any product area. +They are encouraged to reach out to domain experts to discuss different solutions +or get an implementation reviewed, to product managers and UX designers to clear +up confusion or verify that the end result matches what they had in mind, to +database specialists to get input on the data model or specific queries, or to +any other developer to get an in-depth review of the solution. -Authors are recommended to assign merge requests to maintainers from other teams -than their own, to ensure that all code across GitLab is consistent and can be -easily understood by all contributors, from both inside and outside the company, -without requiring team-specific expertise. +### The responsibility of the maintainer + +Maintainers are responsible for the overall health, quality, and consistency of +the GitLab codebase, across domains and product areas. + +Consequently, their reviews will focus primarily on things like overall +architecture, code organization, separation of concerns, tests, DRYness, +consistency, and readability. + +Since a maintainer's job only depends on their knowledge of the overall GitLab +codebase, and not that of any specific domain, they can review, approve and merge +merge requests from any team and in any product area. + +In fact, authors are recommended to get their merge requests merged by maintainers +from other teams than their own, to ensure that all code across GitLab is consistent +and can be easily understood by all contributors, from both inside and outside the +company, without requiring team-specific expertise. + +Maintainers will do their best to also review the specifics of the chosen solution +before merging, but as they are not necessarily domain experts, they may be poorly +placed to do so without an unreasonable investment of time. In those cases, they +will defer to the judgment of the author and earlier reviewers and involved domain +experts, in favor of focusing on their primary responsibilities. + +If a developer who happens to also be a maintainer was involved in a merge request +as a domain expert and/or reviewer, it is recommended that they are not also picked +as the maintainer to ultimately approve and merge it. ## Best practices From 2a631de547b230e52daf591cbbf31e0b1e953d45 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 17 Oct 2018 17:38:45 +0000 Subject: [PATCH 4/4] Strongly recommend involving a domain expert, especially when in doubt. --- doc/development/code_review.md | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/doc/development/code_review.md b/doc/development/code_review.md index 4d3a817e78b..3fe79943fdc 100644 --- a/doc/development/code_review.md +++ b/doc/development/code_review.md @@ -13,8 +13,8 @@ You are strongly encouraged to get your code **reviewed** by a [reviewer](https://about.gitlab.com/handbook/engineering/#reviewer) as soon as there is any code to review, to get a second opinion on the chosen solution and implementation, and an extra pair of eyes looking for bugs, logic problems, or -uncovered edge cases. The reviewer can be from a different team, but it is often -useful to pick someone who knows the domain well. You can read more about the +uncovered edge cases. The reviewer can be from a different team, but it is +recommended to pick someone who knows the domain well. You can read more about the importance of involving reviewer(s) in the section on the responsibility of the author below. If you need some guidance (e.g. it's your first merge request), feel free to ask @@ -48,7 +48,7 @@ from other teams than your own. The responsibility to find the best solution and implement it lies with the merge request author. -Before assigning a merge request to maintainer for approval and merge, they +Before assigning a merge request to a maintainer for approval and merge, they should be confident that it actually solves the problem it was meant to solve, that it does so in the most appropriate way, that it satisfies all requirements, and that there are no remaining bugs, logical problems, or uncovered edge cases. @@ -57,7 +57,7 @@ a passing CI pipeline to avoid unnecessary back and forth. To reach the required level of confidence in their solution, an author is expected to involve other people in the investigation and implementation processes as -appropriate: +appropriate. They are encouraged to reach out to domain experts to discuss different solutions or get an implementation reviewed, to product managers and UX designers to clear @@ -65,6 +65,10 @@ up confusion or verify that the end result matches what they had in mind, to database specialists to get input on the data model or specific queries, or to any other developer to get an in-depth review of the solution. +If an author is unsure if a merge request needs a domain expert's opinion, that's +usually a pretty good sign that it does, since without it the required level of +confidence in their solution will not have been reached. + ### The responsibility of the maintainer Maintainers are responsible for the overall health, quality, and consistency of