668348 - add channel fallback support to AUS3

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[[e-mail] (:bhearsum)]

AUS2 supports "fallback" channels, used for partner repacks. Channels matching "foo-cck-bar" will use snippets for that specific channel. If those aren't found, they will use snippets for the plain "foo" channel.

Comment 1

[[e-mail] (:bhearsum)]

Attachment: add fallback support, tests

The tests in this patch are built on top of the work in bug 668345. They can be reworked if changes are needed, though. I think those parts of this patch are pretty straightforward: The 5.0 JSON is built with generate-json.py, with the second Darwin build target + platform independent attributes added manually. The snippets are tar'ed up versions of the 4.0/4.0.1 -> 5.0 snippets.

There's a bunch of spew from from expandRelease for the yahoo tests:
AUS.expandRelease failed to get release data from db

I haven't figured out exactly what to do about that, and I didn't want to comment it out completely.

As for the actual code itself, it's all a tad ugly still, I wanted to get some feedback before cleaning it up.

Comment 2

[[e-mail] (:bhearsum)]

Attachment: fallback support, with all necessary json files

Also rebased against the latest from bug 668345

Comment 3

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

Comment on attachment 544255 [details] [diff] [review]
fallback support, with all necessary json files

>diff -r 10e88a51b472 AUS.py
>+                # If version or channel are None, we'll match anything
>+                # If they are present, the rules may contain a wildcard, so we
>+                # need to do a regex match.
>+                if (row['version'] is None or self.matchesRegex(row['version'], updateQuery['version'])) and \
>+                  (row['channel'] is None or self.matchesRegex(row['channel'], updateQuery['channel']) or self.matchesRegex(row['channel'], self.getFallbackChannel(updateQuery['channel']))):

The general approach looks fine, but this is getting a bit unwieldy. Time to farm it out to helper function ? I suspect a bunch of this code can get some clean up once the test suite fills out a bit.

>diff -r 10e88a51b472 aus-data-snapshots/4.0.1-5.0/Firefox-5.0-build1.tar.gz
>Binary file aus-data-snapshots/4.0.1-5.0/Firefox-5.0-build1.tar.gz has changed

Left out for compactness ?

Comment 4

[[e-mail] (:bhearsum)]

