Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC 0030] Formalize review workflow #30

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
@timokau
Copy link
Member

timokau commented Aug 12, 2018

Rendered

This is an attempt to improve the current PR-review situation by introducing different PR states and the tooling to change between them.

@timokau timokau force-pushed the timokau:review-workflow branch from dcc0250 to 921fd19 Aug 12, 2018

@Profpatsch

This comment has been minimized.

Copy link
Member

Profpatsch commented Aug 13, 2018

Could we add some kind of “needs two reviewers for non-trivial code” in the process?

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Aug 13, 2018

@Profpatsch this addition would make the primary valuable goal of the RFC (manage backlog) even harder, though…

Re: friction with different tooling: there are plugins for GitHub login integration, so at least «registering elsewhere» can be made cheaper for a maintenance price. Given recent events with some people actively deleting GitHub accounts, self-hosted tool that accepts GitHub/GitLab/BitBucket/Twitter logins and email-based logins could be a lower barrier of entry w.r.t. registration…

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 13, 2018

@Profpatsch with this RFC it would be easier to do that at the first reviewers discretion. They could approve the PR and re-set to needs:review (or maybe needs:merge). While I support that as a general policy, I think that would better be discussed separately. It is easier to decide on multiple small changes and I don't want to overload the brittle rfc process ;)

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 13, 2018

@7c6f434c I think you might underestimate the amount of familiarity with a GitHub/GitLab workflow. I think using a tool that works differently is a significant barrier. I have no data to back that up though and this is probably also not the best place to discuss it. Note that I include GitLab, because it works basically the same way as GitHub and additionally has a lot of community goodwill right now.

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 13, 2018

@zimbatm I'm taking the liberty to move your comment here, its better not to fragment the discussion.

I think it's a good idea! Improving speed and review consistency would be great.

Have you thought of creating a new frontend for it? It could even be gamified a bit with a score of reviewers. A lot of that information could be inferred by the state available on GitHub:

  • needs:review: a PR whose last commit doesn't have an approved review
  • needs:work: a PR whose last review requested changes
  • needs:merge: a PR whose last commit has an approved review

Maybe it's even possible to do that using the github issues filter.

Glad you like the idea :)

I didn't think about a frontend yet, no. Why do you think that might be necessary? What kind of frontend do you imagine?

Automatically inferring the states is an interesting idea. Probably not entirely reliable though. I could imagine that in the long run it might be used to supplement user-set tags. Can non-committers even approve / request changes? And are approvals actually associated with a commit? What if somebody approves and someone else requests changes?

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Aug 13, 2018

Non-committers can review, their review are shown in gray in the review summary, review conflicts happen even between committers anyway — GitHub just mentions there are different reviews.

@bobvanderlinden

This comment has been minimized.

Copy link

bobvanderlinden commented Aug 13, 2018

This is also very much related to pull requests becoming stale: NixOS/nixpkgs#41793. There was quite a discussion there already.

@ryantm

This comment has been minimized.

Copy link
Member

ryantm commented Aug 13, 2018

I try to do something similar with existing GitHub search features:

PR with no review
PR review approved
PR review changes requested

This doesn't work that well when other contributors request changes in a comment instead of in a review.

@timokau timokau referenced this pull request Aug 14, 2018

Merged

Call for content: 2018/06 #61

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 14, 2018

@bobvanderlinden I think many of the PRs you listed there wouldn't necessarily be solved by this RFC. They are default changes where no single commiter feels comfortable on making the decision. For pushing those forward, resolving all known issues and then requesting comments on discourse might be appropriate. After some time without new concerns, it would be easier to merge those.

@ryantm that is a good start! But yes, I think manual setting of states would be better.

@zimbatm

This comment has been minimized.

Copy link
Member

zimbatm commented Aug 14, 2018

@timokau for the frontend it could start as a static page that contains links of Ryan's filters and then evolve from there. Update the nixpkgs README with: "Want to contribute? Help us by reviewing and sorting PRs at https://nixpkgs-review.herokuapps.com". The implementation doesn't matter as much as having a unified entry point to do the reviews. As we start using it we can then augment it with more useful things, patch ofborg to help the review page, ... That may the process is more dynamic and lives in code instead of documentation.

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 14, 2018

@zimbatm that sounds like a lot of work, close to implementing our own issue tracker. Also keeping PR metadata/interface close to the PRs itself is valuable.

That may the process is more dynamic and lives in code instead of documentation

Why would it be different?

to set the PR to `needs:review` again after a fixup. That might be resolved
by some form of automatic triage as explained above. It would also be not much
than the current status, since the first reviewer will still be notified by
GitHub.

This comment has been minimized.

@Mic92

Mic92 Aug 15, 2018

Is it possible to figure out if the First contributor batch on github is set for a pull request? Then additional help could be shown to first timers.

This comment has been minimized.

@timokau

timokau Aug 15, 2018

Author Member

