New Code Review Process

Minimal Flow

This is the workflow expected from a new or inexperienced contributor, or when the change is small. The goal is to minimize friction. It is essentially the usual “GitHub flow”.

1. The contributor files a GitHub pull request with their change.
2. Given that the change is small and simple enough, review is performed in the pull request.
3. When the code is mostly acceptable it is merged by a maintainer or core team member. There are two variants on this:
   • It is fine as is (the code is good, the commit message conforms to standard, etc.): it can be merged with the merge button.
   • It needs minor tweaks (commit message, comments, spacing): the maintainer does a rebase and amend, then pushes the result and closes the PR with a comment “Merged in $someSha1”.

Full Flow, Future Fantasy Version

This is the workflow expected from experienced contributors when the change is large enough to warrant it. Or rather, this is what I imagine it could be, assuming some integrations to be written. I've focussed this on Crucible as in my recent testing that provides the superior environment -- I'll probably write something about that and wire up a demo soon enough.

1. The contributor pushes their change to a branch in their forked repository and creates a pull request on GitHub.
2. The contributor, or a maintainer, adds a comment “@st-review: review this” (or similar) on the pull request.
3. An integration on Crucible receives the resulting web hook and does the needful to create the review in Crucible. It comments on the GitHub pull request with “Review in https://reviews.syncthing.net/someurl”.
4. The review proceeds in Crucible. This step is awesome.
5. When the review is completed, an integration posts a summary comment to the pull request.
6. A maintainer adds a comment "@st-review: merge this" to the pull request, at which point a git merge --squash is done for the pull request branch, pushed to master (if successfull), and the PR closed.

Point 6 could also be just a regular "button merge", but if there are modifications to the PR along the review and it's not fully rebased this results in a cleaner history and offloads that work from the contributor.

Full Flow, Interim Version

Until we have the integrations, this would be what we would need to do manually. I'm not sure we should ever use this particular process as it may be cumbersome, but it illustrates what happens behind the scenes.

1. The contributor pushes their change to a branch in their forked repository.
2. If this is the first time using the full flow, two additional steps are required:
   • The contributor signs up for a Crucible account on https://reviews.syncthing.net/.
   • The contributor requests their forked repository be added for indexing at https://reviews.syncthing.net/. This would require a separate form and integration of some kind, or admin intervention as only admins can add new repositories.
3. The contributor creates the review request in Crucible. Actual process on this to be documented if we were to use this, but in principle it consists of filling out a form with a few comments about the change and indicating the source and destination branches.
4. The contribution is reviewed in Crucible.
5. When the contribution is accepted in Crucible, the contributor creates a pull request in GitHub, with the comment “Reviewed in https://reviews.syncthing.net/someurl”.
6. The contribution is merged.

Crucible Integration Requirements

The integration thing running Crucible-side needs to:

1. Receive web hooks from GitHub when a pull request is opened.
2. Add repositories to Crucible when the pull request repo isn’t already present and indexed.

When a pull request is commented on á la “@st-review: review this”, it should:

1. Forcefully reindex the repository, in case the branch we need was created five seconds ago.
2. Create a new review in Crucible. Here it should attempt to map the GitHub user name to a Crucible user name to set as the author. If that’s not possible, the process stops with a comment made on the pull request “User xyz not found in Crucible; please sign up and retry”.
3. Comment on the pull request with the URL to the newly created review.
4. Maybe set a “pending” status check on a “review” service, so that the pull request has a yellow blob on it, preventing early merge? This needs to be kept up when new commits are pushed.

When a pull request is closed, it should:

1. Post a comment on the pull request with the summary notes.
2. Set the status check to “review completed” if it was successful, or “review abandoned” if it was rejected outright.
3. Someone will close or merge the pull request as appropriate.

Extra Notes

Crucible is happiest in the absence of rebases. I think this is a bit lame, but on the other hand it’s a perfectly valid workflow to have the feature branch progress linearly and actually merge with master instead of being rebased on top of it. However the end result of that is not something we want to merge. We could add an integration so that one could say “@st-review: merge this” in a pull request and the integration would:

1. Fetch the latest master
2. Fetch the latest pull request branch
3. “git merge -squash” it on top of master, taking the pull request title and description as the commit message, or possibly allowing an override in the comment that contained the merge command.
4. Close the pull request with a comment on what happened.