589914 - tag step should not fail on APPROVAL REQUIRED

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[Aki Sasaki (not active)]

I hit this twice today: once because the tree was closed, but the previous APPROVAL REQUIRED status was commented out instead of removed; once because tinderbox.m.o took too long to load so sendchange-close_tree-let_tagging_finish-open_tree didn't work (step 2 took longer than step 3).

Ben says we shouldn't fix this here, because we should close the tree.
However, we don't fail on OPEN, so I think we should add an a=release or blocking-release or something that will allow the tagging step to push during times like this.

Comment 1

[Phil Ringnalda (:philor)]

Though it was suggested to me as a joke, I've been considering adding "a=CLOSED TREE" to my queue of whitespace patches that I use when we need to trigger builds when there's nobody around to trigger them without a patch, which would work fine as the minimum number of chars for tagging pushes to evade every hook, too.

Comment 2

[[e-mail] (:bhearsum)]

I don't like explicitly doing things to encourage lazy behvaiour, but I don't feel _that_ strongly about this. I'm not going to object any further to this.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

I hit this during a release once too, but I think it's important for us to have only one tree status at any given time and that we should remove the current status, put in our closed status (keep a snippet around somewhere for c&p) and then reverse those steps when done. It's cleaner and I'm sure a workaround for this will lead to problems somewhere else if we don't have to pay attention to how many tree statuses are in the tinderbox.

Comment 5

[Aki Sasaki (not active)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=609717#c6

Comment 6

[Armen [:armenzg]]

I would rather not have people hitting this because they didn't know.
I hit it because nothing was fixed and because it can be hit.
Keeping a snippet around to c&p is a behaviour to be taught to people who have not hit the problem before and they get to do a release. I would rather not hit the hook.

a=whatever WFM

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Ok, it sucks to hit this because you don't know.  But I don't see "allowing slopping tinderbox status code" as a fix. The hook is there for the protection of the tree and when we in Releng hit this I think it's a good reminder that we be clean about setting tinderbox page status.  However, as Aki pointed out this will be moot when we do not have push races and tagging does not require closing the tree.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

 That would be: "allowing sloppy tinderbox status code" != fix.

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

If the push race issue wasn't going away then the fix would be to make the hook actually parse the html rather than just do a regexp.

Comment 10

[Aki Sasaki (not active)]

Attachment: OMG BIKESHED

same patch for 08x.

Alternately, let's bikeshed on this longer.

Comment 11

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 489030 [details] [diff] [review]
OMG BIKESHED

Please update the comment ahead of the first change too.

Comment 12

[Justin Wood (:Callek)]

if this is not planned to land on 0.7 branch, can it please?

Comment 13

[Aki Sasaki (not active)]

Attachment: 08x patch

Adding requested comment change. Carrying forward r+. This is for 08x (default branch).

Comment 14

[Aki Sasaki (not active)]

Attachment: 07x patch

Same, but for 07x (same patch, different line numbers)

Comment 15

[Aki Sasaki (not active)]

Attachment: comment fix, 07x

Comment 16

[Aki Sasaki (not active)]

Attachment: comment fix, 08x

Comment 17

[[e-mail] (:bhearsum)]

Bug 508896 will need this work, too. I'll update my patches to include it.

Comment 18

[Aki Sasaki (not active)]

Comment on attachment 489094 [details] [diff] [review]
comment fix, 08x

http://hg.mozilla.org/build/buildbotcustom/rev/af943908626d

Comment 19

[Aki Sasaki (not active)]

Comment on attachment 489093 [details] [diff] [review]
comment fix, 07x

http://hg.mozilla.org/build/buildbotcustom/rev/f825695b7462