Blocking commits when the tree is closed

0 views
Skip to first unread message

Johnathan Nightingale

unread,
Nov 14, 2008, 4:11:17 PM11/14/08
to dev-pl...@lists.mozilla.org
Hi folks,

With lots of help from Ted, I put together a hook for hg that will
prevent commits when the tree is closed. It's not activated yet, and
you can override it by adding "CLOSED TREE" to your push comment. I'd
like to activate it, though, since it would prevent accidental pushes
from people who aren't watching tinderbox as closely as they should,
and doesn't cost us much.

Particulars, in case you're interested:
- It fails open - so we only block commits when we can
affirmatively say the tree is closed
- It uses the statically generated tinderbox pages, not the
showbuilds.cgi ones - download speed hasn't been an annoyance in testing
- I'm only talking about turning it on for mozilla-central, but if
other repos wanted it, it should be adaptable
- Bug tracking actual activation on mozilla-central - Bug 464974
- The hook itself: http://hg.mozilla.org/users/bsmedberg_mozilla.com/hghooks/file/c5ee26216d0c/mozhghooks/treeclosure.py

Do people agree this is desirable as described? It's cheap to toggle
off again if we find a problem after the fact (just remove a line from
the repo's config). In the absence of objections, I say we do it and
see if it adds value or not.

Cheers,

Johnathan

---
Johnathan Nightingale
Human Shield
joh...@mozilla.com

Mike Beltzner

unread,
Nov 14, 2008, 4:15:31 PM11/14/08
to Johnathan Nightingale, dev-pl...@lists.mozilla.org
On 14-Nov-08, at 4:11 PM, Johnathan Nightingale wrote:

> With lots of help from Ted, I put together a hook for hg that will
> prevent commits when the tree is closed. It's not activated yet,
> and you can override it by adding "CLOSED TREE" to your push
> comment. I'd like to activate it, though, since it would prevent
> accidental pushes from people who aren't watching tinderbox as
> closely as they should, and doesn't cost us much.

Clever solution, that. Not only is it hard to do accidentally, it's
also easily searchable. I take it that the script generates an error
message that allows the errant committer to figure out why their
commit failed?

> Do people agree this is desirable as described? It's cheap to
> toggle off again if we find a problem after the fact (just remove a
> line from the repo's config). In the absence of objections, I say
> we do it and see if it adds value or not.

I think it's sensible. It's nice that we can usually police these
things by relying on the good graces and habits of our committers, but
it's nicer still that if someone checks Tinderbox, gets up to get a
coffee, comes back and hits ENTER on their commit and hadn't noticed
that the tree had been closed in the interim, they get a safety net. :)

One further question: what qualifies as a "tree closed" event? A
certain block of text on Tinderbox Status? A non-green tree?

cheers,
mike

Michael Connor

unread,
Nov 14, 2008, 4:17:32 PM11/14/08
to Johnathan Nightingale, dev-pl...@lists.mozilla.org
I think actually closing the tree instead of pretending makes a ton of
sense. Especially as there is a way to explicitly override the
closure, there should not be an issue here. For bonus points, can we
make the master switch take some strings so we can have a same
closures log?

-- Mike

On 14-Nov-08, at 1:11 PM, Johnathan Nightingale <joh...@mozilla.com>
wrote:

> Hi folks,


>
> With lots of help from Ted, I put together a hook for hg that will
> prevent commits when the tree is closed. It's not activated yet,
> and you can override it by adding "CLOSED TREE" to your push
> comment. I'd like to activate it, though, since it would prevent
> accidental pushes from people who aren't watching tinderbox as
> closely as they should, and doesn't cost us much.
>

> Particulars, in case you're interested:
> - It fails open - so we only block commits when we can
> affirmatively say the tree is closed
> - It uses the statically generated tinderbox pages, not the
> showbuilds.cgi ones - download speed hasn't been an annoyance in
> testing
> - I'm only talking about turning it on for mozilla-central, but if
> other repos wanted it, it should be adaptable
> - Bug tracking actual activation on mozilla-central - Bug 464974
> - The hook itself: http://hg.mozilla.org/users/bsmedberg_mozilla.com/hghooks/file/c5ee26216d0c/mozhghooks/treeclosure.py
>

> Do people agree this is desirable as described? It's cheap to
> toggle off again if we find a problem after the fact (just remove a
> line from the repo's config). In the absence of objections, I say
> we do it and see if it adds value or not.
>

> Cheers,
>
> Johnathan
>
> ---
> Johnathan Nightingale
> Human Shield
> joh...@mozilla.com
>
>
>

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

Mike Beltzner

unread,
Nov 14, 2008, 4:27:53 PM11/14/08
to Mike Beltzner, dev-pl...@lists.mozilla.org, Johnathan Nightingale
On 14-Nov-08, at 4:15 PM, Mike Beltzner wrote:

> Clever solution, that. Not only is it hard to do accidentally, it's
> also easily searchable. I take it that the script generates an error
> message that allows the errant committer to figure out why their
> commit failed?

[...]

> One further question: what qualifies as a "tree closed" event? A
> certain block of text on Tinderbox Status? A non-green tree?

Actually reading the script answered both of these questions for me -
oops. For those wondering, yes, friendly error messages are given, and
to close the tree, one must ensure the word CLOSED appears in a span
with the id "treestatus" on the Tinderbox page.

cheers,
mike

Shawn Wilsher

unread,
Nov 14, 2008, 5:06:00 PM11/14/08
to Johnathan Nightingale, dev-pl...@lists.mozilla.org
On 11/14/08 1:11 PM, Johnathan Nightingale wrote:
> With lots of help from Ted, I put together a hook for hg that will
> prevent commits when the tree is closed. It's not activated yet, and you
> can override it by adding "CLOSED TREE" to your push comment. I'd like
> to activate it, though, since it would prevent accidental pushes from
> people who aren't watching tinderbox as closely as they should, and
> doesn't cost us much.
Not that I'm trying to bikeshed on this, but I think something like
"a=sheriff" fits in better with our normal commit messages, and implies
that you better be checking with the sheriff before landing in a closed
tree.

Cheers,

Shawn

Johnathan Nightingale

unread,
Nov 14, 2008, 5:52:46 PM11/14/08
to Michael Connor, dev-pl...@lists.mozilla.org
No Master switch here - it uses the existing mechanism. Sheriffs mark
the tinderbox tree (since we have an admin structure for that and all)
and Hg scrapes the static page to check status.

Ted's original patch used a wiki to store the CLOSED/OPEN status.
That would also have the advantage of a wiki changelog - easy enough
to change back to that system, but it means getting sheriffs to change
how they close the tree, and porting over (or abandoning) the
sheriffpass to the wiki pages. On balance, I went this way because it
involved incremental win without asking anyone to change their
process. If we want to do the other thing, I say keep the forward
motion, file a follow-up bug. :)

Cheers,

J

On 14-Nov-08, at 4:17 PM, Michael Connor wrote:

> I think actually closing the tree instead of pretending makes a ton
> of sense. Especially as there is a way to explicitly override the
> closure, there should not be an issue here. For bonus points, can
> we make the master switch take some strings so we can have a same
> closures log?
>
> -- Mike
>

> On 14-Nov-08, at 1:11 PM, Johnathan Nightingale
> <joh...@mozilla.com> wrote:
>
>> Hi folks,


>>
>> With lots of help from Ted, I put together a hook for hg that will
>> prevent commits when the tree is closed. It's not activated yet,
>> and you can override it by adding "CLOSED TREE" to your push
>> comment. I'd like to activate it, though, since it would prevent
>> accidental pushes from people who aren't watching tinderbox as
>> closely as they should, and doesn't cost us much.
>>

Daniel Holbert

unread,
Nov 14, 2008, 6:17:29 PM11/14/08
to
On 11/14/2008 02:06 PM, Shawn Wilsher wrote:
>> can override it by adding "CLOSED TREE" to your push comment. I'd like
[snip]

> Not that I'm trying to bikeshed on this, but I think something like
> "a=sheriff" fits in better with our normal commit messages

FWIW, I disagree... "a=sheriff" seems messier & more confusing to me (or
at least, no better) than "CLOSED TREE", for a few reasons:

- It means we'd get commit messages with mutltiple a='s, like:
"Bug XXX: Change foobar. r+sr=roc a=beltzner a=sheriff"

- The overriding of "a=" suggests that sheriffs can grant approval
during beta freezes / etc, which is false.

> and implies
> that you better be checking with the sheriff before landing in a closed
> tree.

Anyone who's got checkin privs should already know this... And if they
don't, they're probably not the people we want using the Magic Words to
override this hook anyway. :)