(In reply to comment #3)
> Comment on attachment 544255 [details] [diff] [review] [review]
> fallback support, with all necessary json files
> 
> >diff -r 10e88a51b472 AUS.py
> >+                # If version or channel are None, we'll match anything
> >+                # If they are present, the rules may contain a wildcard, so we
> >+                # need to do a regex match.
> >+                if (row['version'] is None or self.matchesRegex(row['version'], updateQuery['version'])) and \
> >+                  (row['channel'] is None or self.matchesRegex(row['channel'], updateQuery['channel']) or self.matchesRegex(row['channel'], self.getFallbackChannel(updateQuery['channel']))):
> 
> The general approach looks fine, but this is getting a bit unwieldy. Time to
> farm it out to helper function ? I suspect a bunch of this code can get some
> clean up once the test suite fills out a bit.

Yeah, no doubt this needs some cleaning up.

> >diff -r 10e88a51b472 aus-data-snapshots/4.0.1-5.0/Firefox-5.0-build1.tar.gz
> >Binary file aus-data-snapshots/4.0.1-5.0/Firefox-5.0-build1.tar.gz has changed
> 
> Left out for compactness ?

Yeah...Mercurial makes it really hard to show this properly...I had to hand edit the diff in bug 668345 to make things clear :(

Comment 5

[[e-mail] (:bhearsum)]

Attachment: fallback support, cleaned up a bit, rebased against bug 668345

I changed these tests into minimal test cases, like the others from bug 668345. I also factored out the version & channel checks to helper functions.

Comment 6

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

Comment on attachment 545706 [details] [diff] [review]
fallback support, cleaned up a bit, rebased against bug 668345

The code change looks good, but I've got a few suggestions for the tests.

>diff --git a/aus-data-snapshots/fallback-falls-back/Firefox-5.0-build1.json b/aus-data-snapshots/fallback-falls-back/Firefox-5.0-build1.json

For brevity you could just list de in platform's locale list.

>diff --git a/aus-data-snapshots/fallback-falls-back/rules.sql b/aus-data-snapshots/fallback-falls-back/rules.sql

Only need a rule for the release channel.

>diff --git a/aus-data-snapshots/fallback-falls-back/snippets/Firefox/4.0.1/Darwin_x86-gcc3-u-i386-x86_64/20110413222027/de/release/complete.txt b/aus-data-snapshots/fallback-falls-back/snippets/Firefox/4.0.1/Darwin_x86-gcc3-u-i386-x86_64/20110413222027/de/release/complete.txt

Are these /release/ snippets to test the 'release' rule works in the normal case too ? To minimally test fallback I thinking we'd walk release-cck-fallbacktest snippets against a 'release' rule.

>diff --git a/aus-data-snapshots/fallback-override/Firefox-5.0-build1.json b/aus-data-snapshots/fallback-override/Firefox-5.0-build1.json

Could just list ja and ja-JP-mac here.

>diff --git a/aus-data-snapshots/fallback-override/rules.sql b/aus-data-snapshots/fallback-override/rules.sql
>+INSERT INTO update_paths (priority, mapping, throttle, update_type, product, version, channel) VALUES (100, NULL, 100, 'minor', 'Firefox', '4.0*', 'release-cck-yahoo*');

I think we need to make this test stronger because it's a null test. If by some error we didn't hit this rule then there are no others and we get no update, and it looks like the test passes. So how about making it a positive test instead, where the fallback channel has different updates to the release one ?

Comment 7

[[e-mail] (:bhearsum)]

(In reply to comment #6)
> Comment on attachment 545706 [details] [diff] [review] [review]
> fallback support, cleaned up a bit, rebased against bug 668345
> 
> The code change looks good, but I've got a few suggestions for the tests.

These are all good points, I'll address them.

Comment 8

[[e-mail] (:bhearsum)]

Attachment: address review comments

Comment 9

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

Comment on attachment 545943 [details] [diff] [review]
address review comments

>diff -r 3b4ec26e219b aus-data-snapshots/fallback-falls-back/Firefox-4.0.1-build1.json
>diff -r 3b4ec26e219b aus-data-snapshots/fallback-falls-back/Firefox-5.0-build1.json

Looks like you can further minimize the platform list to just Darwin_x86-gcc3-u-i386-x86_64.

>diff -r 3b4ec26e219b aus-data-snapshots/fallback-override/Firefox-4.0.1-build1.json
>diff -r 3b4ec26e219b aus-data-snapshots/fallback-override/Firefox-5.0-build1.json
>diff -r 3b4ec26e219b aus-data-snapshots/fallback-override/rules.sql
>+INSERT INTO update_paths (priority, mapping, throttle, update_type, product, version, channel) VALUES (100, 'Firefox-5.0.1-build1', 100, 'minor', 'Firefox', '4.0*', 'release-cck-yahoo');
>+INSERT INTO update_paths (priority, mapping, throttle, update_type, product, version, channel) VALUES (100, 'Firefox-5.0-build1', 100, 'minor', 'Firefox', '4.0*', 'release');

Looks like the release-cck-yahoo snippets are missing from the diff:
walkSnippets: 0 snippets, 0 PASS, 0 FAIL  (in 0.0 tests/second)

r+ with those nits fixed.

Comment 10

[[e-mail] (:bhearsum)]

Attachment: drop unnecessary platforms, fix up test

(In reply to comment #9)
> Comment on attachment 545943 [details] [diff] [review] [review]
> address review comments
> 
> >diff -r 3b4ec26e219b aus-data-snapshots/fallback-falls-back/Firefox-4.0.1-build1.json
> >diff -r 3b4ec26e219b aus-data-snapshots/fallback-falls-back/Firefox-5.0-build1.json
> 
> Looks like you can further minimize the platform list to just
> Darwin_x86-gcc3-u-i386-x86_64.

Done, and similar for the fallback-override test.

> >diff -r 3b4ec26e219b aus-data-snapshots/fallback-override/Firefox-4.0.1-build1.json
> >diff -r 3b4ec26e219b aus-data-snapshots/fallback-override/Firefox-5.0-build1.json
> >diff -r 3b4ec26e219b aus-data-snapshots/fallback-override/rules.sql
> >+INSERT INTO update_paths (priority, mapping, throttle, update_type, product, version, channel) VALUES (100, 'Firefox-5.0.1-build1', 100, 'minor', 'Firefox', '4.0*', 'release-cck-yahoo');
> >+INSERT INTO update_paths (priority, mapping, throttle, update_type, product, version, channel) VALUES (100, 'Firefox-5.0-build1', 100, 'minor', 'Firefox', '4.0*', 'release');
> 
> Looks like the release-cck-yahoo snippets are missing from the diff:
> walkSnippets: 0 snippets, 0 PASS, 0 FAIL  (in 0.0 tests/second)

Turns out this wasn't added because it was empty, and the test still passed because I didn't have a 5.0.1 json file, so the empty snippet matched the missing 5.0.1 mapping :S. Fixed both of those.

Comment 11

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

Comment on attachment 546177 [details] [diff] [review]
drop unnecessary platforms, fix up test

lgtm

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 546177 [details] [diff] [review]
drop unnecessary platforms, fix up test

Landed on the default branch of my user repo: http://hg.mozilla.org/users/bhearsum_mozilla.com/aus3-proto/rev/16d6abf6cb01

Comment 13

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

Landed in the master repo:
  http://hg.mozilla.org/users/nthomas_mozilla.com/aus3-proto/rev/00541bc0b52e

FIXED ?

Comment 14

[[e-mail] (:bhearsum)]

(In reply to comment #13)
> Landed in the master repo:
>   http://hg.mozilla.org/users/nthomas_mozilla.com/aus3-proto/rev/00541bc0b52e
> 
> FIXED ?

Yup!