-
Notifications
You must be signed in to change notification settings - Fork 193
Add pull request process to CONTRIBUTING docs #892
Add pull request process to CONTRIBUTING docs #892
Conversation
Once a pull request is opened and the review process begins, it is not clear what is needed in order to get the pull request merged. At a high level, we want to try to make the following clear for contributors: - What is the next step towards getting this PR merged? - What's the status of that next step? - Is there anything I can/should do to help make that happen? In doing so, the process aims to clarify the following: - Who is responsible for _reviewing_ my pull requests? - Who is responsible for _approving_ pull requests? - Who is responsible for _merging_ pull requests? - What should I do if it seems like my pull request has gotten "lost in the mix"? How should I determine whether any further action is needed on my part? This is a starting point for documenting a process that we can follow and then further improve. Related: vmware-tanzu#844
acbaf37
to
5a911c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the doc updates, thanks @mcwumbly. I don't have approval rights, but I will go ahead and approve to show my support. I left one comment, but it is mostly food for thought.
CONTRIBUTING.md
Outdated
1. Review is automatically requested from CODEOWNERS and `needs-triage` label is automatically added. | ||
2. Assignee is added to pull request to ensure it gets proper attention throughout the process. | ||
Typically one of the CODEOWNERS will assign themselves, but they may choose to delegate to someone else. | ||
3. Triage removes `needs-triage` and adds `ok-to-merge` if the pull request is generally aligned with product goals and does not conflict with current milestones; otherwise they may add a comment and a `do-not-merge/*` label. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to read this line a couple of times to make sure I knew who Triage
was. I assume, based on discussion in SIG meetings and context clues, that Triage
is referring to a group of tanzu-framework product owners. Do you think there is any chance we need to be more specific about who Triage
is so that community members know exactly who is on the hook for this step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ankeesler I think it could be some combination of: 1) the assignee, 2) tanzu-framework-reviewers, or 3) a person wearing the "triage" hat during some periodic triage meeting... without more context on how that is evolving, I was weary of being overly specific here.
In the meantime, my hope is that this final paragraph offers enough clarity on what to do if things are not progressing:
If any of the above is unclear, and there has been no new activity for 2-3 days,
the contributor is encouraged to seek further information by commenting and
mentioning the assignee or @vmware-tanzu/tanzu-framework-reviewers if there is
no assignee or they themselves are unresponsive.
cc @vuil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can iterate and provider more clarifications when the triage process is more nailed down. For now, I think the quoted paragraph is a useful catch-all on what actions to take when things are not progressing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much thanks for doing this!
I have a question regarding needs-triage
CONTRIBUTING.md
Outdated
1. Review is automatically requested from CODEOWNERS and `needs-triage` label is automatically added. | ||
2. Assignee is added to pull request to ensure it gets proper attention throughout the process. | ||
Typically one of the CODEOWNERS will assign themselves, but they may choose to delegate to someone else. | ||
3. Triage removes `needs-triage` and adds `ok-to-merge` if the pull request is generally aligned with product goals and does not conflict with current milestones; otherwise they may add a comment and a `do-not-merge/*` label. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can iterate and provider more clarifications when the triage process is more nailed down. For now, I think the quoted paragraph is a useful catch-all on what actions to take when things are not progressing.
This label is not used for pull requests.
Related issue: vmware-tanzu#874
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
What this PR does / why we need it
Once a pull request is opened and the review process begins, it is not clear what is needed in order to get the pull request merged.
At a high level, we want to try to make the following clear for contributors:
In doing so, the process aims to clarify the following:
This is a starting point for documenting a process that we can follow
and then further improve.
Which issue(s) this PR fixes
Fixes #844
Describe testing done for PR
Discussion on linked issue
Release note
(no release note needed for this one)
PR Checklist
Additional information
Special notes for your reviewer