756027 - Switch to getting tree status from treestatus.mozilla.org

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Phil Ringnalda (:philor)]

Docs at https://treestatus.mozilla.org/help, telling us to get https://treestatus.mozilla.org/mozilla-central?format=json, and not quite exactly telling us how to uppercase "status" and glue on "reason" and then do something about the fallback crud in http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/0c3f709b0c37/js/UserInterface.js#l179.

Depending on bug 754132 isn't quite right - we depend on it, it depends on us, both need to happen at the same time, and at a time sooner than "once philor figures out how to do it." If you're the one who will be working on it, give me your BrowserID email, and I'll add you as a sheriff so you can test "reason"s.

Comment 1

[Ed Morley [:emorley]]

Are we going to need to validate the contents of the reason, or are we happy that treestatus authentication is sufficient to prevent arbitrary script injection into TBPL? If we do, we'll need obviously need to allow some form of markup in the reason for the links etc that we normally add.

Treestatus fixes most of the pain points of tinderbox, however we still:
* Have to manually add links to bugs.
* Have the boilerplate part of the tree message (eg "please see tree rules") muddled up with the reason.

So if we could linkify {{bug foo}} that would be awesome. 

For the second, do the rest of you think we should consider adjusting the treestatus schema slightly, so there's a "announcement" as well as a "reason" field? Or is that overkill? 

(I just thought it would make it harder for us to mess up the announcement each time when setting a bustage reason - and also would let us display the two separately in TBPL).

Comment 2

[Mark Banner (:standard8)]

(In reply to Ed Morley [:edmorley] from comment #1)
> Treestatus fixes most of the pain points of tinderbox, however we still:
> * Have to manually add links to bugs.
> * Have the boilerplate part of the tree message (eg "please see tree rules")
> muddled up with the reason.
> 
> So if we could linkify {{bug foo}} that would be awesome. 

These seem to be follow-up enhancements that we can do after getting rid of the pain of tinderbox. It is arguable that treestatus should actually do those.

Comment 3

[Chris AtLee [:catlee]]

Is anybody working on this?

Comment 4

[Chris AtLee [:catlee]]

anybody?

Comment 5

[Ed Morley [:emorley]]

It's on my list of things that would make my life as sheriff easier, so if it was still outstanding once I clear a few other things out of the way I was going to take a look; but I haven't started work on it yet.

Comment 6

[Mark Banner (:standard8)]

Attachment: Starter patch

Here's a starter patch. In theory, it should work, however, for some reason the ajax query isn't working.

If I use http://builddata.pub.build.mozilla.org/buildjson/builds-pending.js it succeeds fine, and would give some data (obviously incorrect for this!), if I use https://treestatus.mozilla.org/mozilla-central?format=json then it fails with a status text of "error".

"error" is the best I got.

So I gave up and hacked the error function to fake a json object and pass that through to the updateTreeStatus function.

The hack works and can show the tree status doing the correct thing. I've based it on the same rules as we're using for tinderbox - take the html without sanitizing it. Whether or not we want to keep it like that is another matter.


Due to my lack of knowledge about ajax, I'm not going to put myself forward to trying to fix that at the moment, although I wouldn't be surprised if its somehow a server side issue.

I'm also not intending to do any more tweaks to get this published, though I'd really, really like to see this get finished off and published, so we can have sane tree closures.

Comment 7

[Arpad Borsos [:Swatinem]]

(In reply to Mark Banner (:standard8) from comment #6)
> Due to my lack of knowledge about ajax, I'm not going to put myself forward
> to trying to fix that at the moment, although I wouldn't be surprised if its
> somehow a server side issue.

Yep, seems like a server issue, I don’t see "Access-Control-Allow-Origin: *" in the response headers.

Comment 8

[Arpad Borsos [:Swatinem]]

Comment on attachment 647716 [details] [diff] [review]
Starter patch

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

Looks good. I’ll file a bug to get the access control header in.

::: js/Controller.js
@@ +150,5 @@
>    refreshTreeStatus: function Controller_refreshTreeStatus() {
>      var self = this;
> +    var treestatusName = Config.treeInfo[self.treeName].buildbotBranch;
> +    if ("buildbotBranchExtra" in Config.treeInfo[self.treeName])
> +      treestatusName = Config.treeInfo[self.treeName].buildbotBranchExtra;

That should be +=

::: js/UserInterface.js
@@ +191,5 @@
> +            break;
> +          default:
> +            color = "purple";
> +            status = "Incorrect status, please contact sheriff";          
> +        }

I like a map like {"closed": "red", ... statusmap[data.status] || "purple" a lot better than this switch/case.

@@ +196,2 @@
>  
> +        $('#tree-status').html('<span style="color:' + color + '; font-weight: bold">' + status.toUpperCase() + '</span>. ' + data.reason);

We should probably use a css class instead of the inline style, that way we wouldn’t even need a status to color map.

Comment 9

[Mark Banner (:standard8)]

Attachment: The fix

Ok, that didn't take long to update, and we've now got at least the dev instance of tree status accepting all headers (bug being filed for production).

If you want to test it out, set treeStatusBaseURL to http://treestatus-dev.allizom.org/ - there's the three statuses already set up.

Comment 10

[Arpad Borsos [:Swatinem]]

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

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

Thanks

Comment 11

[Ed Morley [:emorley]]

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

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

::: css/style.css
@@ +864,5 @@
> +}
> +
> +.open {
> +  color: green;
> +}

Affects open TBPL menus too, needs to be #tree-status .open

Comment 12

[Mark Banner (:standard8)]

I've pushed this to the hg repository:

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/6560852179e4

Please can folks give this a test.

We'll need to roll it out in association with the hook changes in the dependent bugs, I suggest we look to IT to do that early next week. Ed also suggested we should make sure folks are around to test the hooks out so that we confirm that they still control the repositories appropriately.

Comment 13

[Ed Morley [:emorley]]

In production :-)

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