759170 - Use treestatus for mercurial closure hooks (Thunderbird comm-central version)

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: The fix

+++ This bug was initially created as a clone of Bug #754132 +++

Similar to bug 754132, I'd like to get the closure hooks switched to treestatus.mozilla.org as soon as possible - I think the main thing that we're waiting on is the tbpl update.

I'm not against SeaMonkey & Calendar switching (and in fact I'd like it), but someone would need to do some js work to display the status on tinderbox.

This patch does the necessary changes for Thunderbird, I've gone for copying the checkTreeState function as that seems the best way of keeping things clean. I've also updated the old trees that aren't used any more to just fail-open. Tests have also been updated.

Comment 1

[Justin Wood (:Callek)]

Comment on attachment 627764 [details] [diff] [review]
The fix

Yes, SeaMonkey would want JS/Magic for tbpl and tinderbox to import a treestatus message if we switched, but that said I'm happier with the app as a single-source-of-info than I am with the Tinderbox Status Message [and I do have access to the app] if you want(ed) to switch us here/in a new patch/bug. I won't block this bug on switching SeaMonkey though.

>@@ -163,12 +199,9 @@ def hook(ui, repo, node, **kwargs):
>         status = 0
> 
>         if apps['thunderbird']:
>-            if not thunderbirdTrees.has_key(repoName):
>-                print "Unrecognized tree!  I don't know how to check closed status for %s and Thunderbird... allowing push, but you should report this!" % (repoName)
>-            else:
>-                status = checkTreeState(repo, repoName, 'Thunderbird', thunderbirdTrees[repoName])
>-                if status == 1:
>-                    return 1
>+            status = checkJsonTreeState(repo, repoName, 'thunderbird')
>+            if status == 1:
>+                return 1
> 
>         if apps['seamonkey']:
>             if not seamonkeyTrees.has_key(repoName):

I'm tempted to say to rip out this to a wrapped function so that when SeaMonkey/others switch we don't have repeat code, or maybe even do it like:
 for app in apps:
   status = checkJsonTreeState(repo,repoName,app)
   ...

To gracefully allow new apps, without necessarily needing to hardcode app in this part anymore ;-)

>diff --git a/runtests.py b/runtests.py
>--- a/runtests.py
>+++ b/runtests.py
>@@ -358,8 +358,13 @@ class TestTreeCommCentralClosureHook(Clo
>     # If this tests attempts to pull something that isn't treeName, then the
>     # re-director should fail for us. Hence we know that the hook is only
>     # pulling the predefined tree and nothing else.
>-    self.redirect("http://tinderbox.mozilla.org/" + treeName + "/status.html",
>-                         '<span id="tree-status">OPEN</span><span id="extended-status">')
>+    if "Thunderbird" in treeName:
>+        self.redirect("https://treestatus.mozilla.org/comm-central-" + treeName.lower() + "?format=json",
>+                      '{"status": "open", "reason": null}')
>+    else:
>+        self.redirect("http://tinderbox.mozilla.org/" + treeName + "/status.html",
>+                      '<span id="tree-status">OPEN</span><span id="extended-status">')
>+

I feel that hardcoding TB as a branch point two test two very different sets of functionality here is a bad idea, and we'd be better off with either a seperate test class, or at least different helper method defines.

>   def actualTestCCClosed(self, treeName, fileInfo):

Also in here.

That said the above are all nits, and at your discretion as to amount of work/degree of change/etc. I'm willing to re-review, but if its "too much work for too little gain" I'm willing to hear that and let you proceed anyway.

Comment 2

[Mark Banner (:standard8)]

(In reply to Justin Wood (:Callek) from comment #1)
> I'm tempted to say to rip out this to a wrapped function so that when
> SeaMonkey/others switch we don't have repeat code, or maybe even do it like:
>  for app in apps:
>    status = checkJsonTreeState(repo,repoName,app)
>    ...
> 
Slightly adapted so that it works fully, I've done this change.

> I feel that hardcoding TB as a branch point two test two very different sets
> of functionality here is a bad idea, and we'd be better off with either a
> seperate test class, or at least different helper method defines.

The only difference is the url and what it returns. I thought about passing a parameter in, but then we'd just take it out again once everyone is across, and with it as it is will bring you directly to where you need to change.

> >   def actualTestCCClosed(self, treeName, fileInfo):
> 
> Also in here.

This is just the same as what it was. I don't think there's huge benefit in testing the messages for each app concerned at this stage.

Comment 3

[Mark Banner (:standard8)]

Attachment: The fix v2

Updated patch, ready for landing.

Comment 4

[Mark Banner (:standard8)]

Checked in:

http://hg.mozilla.org/hgcustom/hghooks/rev/8db0fe2a5231

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

Comment 5

[Ed Morley [:emorley]]

In production :-)

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