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 from

Conversation

timokau
Copy link
Member

@timokau 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.

@Profpatsch
Copy link
Member

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

@7c6f434c
Copy link
Member

@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
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
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
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
Copy link
Member

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
Copy link
Member

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

@ryantm
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
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
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
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>.

Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@timokau
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
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
Copy link
Member Author

timokau commented Aug 15, 2018

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

@timokau
Copy link
Member Author

timokau commented Aug 15, 2018

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

@7c6f434c
Copy link
Member

7c6f434c commented Aug 15, 2018 via email

@timokau
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
Copy link
Member

7c6f434c commented Aug 15, 2018 via email

@timokau
Copy link
Member Author

timokau commented Aug 15, 2018

I updated the RFC text.

@zimbatm
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
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
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
Copy link
Member Author

timokau commented Aug 16, 2018 via email

@Ekleog
Copy link
Member

Ekleog commented Aug 16, 2018

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

@timokau
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
Copy link

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

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/discussion-pr-reviews/2619/5

Copy link

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all I like the effort made here. 👍

4. We can easily gather statistics. In the long run, we might even implement
some sort of automated triage where we regularly ping `needs:work` PRs and
close them after the authors went unresponsive for a while. Or we could
tag them as `needs:takeover`. But this is out of scope.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearification suggestion: This is out of scope of this RFC (also scratch out the "But")



An example PR might go like this. `contributor` creates the PR, `reviewer1`
reviews without commit access, `reviewer2` reviews with commit access.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Rewrite markup:

An example PR might go like this:

1. `contributor` creates the PR
1. `reviewer1` reviews without commit access
1. `reviewer2` reviews with commit access

GitHub.

- We rely even more on the tooling than we do now. However the general workflow
should be portable to different tooling and might even work better there.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearification needed: Is this (more tooling) actually a drawback? I don't think so, acutally! As long as it works and is properly documented, I don't mind only interacting with a bot from first patch to successfull merge!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this is a drawback because with a different platform such as e.g GitLab, the toolings will need a rewrite / adjustments to the new APIs etc.


- Write documentation. It should at least be explained in `CONTRIBUTING.md`,
maybe a greeting bot explaining it to first time contributors would be even
better.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually do both things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Once we decide we choose a tooling / start implement this, we should list this as a requirement.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/suggestion-to-help-maintainers-with-nixpkgs-prs-bloat/4794/2

@doronbehar
Copy link
Contributor

Hey all,

I like the ideas presented here as well and I wanted to mention some GitHub Apps & bots we may want to use once we agree that this should be merged:

If @timokau would like to add me as a co author I'd be happy to help :)

@timokau
Copy link
Member Author

timokau commented Nov 16, 2019

At this point you'd more likely be the main author, since I don't have much time to put into this. I don't think github makes it possible for me to give you access to this PR's branch, but do feel free to fork it and make a new PR :)

@doronbehar
Copy link
Contributor

@timokau I like the idea that many people have already joined the discussion here.. I think you can make GitHub allow us both to collaborate on this on your branch, i.e timokau/rfcs/tree/review-workflow. See this help article to add me to the the users allowed to push the review-workflow branch.

@timokau
Copy link
Member Author

timokau commented Nov 16, 2019

"Restrict who can push to matching branches." is not an option for me in the rules listing. Maybe that's only offered when multiple already have access to the whole repository. If you have other ideas feel free to contact me over IRC (timokau[m]) or email, the other people subscribed to this RFC probably don't care about the technicalities.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/steps-towards-even-more-pr-automation/5634/8

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-can-i-be-more-useful-at-reviewing-pull-requests/6731/2

@turion
Copy link

turion commented Apr 17, 2020

Hi, I'm a bit new to this. I recently experienced (again) that it's hard for a newcomer to review merge requests in a useful way. I like the ideas proposed so far, and I'd like to push this forward. @timokau recommended me to join this RFC as a coauthor in order to finish it, but I'm not entirely sure yet whether that's adequate. What I can certainly contribute is my judgement whether a process will help a newcomer.

@timokau
Copy link
Member Author

timokau commented Apr 17, 2020

I think that's perfectly adequate (although you already know that I think so of course). You are as qualified and as entitled to lead an RFC as anybody else.

In fact, the best ideas often come from people who aren't too stuck in the "old ways" yet.

@timokau
Copy link
Member Author

timokau commented Apr 17, 2020

That said, I'm no longer sure that the best way forward with this is directly through this RFC. It will be stuck in RFC-nitpicking hell for quite some time and the end-result will be untested. We badly need something now.

As an alternative, we could just implement an uninvasive, opt-in proof of concept now. As long as we do not expect or require the majority of people to adopt their workflows, I think we can do this without an RFC (or at least with a quick uncontroversial one).

We could develop a github bot that directs the PR lifecycle. It can instruct contributors and enable them to set tags. In the beginning, it should be strictly opt-in (@some-bot enable in the PR message or something) and as minimal as possible. That could mean just two functionalities in the beginning: @some-bot set needs-review and @some-bot set needs-work.

Then we could gather feedback, iterate on the implementation, slowly start advertising it. Once we're happy with it we can revive this RFC and see if people would like it to be the default workflow. At that point we would likely need to make further changes.

Normally I don't like it when people do the implementation before the RFC, as it can make rejecting the RFC harder. However given the urgency to come up with a better process and the generally positive response to this RFC I think we can do it here.

@turion
Copy link

turion commented Apr 17, 2020

It will be stuck in RFC-nitpicking hell for quite some time and the end-result will be untested.

Maybe we can agree at this point that we basically do want what you propose, and nitpick only so far as to cut it down to the essential? We shouldn't add big new features to what you're proposing, but make it slim.

As an alternative, we could just implement an uninvasive, opt-in proof of concept now.

If you or anyone else can do that, I'll happily test.

@timokau
Copy link
Member Author

timokau commented Jun 4, 2020

I started working on this here: https://github.com/timokau/marvin-mk2

Its not much more than "Hello World" and a name for now. Development will likely be slow. Its entirely possible that I never fish it. I'm only posting this here to avoid duplicate work.

My plan is to develop a Minimum Viable Product that

  • reads all PR messages
  • interprets some command that lets the PR author opt-in into testing the bot
  • when it is activated for a PR, writes one message explaining its function and then listens to 3 commands that switch the state between needs_work, needs_review and needs_merge

I hope that this will not be too much work and I think it can be very useful already. But, as I said, keep your expectations low.

@turion
Copy link

turion commented Jun 5, 2020

I started working on this here: https://github.com/timokau/marvin-mk2

That's awesome! Will you let us know (with some basic usage instructions) when it's possible to alpha-test it?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/rfc-marvin-mk2-the-pull-request-bot/7547/1

@timokau
Copy link
Member Author

timokau commented Jun 6, 2020

That's awesome! Will you let us know (with some basic usage instructions) when it's possible to alpha-test it?

Yes!

@Mic92
Copy link
Member

Mic92 commented Jun 6, 2020

@timokau We can host this on the nix-community infrastructure at some point if you want.

@timokau
Copy link
Member Author

timokau commented Jun 7, 2020

That would be great!

@timokau
Copy link
Member Author

timokau commented Jun 17, 2020

Status update: Its now hosted on nix-community hardware, installed on nixpkgs & ready for testing. See https://discourse.nixos.org/t/rfc-marvin-mk2-the-pull-request-bot/7547/5.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/stats-trends-on-github-issues-prs/7936/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet