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.

Sign up for GitHub

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

Jump to bottom

[RFC 0030] Formalize review workflow #30

Closed
wants to merge 2 commits into from
Closed

[RFC 0030] Formalize review workflow #30

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
921fd19
[RFC 0030] Formalize review workflow
timokau Aug 12, 2018
33fc5e6
Add tooling section
timokau Aug 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 147 additions & 0 deletions rfcs/0030-review-workflow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
---
feature: review_workflow
start-date: 2018-08-12
author: Timo Kaufmann
co-authors: (find a buddy later to help our with the RFC)
related-issues: (will contain links to implementation PRs)
---

# Summary
[summary]: #summary

Formalize a standard workflow for pull-request review and the lifecycle of a
pull request.

# Motivation
[motivation]: #motivation

Currently, there is not much process to PR-reviews. Basically some people go
through recent PRs regularly and review them. That has various drawbacks:

1. PRs can get lost if nobody reviews them in time and they end up too deep in
the list of recent PRs

2. PRs can be forgotten in the middle of the review cycle. Then the PR is already
too old to be discovered and additionally already has a lot of discussion,
making it look "handled"

3. Reviews of people without commit access are discouraged. When those people
review a PR, they essentially *lower* its chances to be merged because of
the same reason: by the time they are ready to merge, they look "handled" and
are less likely to get the attention of somebody with commit access.

4. It is hard to judge how many PRs actually need to be reviewed at the moment.
That kind of statistic would be interesting.

# Detailed design
[design]: #detailed-design

## PR states

Inspired by the [sage trac](https://trac.sagemath.org/query?desc=1&order=id&group=status&status=!closed&summary=~upgrad&or&status=!closed&summary=~updat),
I propose that we introduce a set of states a PR might be in. Every PR has to
be assigned to exactly one of these states and might change between these states
regularly. The most important states would be `needs:work` and `needs:review`.
`needs:work` means that the PR is not ready to be merged as-is and needs to be
improved. `needs:review` means that all the work is done and somebody (usually
the PR creator) thinks that it should be ready for merge. Reviewers (with or
without commit access) can then go through the list of `needs:review` PRs and
review them. They can than either request changes and set the state to
`needs:work` or merge the PR. In case of review by non-commiters, we could
introduce an additional state like `needs:merge`. Reviewing and merging those
PRs should then take little time for the committers and could double as a sort
of training program for potential new committers.

## Tooling

This workflow is not currently possible due to GitHub's restrictions. Namely
only people with commit access can set tags on a PR. This can be resolved
through tooling. Work on this is [in progress](https://github.com/NixOS/ofborg/pull/216).

The tooling should then initialize the PR status on new PRs. In general this
is going to be `needs:review`. One exception of this are PRs that are work in
progress (indicated by a `WIP` tag in the title). Those should start at
`needs:work`. Other exceptions may include RFC type PRs, which may warrant their
own state (`needs:opinions`?).

To reduce friction for new contributors, the tooling could later be expaned
to explain this process to first time contributors.

## Advantages

All the above issues should be resolved by this:

1. There is no reason anymore to preferably review recent PRs. Before, recent
commit were the only ones where one can be sure that they are not reviewed
yet (and thus in a `needs:work` state). Now, reviewers can randomly choose
and `needs:review` PR.

2. Similarly, the reviewer may change at any time. If the first reviewer forgets
or loses interest in a PR, the PR will just end up in the `needs:review` list
again and can easily find a new reviewer.

3. People without commit access can help reviewing commits and lessen the load
on committers without putting the reviewed PRs at a disadvantage.

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

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

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


> contributor: hello: 42.0 -> 43.0 [PR text]

[the bot automatically sets the new PR to `needs:review`]

> reviewer1: Looks good to me. Just one little nitpick: Please add a waving emoji to the name.
> @GrahamcOfBorg status needs:work

> contributor: Good point, more emojis are always good. Done.
> @GrahamcOfBorg status needs:review

> reviewer1: Thanks! I think this is ready for merge.
> @GrahamcOfBorg status needs:merge

> reviewer2: LGTM
> @GrahamcOfBorg build-and-merge

# Drawbacks
[drawbacks]: #drawbacks

- Contributing would be a bit more complicated. There is one more bit of
knowledge new contributors need. It is possible that some contributors forget
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>.


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

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.


# Alternatives
[alternatives]: #alternatives

- Move to a different issue tracker. That would be a great effort and most
more powerful issue trackers would also be unfamiliar to new contributors,
discouraging drive-by contributors.

- Do nothing. All the issues mentioned in the motivation remain unsolved.

# Unresolved questions
[unresolved]: #unresolved-questions

- List the exact states. We should probably start simple and evolve from there.

- Finish the tooling.

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

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.


# Future work
[future]: #future-work

Re-evaluate if the workflow had the predicted benefits.