📖 Update CONTRIBUTING.md with high-level review guidelines #3191
Conversation
Signed-off-by: Vince Prignano <vincepri@vmware.com>
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
size/M
vincepri
changed the title
|
I find I'm sometimes not sure when to Is it worth adding some language about when it's appropriate to LGTM, or is that covered by the note about approval? |
|
@sethp-nr That makes sense, this is only a taste of the full guidelines we should have — I'll add something along those lines as I go through the updates |
|
@sethp-nr @vincepri We could change the repo configuration in https://github.com/kubernetes/test-infra/blob/ab37d8b9138344d510dafee708fee1b03c2d5b1e/config/prow/plugins.yaml and set |
| process. | ||
|
|
||
| - A PR is approved by one of the project maintainers and owners after reviews. | ||
| - Approvals should be the very last action a maintainer takes on a pull request. |
There was a problem hiding this comment.
I know this is a topic subject of many discussions in Kubernetes land...
IMO It will be great to disable the bot that automatically applies /approve for maintainers, so all the PRs can follow the same approach and every PRs requires approval from someone different than the author
There was a problem hiding this comment.
Ohhh... I see now #3191 (comment)
I'm happy to help with this if there is agreement
There was a problem hiding this comment.
That is a fine change, although I want to call out that even in that case, the above guideline stands.
|
|
||
| Any Kubernetes organization member can leave reviews and `/lgtm` a pull request. | ||
|
|
||
| Code reviews should generally look at: |
There was a problem hiding this comment.
xposting #3194 (comment)
I would refer to "REVIEWING.md" for details about what a review should look at
There was a problem hiding this comment.
These bits go in the book as well, let's have the generic high level list here; I'll add a link to the REVIEWING doc once it merges
|
/lgtm |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
Just had a read, everything looks good as far as I'm concerned +1 to the |
| @@ -49,6 +55,33 @@ All changes must be code reviewed. Coding conventions and standards are explaine | |||
| docs](https://github.com/kubernetes/community/tree/master/contributors/devel). Expect reviewers to request that you | |||
| avoid common [go style mistakes](https://github.com/golang/go/wiki/CodeReviewComments) in your PRs. | |||
|
|
|||
| ## Reviewing a Patch | |||
There was a problem hiding this comment.
should this link to REVIEWING.md?
There was a problem hiding this comment.
|
/lgtm |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/test pull-cluster-api-make |
Signed-off-by: Vince Prignano vincepri@vmware.com
What this PR does / why we need it:
This PR is a starting point to better document our review process and what we expect from reviewers.