Closed Bug 553047 Opened 14 years ago Closed 14 years ago

add_patcher_config step of MajorUpdateFactory should be set-up to never fail under normal circumstances


(Release Engineering :: General, defect, P2)



(Not tracked)



(Reporter: bhearsum, Assigned: rail)



(Whiteboard: [automation][updates])


(1 file, 1 obsolete file)

      No description provided.
Assignee: nobody → rail
Attached patch Proposed fix (obsolete) — Splinter Review
Attachment #433316 - Flags: review?(bhearsum)
Attachment #433316 - Flags: review?(bhearsum) → review+
Ben, could you explain why this is a needed ?
In general, the patcher config is already going to exist in CVS. Only for the first MU will we have to land it. I guess the better thing to do here would be to only add it if it doesn't already exist.
Ah, I'd forgotten we use patcher-config-creator every time for major updates, rather than patcher-config-creator once then patcher-config-bump. Mostly I'm looking to avoid not committing a config and then not having history.

So yeah, the cvs add step could move before the bump and be conditional on the non-existence of the config file. Might need to use touch to create an empty file first.
Attachment #433316 - Flags: checked-in?
Blocks: 552994
Comment on attachment 433316 [details] [diff] [review]
Proposed fix

Based on the recent comments I think we'll end up taking a better fix for this
Attachment #433316 - Attachment is obsolete: true
Attachment #433316 - Flags: checked-in?
No longer blocks: 552994
Summary: add_patcher_config step of MajorUpdateFactory should set flunkOnFailure=False → add_patcher_config step of MajorUpdateFactory should be set-up to never fail under normal circumstances
Priority: -- → P2
Attached patch Proposed patchSplinter Review
Attachment #438347 - Flags: review?(bhearsum)
Comment on attachment 438347 [details] [diff] [review]
Proposed patch

This doesn't look like the right order to do things. This should happen after is run, which eliminates the need to touch the file.
Attachment #438347 - Flags: review?(bhearsum) → review-
As we talked on IRC, using bash is the only way to check if file exists on slave side.

I tested this patch in staging using the following scenario.

* Set up a cvs mirror with patcher-configs. Removed all major-update cfg files.
* Set up staging to use 1.9.1 (3.5.9) with MU configured
* First forced run of the major_update builder generates a MU cfg file from scratch and the diff between the production CVS versoin and generated one is sane:
* Second forced run of the major_update builder went fine. add_patcher_config step does nothing, the output of diff_patcher_config is zero, cvs commit went as usual.
Attachment #438347 - Flags: review- → review?(bhearsum)
Comment on attachment 438347 [details] [diff] [review]
Proposed patch

We can land this after 3.6.4.
Attachment #438347 - Flags: review?(bhearsum) → review+
Comment on attachment 438347 [details] [diff] [review]
Proposed patch

changeset:   721:c61313a0054d
Attachment #438347 - Flags: checked-in+
pm01 and pm02 all reconfiged
Whiteboard: [automation][updates]
this is done, i think.
Closed: 14 years ago
Resolution: --- → FIXED
I did notice that it was fixed this week.

Instead of:
cvs add patcher-configs/moz191-branch-major-update-patcher2.cfg
it run this and it did what I wanted:
bash -c if [ ! -f patcher-configs/moz191-branch-major-update-patcher2.cfg ]; then touch patcher-configs/moz191-branch-major-update-patcher2.cfg && cvs add patcher-configs/moz191-branch-major-update-patcher2.cfg; fi

Thanks Rail!
Product: → Release Engineering
You need to log in before you can comment on or make changes to this bug.