search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.45k stars 4.76k forks source link

Documenting Issue Triage and Pull Request workflows #62496

Open eiriktsarpalis opened 2 years ago

eiriktsarpalis commented 2 years ago

Background & Motivation

As part of our initiative to improve our responsiveness metrics in dotnet/runtime, this issue outlines a proposal to formalize and document common workflows for issue triage and community pull request reviews. It is based on the hypothesis originally expressed in https://github.com/microsoft/contributor-community-experiments/issues/1, namely that issues and community PRs that go stale tend to do so for the following reasons:

At a high level this is a proposal to:

It should be noted that defining triage guidelines or a community PR playbook is not in scope for this particular initiative. In other words a workflow document focuses on what state transitions are to be expected but not when and why such state transitions should be applied by the area owner. While authoring triage/PR review guidelines for area owners is something that we want to eventually add to the repo, it addresses an independent concern and should be a separate document.

This proposal was created in consultation with @ericstj, @jeffhandley and @joperezr.

Issue Triage workflows

The issue triage workflow document would include the following information:

The document would also define a diagram such as the following:

image

PR review workflows

We currently don't have a process defined for community PR reviews. As part of this proposal we would like to introduce the following concepts/policies:

Here's what a PR review diagram could look like:

image

cc @danmoseley @jeffschwMSFT @marek-safar @karelz @JulieLeeMSFT for feedback.

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-meta See info in area-owners.md if you want to be subscribed.

Issue Details
## Background & Motivation As part of our initiative to improve our responsiveness metrics in dotnet/runtime, this issue outlines a proposal to formalize and document common workflows for issue triage and community pull request reviews. It is based on the hypothesis originally expressed in https://github.com/microsoft/contributor-community-experiments/issues/1, namely that issues and community PRs that go stale tend to do so for the following reasons: * Area owners have no good way of tracking what tickets require their immediate attention (versus the ones waiting on feedback from authors or need further discussion). In other words, as an area owner I want to be able to write a query showing all untriaged issues or community pull requests where "the ball is firmly on my court". * We don't have a universal mechanism for closing stale issues or community PRs where action is required from the author. This is covered in part by the `needs more info` label in issues but we have no corresponding automation for duplicate or `question` issues or for PRs that have been reviewed as "changes requested". At a high level this is a proposal to: * Define and document common workflows and states of the issue triage process and PR review process. This is inspired by aspnetcore's [TriageProcess.md](https://github.com/dotnet/aspnetcore/blob/main/docs/TriageProcess.md) and VS Code's [Issue Triaging](https://github.com/microsoft/vscode/wiki/Issues-Triaging) documentation. It should be a public document intended to communicate expectations to issue and PR authors (complementing our existing [CONTRIBUTING.md](https://github.com/dotnet/runtime/blob/main/CONTRIBUTING.md) documentation). * Use existing labels or introduce new ones to make workflow states queryable. * Introduce FabricBot rules that assist and automate aspects of the workflows, either by providing automated replies (aka "canned responses") when a label is applied manually or pinging/closing tickets that have gone stale. It should be noted that defining triage guidelines or a community PR playbook is not in scope for this particular initiative. In other words a workflow document focuses on _what_ state transitions are to be expected but not _when_ and _why_ such state transitions should be applied by the area owner. While authoring triage/PR review guidelines for area owners is something that we want to eventually add to the repo, it addresses an independent concern and should be a separate document. This proposal was created in consultation with @ericstj, @jeffhandley and @joperezr. ## Issue Triage workflows The issue triage workflow document would include the following information: * An overview of area labels, area owners and the issue labeling bot. * A definition of "Triaged" (e.g. "an issue is considered triaged if it has been given milestone or has been closed") * An overview of common labels applied after triage is complete: `bug`, `enhancement`, `tenet-performance`, `api-suggestion`, etc. * A definition of the workflow used to automatically close stale issues. The document would also define a diagram such as the following: ![image](https://user-images.githubusercontent.com/2813363/145084944-fcac5f72-354a-4e23-8ace-3da0d9ee144f.png) ## PR review workflows We currently don't have a process defined for community PR reviews. As part of this proposal we would like to introduce the following concepts/policies: * Every pull request must have a **PR champion**: an assignee who is an area owner or member of the wider team. The PR champion is accountable for getting the pull request merged in a timely manner, shepherding reviews, addressing CI issues and merge conflicts, hitting the merge button. A PR champion might be the author or a reviewer but these concepts don't necessarily overlap. * Introduce the concept of **PR triage**: the area owners determine the motivation for the PR, decide whether it aligns with our goals, link any relevant issues and elect a PR champion. This should be an optional step; it should still be possible to immediately review trivial PRs without enforcing a lot of ceremony. * Introduce a `needs-author-action` label to denote PRs needing more work from the author. PRs with "Request Changes" reviews are tagged `needs-author-action` automatically. Stale PRs with `needs-author-action` get pinged automatically after 15 days and gets closed automatically if no further action occurs after another 15 days. The `needs-author-action` concept is a generalization of `needs more info` in issues and it's likely we would want to consolidate the two eventually. Here's what a PR review diagram could look like: ![image](https://user-images.githubusercontent.com/2813363/145090705-816481b5-1e1a-42d2-8538-d17707ce3f9d.png) cc @danmoseley @jeffschwMSFT @marek-safar @karelz @JulieLeeMSFT for feedback.
Author: eiriktsarpalis
Assignees: -
Labels: `area-Meta`
Milestone: -
jeffschwMSFT commented 2 years ago

