Matt Brubeck (:mbrubeck) Assignee
	Description • 12 years ago

Cancelling non-clobber builds can leave the objdir in a broken state and break later builds.  This hides the cancel build button from the TBPL interface except on Try trees.  It'll still be there in self-serve (bug 664858) but at least it's more hidden.

The first patch just adds some info about Try to Config.js so we have a central location for it.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 1 • 12 years ago

This is just to improve readability, since we repeat this so often.

	Phil Ringnalda (:philor)
	Comment 3 • 12 years ago

Just remembered one of the arguments against doing this that I've made during the various times I've had to take that side - I just pushed a cset to release which needed a clobber, and forgot to clobber, so I killed the builds, set the clobber, retriggered. Admittedly, since releng is essentially wontfixing bug 664858 I'll always be able to kill builds through self-serve, but tbpl is much handier.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 4 • 12 years ago

We could put in a confirm() prompt instead...

	Ed Morley [:emorley]
	Comment 5 • 12 years ago

I like the idea of the confirm prompt, given that it will educate people (so they don't just go and hunt down the cancel button in the buildapi directly) & also give the sheriffs flexibility :-)

	Matt Brubeck (:mbrubeck) Assignee
	Comment 6 • 12 years ago

Ideally we would also show a link to open the correct clobberer page, but it would require more changes to the _showMessage code to allow links in messages, and I'm lazy.  Anyway, my goal is to make this message scary enough that people will click "cancel" and won't need to clobber.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 7 • 12 years ago

Not needed to fix this bug, but the previous comment gave me the idea.

Also, I'm starting to think that treeInfo.isTry should be renamed to something more specific like treeInfo.autoClobber, since that's what we're actually checking for when we use it.  Thoughts?

	Ed Morley [:emorley]
	Comment 8 • 12 years ago

Comment on attachment 633171 [details] [diff] [review]
3/3: Ask for confirmation before cancelling builds

I've tested these locally & all works well apart from one thing: I can still cancel pending tests on non-try trees (which can end up cancelling the wrong run).

Could you alter UserInterface_allowCancel such that it returns false for pending non-try please? :-)

	Ed Morley [:emorley]
	Comment 9 • 12 years ago

Is it just pending tests that can be affected by comment 8, or builds as well? I'm not sure.

	Matt Brubeck (:mbrubeck) Assignee
	Comment 10 • 12 years ago

(In reply to Ed Morley [:edmorley] from comment #8)
> Could you alter UserInterface_allowCancel such that it returns false for
> pending non-try please? :-)

Done.

(In reply to Ed Morley [:edmorley] from comment #9)
> Is it just pending tests that can be affected by comment 8, or builds as
> well? I'm not sure.

Yes, cancelling a pending test can end up cancelling the wrong job when coalescing happens, as I have repeatedly learned and then forgotten...

	Matt Brubeck (:mbrubeck) Assignee
	Comment 11 • 12 years ago

Oops, I introduced a minor logic bug.  The selfServeAPIBaseURL check should come first.

	Ed Morley [:emorley]
	Comment 12 • 12 years ago

Comment on attachment 634973 [details] [diff] [review]
3/3: Ask for confirmation before cancelling builds (v3)

r=me with string changes discussed on irc

	Matt Brubeck (:mbrubeck) Assignee
	Comment 13 • 12 years ago

https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/36cb8c54b86b
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/7c585b6fea33
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/ba64e23a383c
https://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/ae79837b4ff1