ReviewBoard is now the official review tool for juju

[Eric Snow]

On Sun, Sep 14, 2014 at 10:28 PM, Menno Smits <menno.smits at canonical.com> wrote:
> Eric,
>
> Thanks for setting this up.
>
> Firstly, in your email you mention "rbt pull" several times. I think you
> mean "rbt post" right? I don't see a pull command documented anywhere.

Correct.  I meant "rbt post". :)

>
> I've run in to one issue so far. When I tried to get my first review in to
> Reviewboard today it took me a long time to figure out how to get it to
> generate the correct diff. After much gnashing of teeth I figured out that
> "rbt post" generates a diff by comparing "origin/master" against the current
> branch. This means that if you haven't updated your local master branch
> recently and pushed your local master branch to your personal fork on Github
> (this is the part I missed) you'll end up with diffs that include lots of
> changes that have already been merged and have nothing to do with your
> branch.
>
> As things stand the workflow actually needs to be:
>
> 1. Ensure your feature branch is rebased against upstream/master
> 2. Create a pull request like normal via github.
> 3. Switch to your local master branch.
> 4. "git pull" to update master
> 5. "git push origin master" to update your personal master on github.
> 5. Switch back to your feature branch ("git checkout -" helps here)
> 6. Run "rbt post" while at your branch to create a review request.
> 7. open the review request in your browser and "publish" it.
>   - alternately use the rbt --open (-o) and/or --publish (-p) flags.
> 8. add a comment to the PR with a link to the review request.
> 9. address reviews until you get a "Ship It!" (like normal, with LGTM).
> 10. add a $$merge$$ comment to the PR (like normal).
>
> This is a bit confusing and inconvenient. I can see us all forgetting to
> keep our personal master branches on GH updated.

Yeah, those steps are a lot, though keep in mind that effectively it's
only 2 steps more than before if you use the -p flag to rbt post and
were already keeping your local master up to date.

However, I'll look into a cleaner approach.  If rbt cannot handle it,
it may make sense to write a quick "git review" command that wraps all
that.  FYI, I've been using a "git refresh" command for a while that
wraps the pull/rebase steps, so writing one that does the extra review
steps shouldn't be too onerous.

All this is part of why I had considered waiting until we could roll
out proper github-reviewboard integration.  However, I felt like
ReviewBoard was enough of a benefit on its own that waiting for the
integration was unwarranted.

>
> It looks like the TRACKING_BRANCH option in .reviewboardrc could be helpful.
> It defaults to "origin/master" but if we changed it to "upstream/master" I
> suspect Reviewboard will then generate diffs against the shared master
> branch instead of what we might happen to have in master in our personal
> forks. The of course relies on every developer having a remote called
> "upstream" that points to the right place (which isn't necessarily true).

I'll take a look at that.

>
> If TRACKING_BRANCH isn't going to work then whatever automation we come up
> with to streamline RB/GH integration is probably going to have to sort this
> out.

Ultimately I think it's worth getting the tooling right in the very
short term. :)

-eric