Reviewing Pull Requests
You can find great resources for this topic on YouTube. I especially recommend watching the following videos:
There are a couple of labels we use that determine how rigorous reviewers should be. The labels also clarify who is to blame if the PR turns out to introduce bugs or issues to the staging branch. These are, in order of severity:
| Label | Meaning | Who takes the blame if production breaks |
|---|---|---|
| PR: Just sign the damn thing | No thorough inspection required, only check the most obvious error sources. Must sign off with an approval message related to salmon (for traditional reasons). |
The PR owner |
| PR: Code Review | PR Owner wants reviewer to inspect only the code changes (no emulation /reproduction expected) |
The PR owner The reviewer, if errors could have been spotted on the code level |
| PR: Needs regular testing [default] | PR owner expects reviewers to reproduce the branch / emulate the app and test if the changes works as described. Thorough pentesting and bug hunting is not expected. |
The PR owner, if errors are machine-specific The reviewer (all other cases) |
| PR: Needs thorough testing | PR owner expects reviewers to thoroughly pentest the changes and bring forward any bugs found in the process, including testing on multiple machines/emulators and versions. | The reviewer |
Every PR with a protected branch (development, staging) should have one of the labels above. The labels are mutually exclusive (a single PR technically can, but should not have more than one PR: label.)