Github + Bugzilla workflow with automation

[James Lal]

Hey All,

After spending some time thinking and talking with others, I would like to
affect some changes to our workflow.
Not only will this affect existing developers, but also our new
contributors and uplift system.
I would like your feedback on this idea to make sure we are all in
agreement.

Since our initial gaia days (over a year ago now) we have made some big
changes to how we develop.
Our current workflow looks something like this:

1. file a bug
2. create a topic branch
3. commit with "Bug XXX - desc"
4. submit pull request
5. wait for travis to pass
6. link pull request to bugzilla
7. r? on bugzilla
8. review
9. r+ on bugzilla
10. land patch
11. copy hash from landed patch and paste into bug and mark the bug
RESOLVED + FIXED.

< semi-manual uplifting >

There are a lot of extra manual steps here that complicate our development
setup.
Even when this does work well we still have complications when uplifting
code.

There are other issues: people merging before travis passing, merge commits
which typically we don't want, and newcomers not having any clue how to do
steps 6, 7, 11, etc..

I think the solution here is to remove a lot of the manual steps with a
"bot" (I am going to omit implementation details here).
This bot would automate a lot of the flow here and make the github <->
bugzilla integration smoother.

Assuming we have this bot working this would be your workflow:
(steps in brackets are automatic steps the bot does)

1. file a bug
2. create a topic branch
3. commit with "Bug XXX - desc"
4. submit pull request
[5.] Bot links pull request to bugzilla or comments on the format of your
invalid pull request commit
[6.] Bot sets reviewer based on the r=X flag if present in your first commit
5. review
6. r+ patch
7a. add checkin-needed to indicate your bug needs to land (instead of r+
=== land)
[7.] bot automatically lands patch on r+/checkin-needed (your r+ has
semantic value meaning this code can land).
[8.] mark bug resolved + fixed
[9.] keep track of commit hash of the merged/rebased/cherry-picked? pull
request

<completely automatic uplifting>

What this would do is strictly enforce landing policies while saving
developer time, and it will cover 90% of all cases.
This would also mean we can stop using the merge button and rely on the bot
to do the work for us.

There are people on the gaia team as well as the a-team who are interested
in supporting the development of this concept.
Assuming no major objections are raised we would begin development in the
next 1-4 weeks.

Feedback ? Questions? Would this make your life easier?
----

Some implementation details:

This would work via a combination of turning off the merge button for most
people, github webhooks, bugzilla notification apis [I spoke to glob about
this stuff].
Eventually this flow would replace our currently uplifting strategies with
an entirely automatic flow.
Once a patch gets a branch specific "+" the code will automatically land on
that branch.

The code would be developed on github and consume itself as a early test.

GELAM and the js related marionette repos would opt into this work very
early on to make sure its stable before anything would be turned on for
gaia.

[Dale Harvey]

This all sounds awesome (can we kill merge commits with this workflow this
as well? :))

One issue I always come up with it the awkwardness of putting r= in commit
messages, reviewers change, you very often dont know who is reviewing a
commit until after you have made the PR, also for new contributors bugzilla
comes up with a nice helper to suggest who should review

But if this automatically attaches pull requests to their specified bug,
then the user can go into bugzilla and flag a review manually, with whoever
pushing the commit (either us or the bot) making sure of the r=, that
sounds great

> _______________________________________________
> dev-gaia mailing list
> dev-...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-gaia
>

[James Lal]

We can kill merge commits with this as well. Its part of the goal actually
to make our mirroring, uplifting, etc... concerns easier to deal with.

The r= thing is up in the air- I thought it would be kind of cool to be
able to manage this via the commits but since the bot is responsible for
landing all the things it could very easily make sure that this is up to
date.

[Jason Smith]

Doesn't one of the webdev teams (maybe mozilla.org) use something like this process you are proposing? If so, can we reuse the bot implementation they have already built?

[James Lal]