Mark Banner

unread,
Nov 15, 2008, 12:07:32 PM11/15/08
to
On 14/11/08 23:17, Daniel Holbert wrote:
> On 11/14/2008 02:06 PM, Shawn Wilsher wrote:
>>> can override it by adding "CLOSED TREE" to your push comment. I'd like
> [snip]
>> Not that I'm trying to bikeshed on this, but I think something like
>> "a=sheriff" fits in better with our normal commit messages
>
> FWIW, I disagree... "a=sheriff" seems messier & more confusing to me (or
> at least, no better) than "CLOSED TREE", for a few reasons:
>
> - It means we'd get commit messages with mutltiple a='s, like:
> "Bug XXX: Change foobar. r+sr=roc a=beltzner a=sheriff"
>
> - The overriding of "a=" suggests that sheriffs can grant approval
> during beta freezes / etc, which is false.

Additionally not all trees have sheriffs as such. A generic solution
that works for everyone seems to be the best here. I'm sure most people
are capable of working the comment into a=xxx for checkin to CLOSED TREE.

Although as I type that, I'm thinking, should we also allow NPOTB to be
an "allow push" as well?

Standard8

Justin Dolske

unread,
Nov 15, 2008, 1:58:22 PM11/15/08
to
On 11/14/08 3:17 PM, Daniel Holbert wrote:
>> Not that I'm trying to bikeshed on this, but I think something like
>> "a=sheriff" fits in better with our normal commit messages
>
> FWIW, I disagree... "a=sheriff" seems messier & more confusing to me (or
> at least, no better) than "CLOSED TREE", for a few reasons:

