Pr review policy#830
Conversation
Signed-off-by: pasinskim <marcin.pasinski@northern.tech>
pasinskim
force-pushed
the
pr_review_policy
branch
from
064ceef to
c499327
Compare
lluiscampos
left a comment
lluiscampos
left a comment
There was a problem hiding this comment.
Thank you for the effort putting this together. I think it was about time to have a policy that we can refer to 📝
I dropped few comments. Note in particular my concern wrt domain experts (see below and let's discuss it there if you see it fit).
In any case I am ready to start following the guideline so 👍 from me.
| ### Linter & Formatter | ||
| Automatically fixes all style and formatting issues. | ||
|
|
||
| We enforce a shared style guide (for example, Mantra will reuse the style guide from nt-gui). |
There was a problem hiding this comment.
| We enforce a shared style guide (for example, Mantra will reuse the style guide from nt-gui). | |
| Where appropriate, we enforce a shared style guide (for example, Mantra will reuse the style guide from nt-gui, Python tests generally formatted with black across all our projects). Some projects on the same language do not share the style for historical reasons (prime example C/C++ from mender not applied to mender-mcu) |
There was a problem hiding this comment.
Maybe we don't need to extend so munch on the examples, but at least I wanted to add some room here for projects that do not share style across the same programming language.
|
|
||
| A tool like SonarQube or Gitlab SATS will be used to scan the PR for code smells, complexity issues, and potential bugs. Some static code analysis can also be done within the Linter & Formatter step. | ||
|
|
||
| ### Type Script (Frontend Specific) |
There was a problem hiding this comment.
What about mentioning other cases like mypy for Python? Or golangci-lint for go?
| Automatically fixes all style and formatting issues. | ||
|
|
||
| We enforce a shared style guide (for example, Mantra will reuse the style guide from nt-gui). | ||
|
|
There was a problem hiding this comment.
Linters do more than just style, I think this section should have more content. (my comment about golangci-lint should maybe go here instead).
| ## Human (Product Review): 2nd Step | ||
| This review focuses on the "what" and "why" of the change. Even if the reviewer doesn't know the specific programming language, they are still a skilled engineer who understands the product and can provide valuable feedback. | ||
|
|
||
| ### Enforce Standard PR Description |
There was a problem hiding this comment.
This will depend a bit on the actual PR. In many cases I think it is an unnecessary overhead (and a duplication) if the author followed good commit practices. On top of it it is very common that a discussion in the PR triggers further commits, and then the description is stalled.
My recommendation is to remove this specific part of the policy.
There was a problem hiding this comment.
To illustrate why I think it is unnecessary,
In this PR I followed the "standard PR Description" but just chopping down the commit message into pieces.
| Do the tests cover the acceptance criteria? (This will be largely covered by the automated test coverage checks). | ||
|
|
||
| ## Human (Domain Expert Review): 3rd Step | ||
| **This final review is performed by an expert in the specific domain (e.g., frontend, backend, client) and programming language. Their focus is on the technical implementation and architecture.** |
There was a problem hiding this comment.
I am concerned in having this final gate applied to all repositories indiscriminately. Two cases that I think require some compromises:
- Areas where we don't have a domain expert. Comes to mind bootloaders (in particular UEFI/GRUB/SecureBoot) or yocto contributed features (systemd-boot, partuuid) which I have been reviewing as best effort basis. With this policy in place I won't review them anymore.
- Areas where we have a single domain expert. If I am the only expert in one thing, which other human will review my work?
This is N/A for most of what we do, though. Any programming language that we use is known for at least two developers, but when it comes to tools, frameworks, integrations or tech in general we will find areas that are one person or no-person which is that this comment is about.
There was a problem hiding this comment.
I agree with Lluis here; we have expert bus factors in some places; I'm thinking mostly about Josef's PRs (meta-mender, for example), that I sometime review trusting him. I don't think we have another expert like Josef in this field.
I guess that this should be a gate applied to only specific repositories, leaving common sense judgment to gray zones
3 participants
What: This PR adds new developer documentation for our official PR Review Policy.
This new documentation establishes a formal three-step review process:
It also introduces the standard "What, Why, How" template that all PRs should use in their descriptions.
Why: We had instances where it was not clear who should make the reviews of specific PRs (example was Mantra work and frontend reviews). This is to make sure we standardize and follow the agreed process for making code reviews. It should ensure all PRs are reviewed consistently and effectively and provide clear expectations for both the PR author and the reviewers.
How: A new documentation file is added to the repository. This file contains the detailed breakdown of the new three-step policy.