Review and approval process discussion
As we have disagreement among the developers on how to approach the code review and approval (see !97 (merged), !76 (merged) and bug #447394), I'm raising this issue to have a proper discussion and come to an agreement that everyone will follow from now on, so there will be no misunderstanding.
We need to decide on the following (please let me know what else):
- What is considered fully reviewed and approved change ready to merge.
- How much time we should allow for other devs to review after approval before merge.
- Who can approve various types of changes: comment change, doc change, code change.
To start the discussion, I propose the following guidelines:
- Any change proposed by a person outside of the Krew must be reviewed and approved.
- Merge requests are approved only by Approve button in GitLab by at least one Krew member with all the threads resolved. Any other types of feedback (reactions like thumbs up, tag "Approved" or positive comments) do not act as MR approval. Let's ditch the tags "Needs Review" and "Approved" as they were created a long time ago to replace missing approval functionality in GitLab.
- For changes in code comments and supporting files (like NEWS or Changelog) proposed by a Krew member, no MR or approval is required, however MR is encouraged.
- For changes in documentation proposed by a Krew member with the Documentation role, no MR or approval is required, however MR is encouraged. If the change is proposed by other people, including a Krew member with the Developer role, regular approval is required.
- For any changes in code or developer documentation, approval is required by at least one Krew member with the Developer role.
- Once a change is approved, it must not be merged for at least 3 days (72 hours) or up to 7 days (168 hours), if explicitly requested by a developer, to allow reviewing the latest revision. This is called a wait period. The timer resets each time a new thread is opened or unresolved (this is tracked by "resolved all threads" messages generated by GitLab), or code is changed (except for rebase). In case all developers sign off, the wait period ends earlier.
- Once the wait period ends, any Krew member may merge the change to the master branch.
- If the change fixes a bug that is applicable to the stable branch, it should be backported to the stable branch no earlier than 14 days after the master branch merge to allow more testing.
For reference, Krew currently consists of:
- @asensi (Developer role)
- @melnichenko (Developer role)
- @hkarel (Developer role)
- @gengisdave (Developer role)
- @yurchor (Documentation role)
Please share your feedback and proposals in the comments below.
Edited by Nikita Melnichenko