Closed Bug 1446160 Opened 6 years ago Closed 6 years ago

update verify shouldn't be silent when diffs are found

Categories

(Release Engineering :: Release Automation: Other, enhancement, P1)

Product:
Release Engineering
Component:
Release Automation: Other
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: bhearsum, Assigned: bhearsum)

References

Details

Attachments

(2 files)

exit with non-zero code when non-binary diffs are detected & fix errors
11.79 KB, patch
sfraser
: review+
bhearsum
: checked-in+
Details | Diff | Splinter Review
update verify shouldn't be silent when diffs are found
45 bytes, text/x-phabricator-request
mozilla
: review+
Details | Review 
The primary purpose of update verify is to detect differences between the MAR and installer of a yet-to-be-shipped release. It does this by applying the MAR to older versions, and diffing it against the unpacked installer.

In the in-tree world (and probably prior to it), we stopped making any noise when diffs are detected. This is largely because there are some cases where diffs are expected and OK. For example, update-settings.ini and channel-prefs.js will aways be different when we apply an RC MAR to a Beta.

At a bare minimum, I think we should failing update verify jobs when diffs are found - this will encourage (hopefully force) people to look at the logs. Doing this means that we should avoid having jobs downstream of them - otherwise we could end up in a situation where we have "failures" that don't block a release.

Other things that might help:
* Maintaining a whitelist of known OK diffs, and exempting those from causing failures. This might be tricky because parsing "diff" output in a reliable way is tricky.
* Attaching a separate artifact to update verify tasks with just the relevant output (eg: the diff output, ideally deduplicated if multiple tests fail with the same output).
Comment on attachment 8960735 [details] [diff] [review]
exit with non-zero code when non-binary diffs are detected & fix errors

Simon, I know you're not as familiar with this code as others, but you seem to be our resident bash expert these days, so I'm hoping you can take a look at it.

This patch is doing a few things:
* Fixing the errors from bug 1446152 & bug 1438906
* Generating a summary of all diffs found during update testing
* Exiting with a non-zero code if _any_ diffs are found (not just binary ones), which will cause the update verify tasks to be set to "failure" state.

You can see logs from a recent staging release on https://tools.taskcluster.net/groups/GuShO7cFRQmKFBjl96_ERg. If you need any background or help understanding update verify just let me know.
Attachment #8960735 - Flags: review? → review?(sfraser)
Comment on attachment 8960742 [details]
update verify shouldn't be silent when diffs are found

Aki Sasaki [:aki] has approved the revision.

https://phabricator.services.mozilla.com/D780
Attachment #8960742 - Flags: review+
Comment on attachment 8960735 [details] [diff] [review]
exit with non-zero code when non-binary diffs are detected & fix errors


>-                unpack_dir=`ls -1`
>+                unpack_dir=$(ls -1)
>+                unpack_dir=$(ls -d "${unpack_dir}")
>                 mv "${unpack_dir}"/*.app .
>-                rm -rf $unpack_dir
>-                appdir=`ls -1`
>+                rm -rf "${unpack_dir}"
>+                appdir=$(ls -1)
>+                appdir=$(ls -d *.app)

nit: Are unpack_dir and appdir meant to be set twice? 

Other than that, looks good.
Attachment #8960735 - Flags: review?(sfraser) → review+
(In reply to Simon Fraser [:sfraser] ⌚️GMT from comment #5)
> Comment on attachment 8960735 [details] [diff] [review]
> exit with non-zero code when non-binary diffs are detected & fix errors
> 
> 
> >-                unpack_dir=`ls -1`
> >+                unpack_dir=$(ls -1)
> >+                unpack_dir=$(ls -d "${unpack_dir}")
> >                 mv "${unpack_dir}"/*.app .
> >-                rm -rf $unpack_dir
> >-                appdir=`ls -1`
> >+                rm -rf "${unpack_dir}"
> >+                appdir=$(ls -1)
> >+                appdir=$(ls -d *.app)
> 
> nit: Are unpack_dir and appdir meant to be set twice? 
> 
> Other than that, looks good.

Just to avoid temporary variables, I guess. I could use those, or maybe nest the $() to avoid it?
(In reply to Ben Hearsum (:bhearsum) from comment #6)
> (In reply to Simon Fraser [:sfraser] ⌚️GMT from comment #5)
> > Comment on attachment 8960735 [details] [diff] [review]
> > exit with non-zero code when non-binary diffs are detected & fix errors
> > 
> > 
> > >-                unpack_dir=`ls -1`
> > >+                unpack_dir=$(ls -1)
> > >+                unpack_dir=$(ls -d "${unpack_dir}")
> > >                 mv "${unpack_dir}"/*.app .
> > >-                rm -rf $unpack_dir
> > >-                appdir=`ls -1`
> > >+                rm -rf "${unpack_dir}"
> > >+                appdir=$(ls -1)
> > >+                appdir=$(ls -d *.app)
> > 
> > nit: Are unpack_dir and appdir meant to be set twice? 
> > 
> > Other than that, looks good.
> 
> Just to avoid temporary variables, I guess. I could use those, or maybe nest
> the $() to avoid it?

We talked about this on IRC, Simon was just referring to the useless first definition of $appdir - I've removed that from my patch.
Attachment #8960735 - Flags: checked-in+
Pushed by bhearsum@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17fd70df9e43
update verify shouldn't be silent when diffs are found. r=aki
I landed this on mozilla-beta too, to make sure it gets to ESR60: https://hg.mozilla.org/releases/mozilla-beta/rev/118f7f30f68af571995954f4df4bcd7ca2243947
https://hg.mozilla.org/mozilla-central/rev/17fd70df9e43
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: