Skip to content

Commit

Permalink
Propose a github review workflow
Browse files Browse the repository at this point in the history
Overhaul howto_contribute for a proposed github workflow. Not much to it... primarily deletion.

- Switch to a github pull request workflow.
- Added `PULL_REQUEST_TEMPLATE.md` to help encourage more useful PR messages.
- Added a blurb about being thoughtful/clear in comments as to which issues are blockers, and which are not.
- (independent of this change) Configured the pantsbuild/pants repo to only allow squash merges in PRs.
- Removed the `rbt` scripts.

Testing Done:
https://travis-ci.org/pantsbuild/pants/builds/177117555

Bugs closed: 3708, 3995

Reviewed at https://rbcommons.com/s/twitter/r/4333/
  • Loading branch information
stuhood committed Nov 19, 2016
1 parent 3d70ca5 commit e35dc2e
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 132 deletions.
13 changes: 13 additions & 0 deletions PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
### Problem

(_explain the context of the problem and why you're making this change. include
references to all relevant github issues._)

### Solution

(_describe the modifications you've made._)

### Result

(_describe how your changes affect the end-user behavior of the system. this section is
optional, and should generally be summarized in the title of the pull request._)
9 changes: 5 additions & 4 deletions rbt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#!/usr/bin/env bash

source $(dirname $0)/build-support/python/libvirtualenv.sh
setup_virtualenv 'rbt' 'RBTools==0.7.5' \
'--allow-external RBTools'
exec rbt "$@"
echo "Pants has switched to a github PR based workflow!"
echo "See http://www.pantsbuild.org/howto_contribute.html#code-review for more info."
echo

exit 1
23 changes: 1 addition & 22 deletions rbt-create
Original file line number Diff line number Diff line change
@@ -1,24 +1,3 @@
#!/usr/bin/env bash

# Convenient shortcut script for creating a new Pants rbt review:
#
# ./rbt-create <name> ... <name> <other flags>
#
# will create a review with the specified names as reviewers, with pants-reviews as a group,
# and with the other flags passed through to rbt.

NAME_REGEX="^[a-zA-Z][a-zA-Z_.-]+$"
REVIEWERS=""

while [[ $1 ]] && [[ $1 =~ ${NAME_REGEX} ]]; do
REVIEWERS="${REVIEWERS},$1"
shift
done

REVIEWERS=${REVIEWERS:1} # Strip leading comma.

if [[ ${REVIEWERS} ]]; then
REVIEWERS_FLAG="--target-people=${REVIEWERS}"
fi

./rbt post -o -g ${REVIEWERS_FLAG} "$@"
./rbt
8 changes: 1 addition & 7 deletions rbt-update
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
#!/usr/bin/env bash

# Convenient shortcut script for updating an existing Pants rbt review:
#
# ./rbt-update <other flags>
#
# will update the existing review, passing the flags through to rbt.

./rbt post -o -u "$@"
./rbt
144 changes: 45 additions & 99 deletions src/docs/howto_contribute.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,6 @@ level, the steps are:
- Get a code review.
- Commit your change to master.

Please note--despite being hosted on GitHub--we do not use pull
requests to merge to master; we prefer to maintain a linear commit
history and to do code reviews with Review Board. You will however
need to create a Github pull request in order to kick off CI and test
coverage runs.

### Identify the change

It's a good idea to make sure the work you'll be embarking on is
Expand Down Expand Up @@ -115,13 +109,13 @@ You can always run the pre-commit checks manually via:
:::bash
$ ./build-support/bin/pre-commit.sh

**Pro tip:** If you want your local master branch to be an exact copy of
**Pro tip:** If you are certain that you have not accidentally committed anything to
the `master` branch that you want to keep, and you want to reset to an _exact_ copy of
the `pantsbuild/pants` repo's master branch, use these commands:

:::bash
$ git co master
$ git fetch upstream
$ git reset --hard upstream/master
$ git checkout master && git reset --hard upstream/master

### Making the Change

Expand Down Expand Up @@ -149,112 +143,64 @@ run relevant tests. If you're not sure what those are,

### Code Review

Now that your change is complete, post it for review. We use `rbcommons.com` to host code reviews:
Now that your change is complete, post it for review. We use `github.com` pull requests
to host code reviews:

#### Posting the First Draft

**Before posting your first review,** you need to both subscribe to the [pants-reviews
Google Group](https://groups.google.com/forum/#!forum/pants-reviews) and create an
[RBCommons](https://rbcommons.com) account. Its critical that the email address you use for each
of these is the same, and it's also recommended that you have that same email address registered as
one of your [email addresses](https://github.com/settings/emails) with Github.

_A special warning to `@twitter.com` contributors:_ The twitter.com email domain does not permit
emails being sent on behalf of its members by RBCommons. As such, you should use a personal email
address or some other non-`@twitter.com` email address to subscribe to both RBCommons and
pants-reviews.

To create your RBCommons account, visit <https://rbcommons.com/account/login/> and click "Create
one now.". To sign up for [email protected], just browse to
<https://groups.google.com/forum/#!forum/pants-reviews/join>.

To set up local tools, run `./rbt help`. (`./rbt` is a wrapper around
the usual RBTools [rbt](http://www.reviewboard.org/docs/rbtools/dev/)
script.) The first time this runs it will bootstrap: you'll see a lot of
building info.

Before you post your review to Review Board you should <a
pantsref="dev_run_all_tests">create a Github pull request</a> in order
to kick off a Travis-CI run against your change.

Post your change for review:

:::bash
$ ./rbt post -o -g

The first time you `post`, rbt asks you to log in. Subsequent runs use
your cached login credentials.

This `post` creates a new review, but does not yet publish it.
When <a pantsref="dev_run_all_tests">all of the tests are green on travis</a>, you're
probably ready to request review for the change!

At the provided URL, there's a web form. To get your change reviewed,
you must fill in the change description, reviewers, testing done, etc.
To make sure it gets seen by the appropriate people and that they have
the appropriate context, add:
To get your pull request reviewed, you should:

- `pants-reviews` to the Groups field
- Any specific [pants committers](https://www.rbcommons.com/s/twitter/users/)
who should review your change to the People field
- The pull request number from your Github pull request in the Bug field
- Your git branch name in the Branch field.
- Include a short and descriptive title.
- Fill in each of the sections of the pull request template, and include links to any
relevant github issues for the change.
- Mention (usually in a comment on the PR) any specific
[pants committers](https://github.com/orgs/pantsbuild/teams/committers)
who should review your change. Running `git log -- $filename` on one or more of the files that
you changed is a good way to find potential reviewers!

When the review looks good, publish it. An email will be sent to the
`pants-reviews` mailing list and the reviewers will take a look. (For
your first review, double-check that the mail got sent; rbcommons tries
to "spoof" mail from you and it doesn't work for everybody's email
address. If your address doesn't work, you might want to use another
one.)

Note that while only committers are available to add in the People field,
users with an rbcommons account may still post reviews if you provide
them with a link manually or they see it in the google group.
Finally, when the review is ready for attention, add the `reviewable` label: this is the
signal to committers and contributors that the review is ready for attention. You can see
a list of all actively `reviewable` reviews [here](https://github.com/pantsbuild/pants/labels/reviewable).

#### Iterating

If reviewers have feedback, there might be a few iterations before
finally getting a Ship It. As reviewers enter feedback, the rbcommons
page updates; it should also send you mail (but sometimes its "spoof"
fails).

If those reviews inspire you to change some code, great. Change some
code, commit locally. To update the code review with the new diff where
`<RB_ID>` is a review number like `123`:
If reviewers post any feedback
([for more information on providing feedback see](https://help.github.com/articles/reviewing-proposed-changes-in-a-pull-request/)),
there might be a few iterations before finally getting a Ship It. As reviewers enter
feedback, the github page updates; it should also send
you mail as long as you are `Subscribed` to notifications for the pull request.

:::bash
$ ./rbt post -o -r <RB_ID>
Github pull requests currently lack the ability to mark a review comment as "blocking"
for a change, so it's important for contributors and reviewers to communicate clearly which
comments they view as blocking. If it's unclear whether a reviewer feels strongly about
a particular point, please bias toward clearing up the uncertainty before proceeding.

Look over the fields in the web form; perhaps some could use updating.
Press the web form's Publish button.

If need a reminder of your review number, you can get a quick list with:
If those reviews inspire you to change some code, great. Change some
code and commit locally. When you're ready to update the pull request with your changes,
push to the relevant branch on your fork as you did before:

:::bash
$ ./rbt status
r/1234 - Make pants go even faster
$ git push <your-username> $FEATURE_BRANCH

### Commit Your Change
Look over the fields in the pull request you created earlier; perhaps some could use updating.
Press the web form's `edit` button.

At this point you've made a change, had it reviewed and are ready to
complete things by getting your change in master. (If you're not a
committer, please ask one to do this section for you.)
If at any point you need to make changes that will fundamentally overhaul a review,
consider temporarily removing the `reviewable` label in order to let reviewers know
to hold off until the code is ready.

:::bash
$ cd /path/to/pants/repo
$ ./build-support/bin/ci.sh
$ git checkout master
$ git pull
$ ./rbt patch -c <RB_ID>

Here, ensure that the commit message generated from the review summary
is accurate, and that the resulting commit contains the changes you
expect. (If `rbt` gives mysterious errors, pass `--debug` for more info.
If that doesn't clarify the problem, mail pants-devel (and include that
`--debug` output).)
### Committing a Change

Finally,
At this point you've made a change, had it reviewed (and received one or more Ship Its!) and
are ready to complete things by getting your change in master. (If you're not a
committer, please ask one to do this section for you.)

:::bash
$ git push origin master
A committer should push the `Squash and merge` button on the PR, and ensure that the
commit message generated from the review summary is accurate. In particular, the title should
contain a useful summary of the change, and the description should follow the pull request
template (defined in the root of the repo at `PULL_REQUEST_TEMPLATE.md`).

The very last step is closing the review as "Submitted". The change is
now complete. Huzzah!
Finally, the committer will select `Confirm squash and merge`. The change is now complete. Huzzah!

0 comments on commit e35dc2e

Please sign in to comment.