Several of these elements are defined in this document: https://github.com/dotnet/runtime/blob/main/docs/issues-pr-management.md. We should consider either merging this document with these proposed resources or enhance the existing one.

jeffschwMSFT commented 2 years ago

cc @mangod9 @agocke @tommcdon @JulieLeeMSFT

danmoseley commented 2 years ago

Thanks for writing this out. For PR's, we rarely use the Github "changes requested" state. We just make comments. "needs-author-action" would need to be applied manually -- do you expect that to be applied often? Or just when there's been a pause and we just want to clarify where next action lies?

One concern that's been expressed about "needs-author-action" workflow on PR's is that sometimes the delay is on the reviewer side. I think we simply need to continue to hold reviewers accountable for being timely as well. We have been doing that with an internal mailer and I think it's improved. As well we've discussed that we could sometimes do better at rejecting PR's early if they're not ones it makes sense to take.

Interested in any feedback here from our community on the issue and PR workflow here.

joperezr commented 2 years ago

do you expect that to be applied often? Or just when there's been a pause and we just want to clarify where next action lies?

I do expect this to have to be applied often, and I do agree with you that we rarely use the changes-requested review option but that is the only way that we thought of for hooking to automation for this. Another option would be to subscribe to PR review events and set this label whenever one non-approving event happens, although that might be too noisy and/or not work for single-comment reviews.

Basically the idea of the PR automation is to easily tell if the "ball is on the author's court" or in ours. So basically to know who is waiting on who, and who is responsible for the next action in order for the PR to move forward. Once the PR is open, then maintainers are expected to review, and that state is denoted by not having the "needs-author-action" label. Once the maintainer reviews the PR, then the label is applied, and so it is clear that we are now waiting on the author to comment/respond/address feedback. This helps to manage better that delay on the reviewer side, as maintainers can easily filter out and see all the PRs where the author is waiting on them (basically all Prs where that label isn't applied).

We are open to suggestions on how this could be better expressed/automated, but just wanted to more or less go through the thought process that went on while designing this.

jeffhandley commented 2 years ago

One concern that's been expressed about "needs-author-action" workflow on PR's is that sometimes the delay is on the reviewer side. I think we simply need to continue to hold reviewers accountable for being timely as well. We have been doing that with an internal mailer and I think it's improved.

As @joperezr and @eiriktsarpalis described, we would never have the "needs-author-action" label on a PR when the PR is in the reviewers' court. We've also been testing out some private GitHub project boards that area pod/owners could use to show their PRs in a view similar to how we look at issue triage boards. With those tools, the "which PRs are waiting on us" efficiency should be significantly improved, and PRs should go stale because of us far less often. The internal mailer is then our backstop only, not the primary mechanism for accountability.

As well we've discussed that we could sometimes do better at rejecting PR's early if they're not ones it makes sense to take.

The feedback from area owners is that right now this is just too hard to do because we don't have good structure around how/when/why to do that. Formalizing that PRs get triaged before review gives us a familiar mechanism for doing that (by making PR triage very similar to issue triage).

danmoseley commented 2 years ago

For PR's from regular committers, we today most often add comments (so there's work for the author) but sign off (because from past experience, we're confident they'll address them satisfactorily). For less familiar contributors, we generally don't do that -- we don't mark needs changes, but we don't mark approved either. It sounds like it's that second category that we'd likely more consistently mark needs changes. Seems like a reasonable experiment to me.

@stephentoub @jkotas any thoughts?

stephentoub commented 2 years ago

For regular contributors with merge rights, it's common to leave comments but sign off; after all, that contributor has the ability to merge themselves whenever they need to. They address the feedback and merge. There's no need in many cases for someone else to get involved, and I'm not sure we should be adding additional process around that. I can't tell if that's being suggested or not.