Yes, that would be cool. @grahamc do you know if the GitHub api allows that?

This comment has been minimized.

@7c6f434c

7c6f434c Aug 15, 2018

Member

I think it has to be a setting, not even API — this should change what is shown before submission of the PR, there is no event to be notified about at that point…

After submission The Tool could check if the submitter is known and post a link/summary of review process.

This comment has been minimized.

@timokau

timokau Aug 15, 2018

Author Member

I imagined something like a message of our friendly bot:

Hi! Looks like this is your first contribution. Glad to have you! I automatically set the status of this pull request to `needs:review`. That tells reviewers that you consider this PR good to go. If they request changes, they will change the status. If you at any time want to change the status again (e.g. change it back to `needs:review` after implementing requested changes), you can ask me to do it with @\grahamcOfBorg status needs:review.

Also make sure you read the whole <link to contributing.md>.
@Mic92

Mic92 approved these changes Aug 15, 2018

Copy link

Mic92 left a comment

I agree with workflow proposed here. I would set the initial state of a new pull request to needs:review automatically.

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 15, 2018

I would set the initial state of a new pull request to needs:review automatically.

I agree. I implicitly did that in the example, but did not make it explicit in the RFC description.

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Aug 15, 2018

I guess immediate needs:review should be conditional on «unless WIP or RFC tags are present». RFC tag should cause something slightly different like needs:opinions

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 15, 2018

Yes, good point. I will amend the RFC text.

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 15, 2018

Although don't RFC prs in nixpkgs generally need review?

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Aug 15, 2018

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 15, 2018

In that case needs:work doesn't seem appropriate either right? Shouldn't those PRs be in the rfc repo?

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Aug 15, 2018

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 15, 2018

I updated the RFC text.

@zimbatm

This comment has been minimized.

Copy link
Member

zimbatm commented Aug 15, 2018

@timokau I don't mean to push more work onto you. I you see a shorter path of implementation by changing ofborg then it's probably fine as well. For me the value is really extracted once there is a unified way of reviewing PRs for everybody. It makes it clearer how to participate and makes the process more consistent. The implementation is just a detail.

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 15, 2018

@zimbatm of course, I'm just happy I got a conversation going :) All feedback is welcome.

In case we want a frontend, I couldn't do the implementation anyways. Absolutely no experience there. But forgetting all implementation costs (which I think is valuable to figure out the best possible solution before making a reality check), I'm not 100% convinced of the added value either.

Yes the unification and streamlining is really the most important part, independent of the details.

@zimbatm

This comment has been minimized.

Copy link
Member

zimbatm commented Aug 16, 2018

:) Just reflecting on this, how did you learn about the best way to review PRs in nixpkgs?

I think the current shortest path is to find the CONTRIBUTING.md document (I think it only shows to new contributors in the UI). The document is heavily tilted towards creating more PRs instead of helping with the reviews.

It then links to the manual which explains how to review the PRs. It doesn't give any advice on how to filter the PRs.

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Aug 16, 2018

@Ekleog

This comment has been minimized.

Copy link
Member

Ekleog commented Aug 16, 2018

@zimbatm The CONTRIBUTING.md is linked to in the checklist of every new PR :)

@Mic92

This comment has been minimized.

Copy link

Mic92 commented Aug 16, 2018

I do nix-review pr <number> after I looked at the change and then look at the pull request description to see what the author has done regarding testing.

@timokau timokau referenced this pull request Aug 29, 2018

Open

[WIP] Labels #216

0 of 3 tasks complete
@ryantm

This comment has been minimized.

Copy link
Member

ryantm commented Sep 11, 2018

I am not in favor of this RFC in its current form because I think it
is too complicated. I do not think we need any additional tooling to
handle PRs in a better way. I think GitHub provides plenty of tooling
for handling our PRs, we just need to have clear plan for using it
effectively.

I am excited to co-author this if @timokau is open to changing it
along the lines I propose below.

I think we should add instructions like below to the CONTRIBUTING.md or a
page that is linked from there:

PR review workflows

PR needs review

  1. Visit
    https://github.com/NixOS/nixpkgs/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+review%3Anone+sort%3Acreated-asc+-label%3A%222.status%3A+work-in-progress%22+-label%3A%222.status%3A+merge+conflict%22
    and work from oldest PR to newest PR. (First come, first served.)

  2. If the PR is a work in progress and does not have label "2.status:
    work-in-progress" either add this label, or if you do not have
    permission, add a "Request changes" review with a "Review
    Summary" of "PR triage: work-in-progress". Move on to the next PR.

  3. If the PR currently does not merge and does not have label
    "2.status: merge-conflict" either add this label, or if you do not
    have permission to, add a "Request changes" review with a "Review
    Summary" of "PR triage: merge-conflict". Move on to the next PR.

  4. If the last activity on the PR is a comment that is not from the
    pull requester, and you agree with their comments, add a "Request
    changes" review with a "Review Summary" of "PR triage: please
    address issues rasied by commenters". Move on to the next PR.

  5. If @GrahamcOfBorg has an unexpected build failure on the PR, add a
    "Request changes" review with a "Review Summary" of "PR triage:
    please fix OfBorg build". Move on to the next PR.

  6. If you feel that you are capable of code review, do a normal GitHub
    code review. If you suggest any changes at all, choose "Request
    changes". If looks good to you, choose "Approve". If you can, tell
    @GrahamcOfBorg to build the PR. Move on to the next PR.

  7. If you feel you cannot process this PR, move on to the next
    PR. Someone else will process it later.

