656732 - Figure out what the builds in the top right are good for and implement that

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Steve Fink [:sfink] [:s:]]

I'm not sure how to get any more specific. Perhaps we can rename the bug after discussion.

This bug was spawned from discussion in bug 655880.

philor:

I personally think of the boxes in the top right as a sort of unreliable look at how the tip of the tree is currently doing, and as having very nearly nothing to do with starring, ...

arpad:

We should probably discuss what to do with the boxes on the upper right. If they don’t provide any real value we should fix them.
File a followup for discussion please.

Comment 1

[Steve Fink [:sfink] [:s:]]

I find the boxes useful for seeing the general state of the tree (and infrastructure) at a glance. Usually that's for "is now a good time to push", but not always.

I occasionally use them for jumping through starrable builds one at a time. I could press 'u' to restrict to just starrable stuff, but it disturbs me for my dashboard to be mutating; I want to see them in context. I could use the j/k (or n/p) hotkeys, but my brain has "mouse context" and "keyboard context" and I don't always want to switch.

The push-independence is key.

Comment 2

[Arpad Borsos [:Swatinem]]

Little history:
This feature started out as a table on the lower right that showed the same thing that the tinderbox headings do but grouped by os and type instead of a flat array. So similar to tinderbox, it showed the most recently finished run for a particular machine.
Then I restructured the UI, moved them to the top and just showed boxes that were not green, ordered by color (red left, orange right) instead of by os/type.
Still, it shows the most recent finished run for a particular machine.

So what do we really want instead of that?
- Maybe something like bug 568819? “Tell me if I can push“?
- A list of ALL oranges would be overkill, it already overflows into the tree status on Jägermonkey that has a lot of orange
- A list of ALL UNSTARRED failures? That may help with starring when you don’t want to bother looking at the pushes list at all, but is that helpful? There is already the only-unstarred filter for that.
- something completely different?
- drop that feature completely? (that would surely clean up a lot of code ;-)

My guess is that the preference would be 1 or 3.
Also the question is how should it look like from the UI perspective?
Something like “tip is healthy”/“tip has 2 starred oranges”?
Maybe something like “last healthy revision is 0123456789abc; 2 csets behind tip”

Technical question: When can we consider tip to have enough finished results to be considered a healthy tree? How to treat intermittent orange? (There was a blog about automatic try-forwarding some time ago that mentioned to automatically retrigger on failure so the second run would not be orange; need to find the url for that)
Also: “starred” does not mean it is intermittent, it can mean anything really.

Comment 3

[Arpad Borsos [:Swatinem]]

http://ehsanakhgari.org/blog/2011-04-28/assisted-landing-patches-mozilla-central thats what I was looking for

Comment 4

[Mark Banner (:standard8)]

I generally use them to get an idea of the state of the tree and for starring. They are especially useful for starring, because finding the latest orange in amongst all the items shown is a PITA.

Whilst there are shortcut keys, they aren't actually advertised anywhere, so the only way you're going to find them is by randomly pressing buttons or digging through the source code (oops, that's probably a separate bug that should be filed).

The biggest fault with the icons in the top-right is that they don't actually reflect the state of builds for the latest changeset. If a build from an old changeset comes in late then it'll get shown, even though there are later builds for that builder that are green. IMO this is the bug that needs fixing.

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to comment #4)
> Whilst there are shortcut keys, they aren't actually advertised anywhere, so
> the only way you're going to find them is by randomly pressing buttons or
> digging through the source code (oops, that's probably a separate bug that
> should be filed).

Thanks for reminding me. I've been meaning to file bug 656770.

Comment 6

[Phil Ringnalda (:philor)]

I've pretty much stopped looking at them entirely, since I live in onlystarred and star nearly everything, so that colors my opinion, but one thing I think I might like from them is turning them into a menu with the text "Failed: 30 Unstarred: 5" that opens a (fairly narrow) popup panel with the colored boxes. Probably be hard to make it work with a too-small window when a failure is Linux Talos on the tip push, and thus at the top right corner of the screen, though.

Comment 7

[Ed Morley [:emorley]]

Now that the shortcut keys are shown from the help menu & the starred/unstarred toggle can be activated using the mouse, I feel that there is even less use for these boxes now. Taking them out will also stop bug 663864, which quite frankly drives me nuts when there's an infra issue, or if trying to keep TBPL in a narrow window.

As such, I'd like to just remove them unless there are major objections from anyone - leaving only the "X jobs are failing" string.

Comment 8

[Ed Morley [:emorley]]

Attachment: Patch v1

Removes the failing run squares, but keeps the "N Jobs is/are failing" string. Can't see anything other than the few CSS rules that is now unused and can be taken out, given that all the machineResult stuff is shared with the main pushes panel etc.

Comment 9

[Phil Ringnalda (:philor)]

Comment on attachment 626057 [details] [diff] [review]
Patch v1

Swatinem's a much better choice for reviewer, since I've never been within a mile of any part of this :)

Comment 10

[Arpad Borsos [:Swatinem]]

Comment on attachment 626057 [details] [diff] [review]
Patch v1

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

This is fine for now. I guess we could do a lot more cleanups. Especially if we remove the notion of "current" jobs (that is, the most recent job for a particular machine).

::: css/style.css
@@ -498,5 @@
> -  box-shadow: inset 0 3px 10px rgba(0, 0, 0, 0.1);
> -}
> -
> -#status a.hasNote {
> -  background-image: url(../images/starredbox.png);

Please check is this is used anywhere else and remove the file if it isnt.

Comment 11

[Ed Morley [:emorley]]

Attachment: Patch v2

(In reply to Arpad Borsos (Swatinem) from comment #10)
> This is fine for now. I guess we could do a lot more cleanups. Especially if
> we remove the notion of "current" jobs (that is, the most recent job for a
> particular machine).

The "N Jobs is/are failing" string + the tab title still use the notion of current jobs (I've only just started looking at the UI code, so I could be missing the obvious), so we'd need to decide that we're happy just showing total unstarred in those locations before we can do any further cleanup aiui. Personally I'm not overly attached to the tab title count showing only the latest unstarred jobs/runs, so would be happy to make this optimisation. Either way suspect worth breaking out to another bug :-)


> > -#status a.hasNote {
> > -  background-image: url(../images/starredbox.png);
> 
> Please check is this is used anywhere else and remove the file if it isnt.

Ah yes, sorry missed this. The file isn't used elsewhere, removed in this updated patch.

Comment 12

[Arpad Borsos [:Swatinem]]

(In reply to Ed Morley [:edmorley] from comment #11)
> The "N Jobs is/are failing" string + the tab title still use the notion of
> current jobs (I've only just started looking at the UI code, so I could be
> missing the obvious), so we'd need to decide that we're happy just showing
> total unstarred in those locations before we can do any further cleanup
> aiui. Personally I'm not overly attached to the tab title count showing only
> the latest unstarred jobs/runs, so would be happy to make this optimisation.
> Either way suspect worth breaking out to another bug :-)

Thats exactly what I mean: does the distinction between total or currently failing jobs provide any value? Lets discuss that in a new bug.

Comment 13

[Ed Morley [:emorley]]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/276c25fd49e8


(In reply to Arpad Borsos (Swatinem) from comment #12)
> Thats exactly what I mean: does the distinction between total or currently
> failing jobs provide any value? Lets discuss that in a new bug.

Filed bug 757879 :-)