For regular contributors without merge rights, today someone with merge permissions needs to come along and merge it anyway, and they're responsible for validating that all changes were successfully addressed. I'm not sure how the proposed automation helps with that. Frequently today when I leave comments that do need to be addressed, I simply leave the comments and don't change the state of the PR. If the request is to always change the state, e.g. to leave comments and then change the state to changes requested, for such contributors that seems ok to try, though I'm a little skeptical of the overhead it'll add to the process. If I leave a comment here or there, am I required to change the state to changes requested? What if I'm not the assigned PR champion? What if it's a question rather than a change actually being requested? What if someone comments, changes the state to changes requested, and then I suggest that change not be made / offer a reason for why it's the way it is (it's not my PR)? Etc.

danmoseley commented 2 years ago

There is no reason to necessarily ask anyone to set "changes requested" any more than they do today. If they do, needs-author-action would applied; if they don't, they could apply the label manually (exactly as for issues today), or not use it at all. And then see how that experiment goes?

jkotas commented 2 years ago

I only use "changes requested" when the PR has something very questionable that I really do not want to see merged. It is very rare.

Note that "changes requested" state blocks merging, unless you are elevated to admin. It is mildly annoying when the PR is good to go, but it cannot be merged (without elevating to admin) because the person that flagged it as "change requested" is on vacation.

joperezr commented 2 years ago

yeah, I wouldn't force everyone to start using "changes requested" for every review, the only ask in order to better follow the process, would be to make sure that after you are done reviewing and you want to signal that you are now waiting on the author (aka the ball is not on your court any longer) just add the label manually. When the author comments, pushes new commits, or makes any change on the PR, the label will be automatically removed again, signaling you that you should probably take another look.

If I leave a comment here or there, am I required to change the state to changes requested?

@stephentoub the idea here is that this label really is more for you as a maintainer to better manage which PRs are waiting on you, and which ones aren't. The idea here is basically that, if the comments or questions you added means you are done reviewing, and now you want to wait on author's input (whether that be author's commit or just a comment as a response) then you add the label. That will help you better manage the PRs you need to review, as you will be able to easily just filter out all of the PRs which don't have that label to get all of the ones were you are not waiting on author's input.

jeffhandley commented 2 years ago

The idea here is basically that, if the comments or questions you added means you are done reviewing, and now you want to wait on author's input

This should also help other team members quickly/easily see that the ball is in the author's court. I know I've had times when I was unsure if a comment someone else made was "blocking" or not. Should I also review? It looks good to me; should I merge it? Wait, who's supposed to be doing what here? All of that should be more clear with the label applied, with or without changes requested.

vcsjones commented 2 years ago

The issue triage workflow document would include the following information Interested in any feedback here from our community on the issue and PR workflow here.

I would be interested in knowing if or how community triagers can contribute to this workflow.

Is there an eventual goal for say, triagers to close duplicates where we have high confidence it is a duplicate? What about applying needs-more-info where clearly the issue is waiting for input from the author?

I largely have a decent understanding of things going on in the area-System.Security). You do have some active community members that are able to help, and, it would seem useful to define a path which they (we) can contribute their efforts to it. It would probably be non-trivial to define very clear guidelines of what community members can and can't do in that regard, and how to remediate when mistakes are made, but I think an effort worth exploring.

huoyaoyuan commented 2 years ago

The triagers should get tagged here since we are doing a part of this job.

eiriktsarpalis commented 2 years ago

Good point, cc @SingleAccretion @tmds

eiriktsarpalis commented 2 years ago

For regular contributors without merge rights, today someone with merge permissions needs to come along and merge it anyway, and they're responsible for validating that all changes were successfully addressed. I'm not sure how the proposed automation helps with that.

That would be a concern addressed by having "PR champions" and not with any of the proposed review automation. Ultimately the PR champion is accountable for resolving the PR on time, and it is they that need to keep track of their open PR backlogs. Consequently the needs-author-action label and related automation is intended to be used by PR champions and not the casual reviewer that leaves a few comments and moves on. Casual reviewers might still apply the label if appropriate but ultimately it's only there to help champions.

It would probably be non-trivial to define very clear guidelines of what community members can and can't do in that regard, and how to remediate when mistakes are made, but I think an effort worth exploring.

Publishing a set of triage/review guidelines or playbook is something that @ericstj is looking at, and would complement a workflow document very nicely.

stephentoub commented 2 years ago

Ok, so to summarize, nothing would change here for anyone except for a PR's champion who might add that label. No one else need add the label, no one else need "request changes", etc. Yes?

By whom and when in the flow is a champion assigned?

eiriktsarpalis commented 2 years ago

Yes?

Yep. The goal is to reduce stale PRs and more clearly communicate expectations to community contributors, not change how reviews are being done.

By whom and when in the flow is a champion assigned?

By the area owners during triage meetings, as early as possible in the PR's lifetime.

eiriktsarpalis commented 2 years ago

I think this has been mentioned by @joperezr already, but I should clarify that this is not a proposal to mandate "request changes" in PR reviews, it is however recognizing that "request changes" is github's first-class representation of our needs-author-action label. I've generally avoided using "request changes" in the past, however I've recently been experimenting with applying it more liberally where it makes sense. I suspect it helps communicating expectations more clearly to the author.