PR needs second review

  1. Visit
    https://github.com/NixOS/nixpkgs/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+review%3Aapproved+sort%3Acreated-asc+-label%3A%222.status%3A+work-in-progress%22+-label%3A%222.status%3A+merge+conflict%22+
    and work from oldest PR to newest PR. (First come, first served.)

  2. If @GrahamcOfBorg has not built this PR, ask it to. Revist after it
    has built.

  3. If it also looks good to you and doesn't seem major enough to
    warrant more discussion, merge it.

Following up on PRs you approved

  1. Visit
    https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+%22init+at%22+-label%3A%222.status%3A+work-in-progress%22+-label%3A%222.status%3A+merge+conflict%22+reviewed-by%3Aryantm+review%3Aapproved+sort%3Acreated-asc
    change "ryantm" to your GitHub username, and work from oldest PR to newest PR. (First come, first served.)

  2. If there are no changes, @GrahamcOfBorg has built it, and it has
    been at least three days, merge it.

Following up on PRs you requested changes on

  1. Visit
    https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+%22init+at%22+-label%3A%222.status%3A+work-in-progress%22+-label%3A%222.status%3A+merge+conflict%22+reviewed-by%3Aryantm+review%3Achanges-requested+sort%3Aupdated-asc
    change "ryantm" to your GitHub username, and work on the least recently updated PR first.

  2. If there has been no updates since you first reviewed the PR, and
    30 days have elapsed, close the PR with comment "PR triage: closing
    this PR because there has been no resposne to requsted changes in
    30 days. We can open it later, if you ask us to.", or if you do not
    have permission, add a comment asking for it be closed: "PR triage:
    please close this PR because there has been no response to
    requested changes in 30 days."

this comment is also available at https://gist.github.com/ryantm/44431aba6b15519cdcccd191525bd0ac

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Sep 11, 2018

I'm definitely willing to change and improve the RFC. However, I'm not convinced your suggested workflow is an improval. If anything, it seems more complicated to me. Basically everything I suggest to be done by a bot would instead be done by a mix of pre-defined strings (essentially bot commands) in PR-reviews and manual reviewers ("bots") executing these. And what is a PR-owner to do if somebody tagged their PR as work-in-progress or merge-conflict and it is now hidden from all the suggested searches?

@ryantm

This comment has been minimized.

Copy link
Member

ryantm commented Sep 11, 2018

Cool. Your point about the tags is a good one. I think the best approach here is to just not use those labels. I'll rewrite it getting rid of those. I'll also write a better description of how the reviews would work independent of all the specific advice about what to do with each specific PR.

@ryantm

This comment has been minimized.

Copy link
Member

ryantm commented Sep 11, 2018

Well darn. I should have tested this more before writing that up. It appears that for the purposes of my proposed searches, GitHub ignores the reviews not from collaborators. There are subtle UI cues: It says "x suggested changes" vs "x requested changes".

Okay, I am back to thinking the bot approach is probably going to work best for us! I'll try to help you make it happen.

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Sep 11, 2018

Glad we a agree :) I also truly believe the bot workflow is simpler. It can also be expanded with UX features like reminding users they can set "needs:review" themselves when a PR has been in "needs:work" for a while.

Given the general agreement here, I think this is only blocked on the relevant bot capability (and at least formally a co-author). We could then begin with a minimal future set and expand it if it works well.

@edolstra edolstra changed the title Formalize review workflow [RFC 0030] Formalize review workflow Jan 10, 2019

@domenkozar

This comment has been minimized.

Copy link
Member

domenkozar commented Jan 31, 2019

@timokau are you interested to still be the author? We've reviewed this at RFC steering committee and could open for nominations to move it forward down the process.

@globin globin added the status:new label Feb 7, 2019

@edolstra

This comment has been minimized.

Copy link
Member

edolstra commented Feb 14, 2019

We'll close this for now since we didn't hear back from the author. Please reopen if there is interest!

@edolstra edolstra closed this Feb 14, 2019

@timokau

This comment has been minimized.

Copy link
Member Author

timokau commented Feb 14, 2019

Sorry, got lost in my todo list. I'll revisit when I have time.

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Feb 16, 2019

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/code-of-conduct-or-whatever/2134/14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.