PR Process
The workflow is ultimately a two-stage PR review process where there will be a first tier of identified (one or potentially multiple) reviewers who do the first review and then pass it off to a final review done by a more experienced reviewer to actually merge the PR into the repo, providing that the PR is of sufficient quality and there were no issues found during the reviews.
In case there were issues found, the reviewers can ask for clarification and/or request changes by adding comments to the PR and pushing its state back to [WIP] as an indicator that there is action required from the author of the PR. See full workflow [1] and glossary below.
It is also crucial to understand that the states used during PR review process ([WIP], [WIPTEST], [RFR]) clearly define who is responsible for moving a PR forward at any given point. The main purpose of this is to make sure that reviewers don’t waste time on PRs that are still subject to change or PRs that have not been properly tested and that authors quickly see which of their PRs require action to be moved forward.
The hope is that this format will expose the first tier of reviewers to more areas of the framework as well as serving as a mentoring opportunity to grow more PR reviewers / mergers on all the different teams that contribute to this framework.
Comments are the most important part of any PR review. Without good, accurate and meaningful comments, a PR review is just a way for a reviewer to say No. Comments must not just state what is wrong, but also give enough information for the author to understand and fix. Below is an example of a "good" comment.
REQUIRED: We shouldn't add time.sleep into this code as external factors could cause it to be greater than this time. Extending the timeout further just leads to a the process failing much more slowly. We should try instead to find something we can wait for so that we know that the previous step has finished.
The author now knows, three things
- What is wrong
- Why it's wrong
- What is the suggested course of action to fix it
You should always ensure your comments answer these questions for anything other than trivial typos or lint errors.
Initial comments MUST always be prefixed with one of the following in bold.
REQUIRED: This means the reviewer will not move the PR on to the next stage until this work is completed or the question is answered. The PR will be moved back to WIP after the review is complete.
OPTIONAL: This means the reviewer can move the PR on to the next stage, unless another comment in the review prohibits them from doing so.
QUESTION: This means the reviewer is interested in hearing more about a particular topic, but CAN still merge the PR. If the moving forward of the PR requires an answer to the question, then the commend should be REQUIRED instead.
The Author should clearly state the main purpose of the PR in its description, explaining any potentially controversial design choices he made
- Leads the PR to full completion in a reasonable amount of time, ideally submitting commits as the work progresses
- Moves the PR state between [WIPTEST] and [WIP] depending on whether PRT testing is desirable or not respectively
- Moves the PR to [RFR] only when the PR is complete and fully tested; if there are failures or issues present, he/she is required to list them in the PR before changing the state and explain why reviewers should accept the PR despite those issues
- Cooperates with PR Reviewers by replying to their inquiries and updating the PR based on their suggestions/requests
- Must not continue to modify a PR that is already in [RFR] state (he/she would have to move it back to [WIP] state, thus stopping the review process)
- Must not move an untested PR or a PR with unexplained failures under review
- Review all open [RFR] here
- Review the PR as soon as possible, once switched to the [RFR] state by PR Author
- Review the PR as thoroughly as possible to minimize the amount of fixing cycles needed
- Assign the PR the appropriate label based on this document during initial review
- Move the PR to [1LP][RFR] for the final review or pushes it back to [WIP] with questions or requests for code change
- Cooperate with PR Authors by replying to their inquiries and helping devise solutions where knowledgeable enough
- Review all open [1LP][RFR] PRs
- Reviews the PR as soon as possible, once switched to the [1LP][RFR] by 1st Reviewer
- Reviews the PR as thoroughly as possible to minimize the amount of fixing cycles needed
- Merges the PR, pushes it back to [1LP][WIP] with questions or when small changes are required or all the way back to [WIP] if the PR requires major re-work
- Cooperates with PR Authors by replying to their inquiries and helping devise solutions where knowledgeable enough
An end to end example,
- John starts development by creating a branch and opening a PR with [WIP] in the title
- John finishes development and changes the [WIP] to [WIPTEST] to allow PRT to test it
- John has a failure which is unrelated to his test so he explains that in the description and changes the tag in the title to [RFR]
- Liam as a first level review picks up the PR as it is in [RFR], he finds a problem with a test and asks John to fix it via a negative review. He changes the title tag back to [WIP]
- John makes the change, pushes it to [WIPTEST]
- John sees that PRT tests are good apart from the same single failure and so pushes it to [RFR]
- Liam is now happy with the PR and adds an additional tag to make it [1LP][RFR]
- Simone is a second level reviewer, she now sees that the PR has passed the first level review and reviews the PR. She finds a small problem, submits a negative review, and removes the [RFR] tag replacing it with a [WIP] so that it becomes [1LP][WIP]
- John makes the appropriate change and pushes to [1LP][WIPTEST]
- John sees that the PR is passing and moves to [1LP][RFR]
- Simone has one question and so makes a comment and changes the tag to [1LP][WIP]
- John answers the question, which needed no changes and so moves it straight to [1LP][RFR]
- Simone is happy with the PR and so merges
Alternatively at step 8 above, Simone may find a large problem with the code. If so she removes the [1LP] and [RFR] tags, moving the PR back to step 1 as [WIP]. The PR restarts the process and must go through first level review again.
