754132 - Use treestatus for mercurial closure hooks

Status: RESOLVED FIXED

(Developer Services :: Mercurial: hg.mozilla.org, defect, P4)

Description

[Chris AtLee [:catlee]]

Attachment: update treeclosure.py to look at treestatus instead of tinderbox

We have treestatus up \o/

Now we need the treeclosure.py hg hook to look at it instead of tinderbox. This patch is a first attempt at that.

Ted, for some reason I think you were the initial owner of this, so you get tagged for a f?

Corey, how is this hook enabled for the various repos? This script currently prints a warning, but fails open, for repos that don't have a corresponding entry in treestatus.

Comment 1

[(not currently active) Ted Mielczarek]

I think johnath wrote the first version of this hook, but I'm the module owner of this stuff, so I can certainly review it.

The hook is enabled on a per-repo basis, AIUI.

Comment 2

[Ben Kero [:bkero]]

Currently the hook is enabled for individual repositories in $repo/.hg/hgrc under the [hooks] section with a line that looks like:

pretxnchangegroup.a_treeclosure = python:mozhghooks.treeclosure.hook

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 622984 [details] [diff] [review]
update treeclosure.py to look at treestatus instead of tinderbox

Review of attachment 622984 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks great, just a few comments.

Also, there are unit tests for this hook, so you'll need to adjust them:
http://hg.mozilla.org/hgcustom/hghooks/file/61b22f7461bd/runtests.py#l207

(they mock out the HTTP requests, so it should be easy enough to test what you need to test)

::: mozhghooks/treeclosure.py
@@ -45,5 @@
> -    'try'            : 'http://tinderbox.mozilla.org/Try/status.html',
> -    'try-comm-central': 'http://tinderbox.mozilla.org/Thunderbird-Try/status.html',
> -    'services-central': 'http://tinderbox.mozilla.org/Services-Central/status.html',
> -    'shadow-central' : 'http://tinderbox.mozilla.org/Shadow-Central/status.html',
> -}

<3

@@ +25,5 @@
>  
> +treestatus_base_url = "https://treestatus.mozilla.org"
> +
> +# For testing
> +# treestatus_base_url = "https://treestatus-dev.allizom.org"

I'd just leave this comment out, unless you want to add support for overriding the URL with an environment variable, in which case you could add that and document this as the example server to use...

@@ +31,3 @@
>  def hook(ui, repo, **kwargs):
> +    name = os.path.basename(repo.root)
> +    url = "%s/%s?format=json" % (treestatus_base_url, name)

The expectation is that treestatus will only use the repo name itself, and not the path? (like integration/mozilla-inbound)

@@ +54,5 @@
> +
> +    except (ValueError, IOError), (err):
> +        # fail open. no sense making hg unavailable
> +        # if treestatus is down
> +        print "Error: %s" % err, url

If we turn treestatus into a production service, we should consider making this fail closed. treestatus seems simple enough that we could make that work.

Comment 4

[Chris AtLee [:catlee]]

> @@ +31,3 @@
> >  def hook(ui, repo, **kwargs):
> > +    name = os.path.basename(repo.root)
> > +    url = "%s/%s?format=json" % (treestatus_base_url, name)
> 
> The expectation is that treestatus will only use the repo name itself, and
> not the path? (like integration/mozilla-inbound)

We could do that - treestatus supports trees with '/' in their names (e.g.
https://treestatus-dev.allizom.org/integration/fx-team). I'm not sure how I'd
get the hg hook to decide which part of the path was part of the tree/branch
name, and which part was irrelevant directories on the system.

> 
> @@ +54,5 @@
> > +
> > +    except (ValueError, IOError), (err):
> > +        # fail open. no sense making hg unavailable
> > +        # if treestatus is down
> > +        print "Error: %s" % err, url
> 
> If we turn treestatus into a production service, we should consider making
> this fail closed. treestatus seems simple enough that we could make that
> work.

Let's deploy with fail open first and see how it handles things.

Comment 5

[(not currently active) Ted Mielczarek]

There's no way (AFAIK) to get the relevant portion of the path from the hook, and we don't have any overlap in repo names that I know of, so let's just go with it. I was just sanity-checking.

Comment 6

[Chris AtLee [:catlee]]

Attachment: update treeclosure.py to look at treestatus instead of tinderbox

addressed your feedback comments:
* fixed up tests \o/
* removed commented out staging instance

Comment 7

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

whats left to do here?

Comment 8

[(not currently active) Ted Mielczarek]

This needs a bug to get deployed on hg.mo, and bug 756027 needs to get fixed to make TBPL use treestatus as well.

Comment 9

[Ed Morley [:emorley]]

We also need to first:
* Populate the tree names & default status messages for trees that will be using this hook: filed bug 758882.
* Give active sheriffs permissions on treestatus.mozilla.org: filed bug 758886.

And optionally:
* Make sure restoring previous state works: https://github.com/catlee/treestatus/issues/6

Comment 10

[Mark Banner (:standard8)]

Bug 759170 has the equivalent patch for the Thunderbird trees, it is all ready for landing. I'd appreciate it if we could arrange for both of these to be rolled out at the same time, rather than requiring two separate updates of hg.mozilla.org.

Comment 11

[Mark Banner (:standard8)]

Checked in:

http://hg.mozilla.org/hgcustom/hghooks/rev/623817e95249

We're aiming to file the bug to get it pushed out on Monday, in-sync with the tbpl changes.

Comment 12

[Ed Morley [:emorley]]

I have a draft "simultaneously push tbpl and hghooks to prod" bug ready, but holding off filing until the caching has been tweaked:
https://github.com/catlee/treestatus/issues/18

Comment 13

[Ed Morley [:emorley]]

In production :-)

See bug 782239 for testing/where I've notified now that we've switched.