feat: ADR for PR reviews #94
Conversation
sesheta
added
the
size/M
|
|
||
| ## Context and Problem Statement | ||
|
|
||
| Each community has its own `contributing.md` file which outlines how are users expected to contribute to the community. That guide for contributions should also clearly state what will happen once a PR is submitted and how reviews are performed. |
There was a problem hiding this comment.
Well..we don't have it in every repo yet, that in it self is also a problem. Maybe we should add it to all the repos and as part of the template too?
There was a problem hiding this comment.
Yeah, we probably should.
There was a problem hiding this comment.
Actually, @HumairAK I don't feel like we should replicate content that much. Would you mind if I just transfer the contributing doc into blueprints as an ADR and then create a blank contributing doc in each repo linking to that contributing ADR?
|
/lgtm |
docs/adr/0016-pr-review.md
Outdated
|
|
||
| 1. A PR is submited | ||
| 2. Prow (represented by a Sesheta bot-account) will pick a selection of reviewers from corresponding `OWNERS` file (based on modified files in the PR). | ||
| 3. Reviewers assess the changes from programming/coding point of view and either provide feedback or respond with `/lgtm` chat-ops command. |
There was a problem hiding this comment.
Grammar nit: From a programming/coding point of view
But I would probably just drop that for:
Reviewers assess the proposed changes and either provide feedback or respond with ...
I see below that you're trying to make some distinction between "programming" and "architectural", maybe, but I'm not sure that's germane; I think both the "reviewers" and "approver" are probably looking at a variety of aspects.
There was a problem hiding this comment.
Yeah.. I think those roles are best described in the upstream docs anyways. It's linked above when explaining the OWNERS file.
I've copied some of it from those docs and changed it here instead:
- Reviewers assess the changes from programming/coding point of view and either provide feedback
+ Reviewers assess the code for general code quality, correctness, sane software engineering, style. They either provide feedback- Approvers evaluate architectural aspects of the PR and either provide feedback
+ Approvers evaluate architectural aspects of the PR, holistic acceptance criteria, including dependencies with other features, forwards/backwards compatibility, API and flag definitions. They either provide feedbackWDYT?
There was a problem hiding this comment.
That seems fine, although there's a word missing in both phrases
- Reviewers assess the code for general code quality, correctness, sane software engineering, and style. They either provide feedback
And:
Approvers evaluate architectural aspects of the PR and holistic acceptance criteria, including dependencies with other features, forwards/backwards compatibility, API and flag definitions. They either provide feedback
There was a problem hiding this comment.
Thank you, sir! 🙂 Updated! 👍
Signed-off-by: Tomas Coufal <[email protected]>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anishasthana, HumairAK 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 |
sesheta
added
the
approved
Resolves: #93