Closed Bug 589914 Opened 11 years ago Closed 11 years ago

tag step should not fail on APPROVAL REQUIRED


(Release Engineering :: General, defect, P3)



(Not tracked)



(Reporter: aki, Assigned: aki)



(Whiteboard: [automation][simple])


(2 files, 3 obsolete files)

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.
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.
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.
Duplicate of this bug: 609717
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.
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
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.
 That would be: "allowing sloppy tinderbox status code" != fix.
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.
Whiteboard: [automation][simple]
Attached patch OMG BIKESHED (obsolete) — Splinter Review
same patch for 08x.

Alternately, let's bikeshed on this longer.
Attachment #489030 - Flags: review?(nrthomas)
Comment on attachment 489030 [details] [diff] [review]

Please update the comment ahead of the first change too.
Attachment #489030 - Flags: review?(nrthomas) → review+
if this is not planned to land on 0.7 branch, can it please?
Attached patch 08x patch (obsolete) — Splinter Review
Adding requested comment change. Carrying forward r+. This is for 08x (default branch).
Attachment #489091 - Flags: review+
Attached patch 07x patch (obsolete) — Splinter Review
Same, but for 07x (same patch, different line numbers)
Assignee: nobody → aki
Attachment #489030 - Attachment is obsolete: true
Attachment #489092 - Flags: review+
Flags: needs-reconfig?
Attached patch comment fix, 07xSplinter Review
Attachment #489092 - Attachment is obsolete: true
Attachment #489093 - Flags: review+
Attached patch comment fix, 08xSplinter Review
Attachment #489091 - Attachment is obsolete: true
Attachment #489094 - Flags: review+
Bug 508896 will need this work, too. I'll update my patches to include it.
Closed: 11 years ago
Resolution: --- → FIXED
Flags: needs-reconfig?
Product: → Release Engineering
You need to log in before you can comment on or make changes to this bug.