I don't think the overloading of the a= message would be all that big a
deal. Sheriffs shuld know they can't grant it as a replacement for the
normal approval flags, and IIRC we had multiple a= flags at the end of
the FF3 cycle anyway.

However, I do think it would be a little confusing when there's no
official sheriff around (AWOL, or #developers). Who then is the sheriff
approving the landing?

It might be interesting to show who signed off on the landing, though...
closedtree=sheriff, closedtree=me, etc. Preferably in a light shade of
blue. :)

Justin

Justin Wood (Callek)

unread,
Nov 15, 2008, 7:16:35 PM11/15/08
to
Johnathan Nightingale wrote:
> No Master switch here - it uses the existing mechanism. Sheriffs mark
> the tinderbox tree (since we have an admin structure for that and all)
> and Hg scrapes the static page to check status.
>
> Ted's original patch used a wiki to store the CLOSED/OPEN status. That
> would also have the advantage of a wiki changelog - easy enough to
> change back to that system, but it means getting sheriffs to change how
> they close the tree, and porting over (or abandoning) the sheriffpass to
> the wiki pages. On balance, I went this way because it involved
> incremental win without asking anyone to change their process. If we
> want to do the other thing, I say keep the forward motion, file a
> follow-up bug. :)

For what its worth, I'm considering adjusting the hook to work for wiki
*and* tinderbox (legibly).

Such that there can be an OVERRIDE and a custom check function for any
tree, and in the case of mozilla-central and mobile it can continue to
be tinderbox (unless someone else really wants something better) and the
hook itself can be used elsewhere easily (ala: wiki pages)

--
~Justin Wood (Callek)

Reply all
Reply to author
Forward
0 new messages