I am not aware of any such tooling. If it exists we should attempt to use
it. Our current plans are essentially to build on top of a bunch of
existing open source tooling.

[Axel Hecht]

Hi,

I'd invite you guys to take in all the discussion that happened in
mozilla.dev.platform over the past few months regarding landing strategies.

A lot of conceptional challenges discussed around the various inbound
branches and mozilla-central seem to in common with this idea.

It covers topics like "if you patch is good and passes tests based on X,
what does that mean for it on Y". Test coverage and test reliability go
into that, among many other things.

Axel

[Robert Helmer]

I am on the former webdev team called "Webtools" (now Web Engineering), we
along with other parts of webdev have been working on and using a set of
tools to integrate github and bugzilla for quite a while. We work on a lot
of infra like crash-stats, l10n tools, new telemetry dash, etc.

A few of the more useful ones are:

* leeroy (a bot that does jenkins CI runs for github pull requests - it
uses the github status API, and works in a similar way to the travis bot)
* ddash's github robot - can comment and auto-close bugs based on github
commits (bug # and closing behavior is based on the comment used)
* various extensions such as https://github.com/pmclanahan/github-list-bugs/

cc'ing lonnen who has worked on this and other tools, he may have something
to add/correct above. I have had more than one suggestion that we do a
brownbag or some other kind of recording to explain more about the
bugzilla+github workflow we've been using. We'd love to chat with whomever
will do the gaia work here too and get more cross-pollination going (I do
expect that our workflow would not be drop-in for gaia project)

On Fri, Aug 9, 2013 at 12:25 PM, James Lal <ja...@lightsofapollo.com> wrote:

> I am not aware of any such tooling. If it exists we should attempt to use
> it. Our current plans are essentially to build on top of a bunch of
> existing open source tooling.
>
> On Fri, Aug 9, 2013 at 12:15 PM, Jason Smith <jsmith....@gmail.com
> >wrote:
>

[Gabriele Svelto]

Hi James, all,

> 1. file a bug
> 2. create a topic branch
> 3. commit with "Bug XXX - desc"
> 4. submit pull request
> [5.] Bot links pull request to bugzilla or comments on the format of your
> invalid pull request commit
> [6.] Bot sets reviewer based on the r=X flag if present in your first commit
> 5. review
> 6. r+ patch
> 7a. add checkin-needed to indicate your bug needs to land (instead of r+
> === land)
> [7.] bot automatically lands patch on r+/checkin-needed (your r+ has
> semantic value meaning this code can land).
> [8.] mark bug resolved + fixed
> [9.] keep track of commit hash of the merged/rebased/cherry-picked? pull
> request

this would definitely be an improvement over our existing strategy but it makes me wonder why don't we simply adopt our existing Bugzilla Gecko workflow for Gaia too, getting rid of pull requests entirely, attaching patches to bugs directly and enforcing commit access level like we do for the Mercurial repos. This might require more work upfront but would ensure we have only one contribution flow and not two. Besides I dislike reviews on GitHub; when looking them up from an old bug it's hard to follow how the patch evolved; sometimes it's even hard to figure out if what landed was what had been reviewed. Having reviews there also means that we rely on GitHub for storing information which really belongs on Bugzilla and if we'd ever want to do some data-mining on it (for automatic review suggestions for example?) we can't because it's not in our databases.

As a final remark, this doesn't mean I wouldn't like the approach you're suggesting implemented; in fact whatever improvement we can make over the current system is _very_ welcome to me!

Gabriele

[Julien Wajsberg]

Le 11/08/2013 13:16, Gabriele Svelto a écrit :
> Hi James, all,
>
>> 1. file a bug
>> 2. create a topic branch
>> 3. commit with "Bug XXX - desc"
>> 4. submit pull request
>> [5.] Bot links pull request to bugzilla or comments on the format of your
>> invalid pull request commit
>> [6.] Bot sets reviewer based on the r=X flag if present in your first commit
>> 5. review
>> 6. r+ patch
>> 7a. add checkin-needed to indicate your bug needs to land (instead of r+
>> === land)
>> [7.] bot automatically lands patch on r+/checkin-needed (your r+ has
>> semantic value meaning this code can land).
>> [8.] mark bug resolved + fixed
>> [9.] keep track of commit hash of the merged/rebased/cherry-picked? pull
>> request
> this would definitely be an improvement over our existing strategy but it makes me wonder why don't we simply adopt our existing Bugzilla Gecko workflow for Gaia too, getting rid of pull requests entirely, attaching patches to bugs directly and enforcing commit access level like we do for the Mercurial repos. This might require more work upfront but would ensure we have only one contribution flow and not two. Besides I dislike reviews on GitHub; when looking them up from an old bug it's hard to follow how the patch evolved; sometimes it's even hard to figure out if what landed was what had been reviewed. Having reviews there also means that we rely on GitHub for storing information which really belongs on Bugzilla and if we'd ever want to do some data-mining on it (for automatic review suggestions for example?) we can't because it's not in our databases.

I'd like to +1 what Gabriele is saying here.

If something gets more automatic, I would love to see the current patch
attached to bugzilla too. The PR URL would go in the comment. The tricky
point is: when do you obsolete the old one and push a new patch to
bugzilla ? Can we have github triggers based on a comment on github ?

I don't mind merge commits so much, we can just ignore them with git
commands anyway, eg "git log --no-merges". However clearly our current
process has some overhead. As a dev I woud be happy enough with a
command-line tool that would attach tthe patch to bugzilla _and_ open a
PR on github if not open already. As a matter of fact I'd rather have
this command line tool than something that would attach stuff to my bugs
automatically.

However I really like the idea of a bot commiting patches based on
r+/checkin-needed.

--
Julien

[Etienne Segonzac]

On Fri, Aug 9, 2013 at 7:09 PM, Dale Harvey <da...@arandomurl.com> wrote:

> One issue I always come up with it the awkwardness of putting r= in commit
> messages, reviewers change, you very often dont know who is reviewing a
> commit until after you have made the PR, also for new contributors bugzilla
> comes up with a nice helper to suggest who should review
>

+1
Especially since the current workflow advocates against putting the "r=" in
the commit message before the review is actually granted.

On a more general note, focusing on automating the "landing and uplifiting
of green and r+ed patch" sounds more manageable than automating "all the
things!" :)
--
Etienne

[John Ford]

If we get rid of merge commits entirely, we'll almost certainly have to alter the commit metadata as it's rebased onto master branch [1]. When that happens, we could rewrite the subject line of the last commit message in the series to contain r=<r+ granter>.

We could:
1. monitor for a patch to be r+'d
2. when it's r+'d, check that the r+'er is authorized to land
3. if authorized, rebase the patch and set r= in the commit subject
4. if not authorized, set r+ back to r? and comment that the r+'er was not authorized

As far as obsoleting patches, the github pr api exposes the information we'd need to expire patches and trigger new ones to be created.

John



[1] The only case we would not need to rewrite history is the exceedingly unlikely case where the patch can be fast-forward merged into master

[Fabrice Desre]

On 08/14/2013 02:07 PM, John Ford wrote:
> If we get rid of merge commits entirely, we'll almost certainly have to alter the commit metadata as it's rebased onto master branch [1]. When that happens, we could rewrite the subject line of the last commit message in the series to contain r=<r+ granter>.
>
> We could:
> 1. monitor for a patch to be r+'d
> 2. when it's r+'d, check that the r+'er is authorized to land
> 3. if authorized, rebase the patch and set r= in the commit subject
> 4. if not authorized, set r+ back to r? and comment that the r+'er was not authorized

r+ is not the flag to track on bugzilla to trigger pushes, but
checkin-needed. Indeed it's very common (at least in gecko-land) to give
r+ with comments to address.

Fabrice
--
Fabrice Desré
b2g team
Mozilla Corporation