Closed Bug 648087 Opened 9 years ago Closed 9 years ago

Support channel changing

Categories

(AUS Graveyard :: API, defect, P1)

defect

Tracking

(firefox5+ fixed)

RESOLVED FIXED
Tracking Status
firefox5 + fixed

People

(Reporter: rstrong, Assigned: rhelmer)

References

()

Details

(Whiteboard: [channel-switcher])

Attachments

(1 file, 4 obsolete files)

The basic approach suggested is the client will have a new param in the url when changing the channel so existing release / nightly process doesn't need to be changed. When this param exists the value will be the name of the channel to change to and the update snippet should point to the latest update for that channel. Ideally the snippet will only contain a complete update but I will most likely add code to the client to only download complete updates when performing a channel change. Another suggestion I think would be good is to have a well known location for each channels change channel update snippet. We've had requests from sysadmins for the ability to query for the latest official release which doing this would provide.
I went with the following for the new param when changing the channel where the value equals the new channel
newchannel=channelname

so changing from aurora to release would be
newchannel=release
(In reply to comment #1)
> I went with the following for the new param when changing the channel where the
> value equals the new channel
> newchannel=channelname
> 
> so changing from aurora to release would be
> newchannel=release

Is this a query parameter, or another value on the request path?
Query per the discussions last week.
Assignee: nobody → rhelmer
Priority: -- → P1
Do we have an ETA on this yet?
(In reply to comment #4)
> Do we have an ETA on this yet?

I am working on this now, my goal is to get a patch up for review by the end of this week.
Status: NEW → ASSIGNED
Scoped this out a bit with catlee in IRC last weekend (linked in URL), summary points are:

* going from release/beta -> nightly/aurora is relatively easy
** it's very similar to our current "latest nightly" logic
* going from aurora/nightly -> release/beta is a little trickier
** it's tough to tell what the latest release is, since it's non-trivial sorting the Firefox version number scheme

I am assuming we don't want to offer the choice of multiple release/beta branches, and just want to present "beta channel" or "release channel", please correct me if this is mistaken.

The current plan is to:

1) add a new server-side config option to AUS, which will allow us to specify the release/beta versions that we want channel-changing users to go to
2) add a new function to get the latest (by buildid) complete.mar for current release/beta (similar to the "latest nightly" logic in getLatestCompleteBuild() )
3) add the newchannel logic to index.php, which uses the above

(In reply to comment #0)
> Another suggestion I think would be good is to
> have a well known location for each channels change channel update snippet.
> We've had requests from sysadmins for the ability to query for the latest
> official release which doing this would provide.

Perhaps we could expose #1 above to provide this, I think it should be a separate bug. Would you mind filing this? I haven't seen the requests so don't have a good feel for the requirements.

Also, although sorting Firefox release version numbers is non-trivial, it's something we've done on other projects, so we could do this automatically in the future instead of having to specify it in the config. For the sake of safety and expediency I'd rather not tackle that as part of adding the channel-changing feature though.
Sounds good and I'll file the bug. Not sure if it is clear but the only channels that will support channel changing to the latest at this time are aurora, beta, and release.
Duplicate of this bug: 648070
Attached patch switch to aurora [wip] (obsolete) — Splinter Review
Switching to aurora (or any channel that uses the "nightly" logic to decide latest) looks fairly straightforward, feedback welcome.

This will also work for users on a branch that has a beta (for example 4.0 with newchannel=beta would get the 4.0.1 complete.mar), although from previous discussions it sounds like what we really want is to send all users to the same beta regardless of the user's current branch. If that is true then this needs an override and alternate way to getLatestCompleteBuild (as described in comment 6).

This approach also will not work for moving to an older release (aurora->beta, beta->release), and we want the same branch-overriding behavior as described above.

I am thinking of adding a way to set Patch->isChannelChanging in (similar to the current isNightlyChannel) to make the necessary changes in findPatch more straightforward, to avoid adding more args to findPatch()
Attachment #527308 - Flags: feedback?(catlee)
note: the mar files previous to bug 386760 landing do not support channel changing so going back to any mar generated prior to that landing or on a branch without those changes should not be supported. Also, it was decided that we don't want anyone changing to any 4.0.x builds so we don't lose testers on Aurora.
(In reply to comment #10)
> note: the mar files previous to bug 386760 landing do not support channel
> changing so going back to any mar generated prior to that landing or on a
> branch without those changes should not be supported. 

We'll be able to control (via AUS config) which version "channel changers" asking for beta/release get, so I think this will be covered. Thanks for pointing it out, need to keep this in mind when testing.

> Also, it was decided that
> we don't want anyone changing to any 4.0.x builds so we don't lose testers on
> Aurora.

I think we'll be covered here too as above. Users switching to "release", "beta" or "aurora" will get the same build regardless of the branch/channel they started out on. 

If any of this sounds off, please let me know.
Comment on attachment 527308 [details] [diff] [review]
switch to aurora [wip]

>@@ -172,6 +178,7 @@
>             // Check for OS_VERSION values and scrub the URI to make sure we aren't getting a malformed request
>             // from a client suffering from bug 360127.
>             if (empty($clean['platformVersion']) || $clean['platformVersion']=='%OS_VERSION%' || preg_match('/^1\.5.*$/',$clean['version'])) {
>+                error_log('malformed request');

What's this for?

I also don't see any handling of moving from e.g. aurora -> beta. There won't be any snippets for the users's build id in the beta snippets directory.
Attachment #527308 - Flags: feedback?(catlee) → feedback-
Comment on attachment 527308 [details] [diff] [review]
switch to aurora [wip]

misunderstood that this is only for the X -> aurora side of things.
Attachment #527308 - Flags: feedback- → feedback+
(In reply to comment #12)
> Comment on attachment 527308 [details] [diff] [review]
> switch to aurora [wip]
> 
> >@@ -172,6 +178,7 @@
> >             // Check for OS_VERSION values and scrub the URI to make sure we aren't getting a malformed request
> >             // from a client suffering from bug 360127.
> >             if (empty($clean['platformVersion']) || $clean['platformVersion']=='%OS_VERSION%' || preg_match('/^1\.5.*$/',$clean['version'])) {
> >+                error_log('malformed request');
> 
> What's this for?


We don't need to keep in the final patch, but normally this fails silently so I've had it on for testing.

 
> I also don't see any handling of moving from e.g. aurora -> beta. There won't
> be any snippets for the users's build id in the beta snippets directory.

(In reply to comment #13)
> Comment on attachment 527308 [details] [diff] [review]
> switch to aurora [wip]
> 
> misunderstood that this is only for the X -> aurora side of things.

We discussed in IRC; nothing jumps out at catlee from my comment, working on a patch now.
Attached patch support channel changing (obsolete) — Splinter Review
This supports channel changing to nightly channels (incl. aurora) as well as release/beta (incl. releasetest/betatest). The latter must be configured, and allows changing per product as well as per channel; for example the following is possible:

Firefox Beta -> 6.0
Firefox Release -> 5.0

Default is hardcoded to 5.0 for both of the above, but I've tested with this set to 4.0 and it seems to work as intended ("newchannel=release" will give a complete to the latest Firefox 4.0 build in that case).

I've done limited local testing (with a full production dataset) for the usual nightly, release, beta cases as well as all permutations of channel-changing. Working on adding to the automated AUS testsuite.
Attachment #527308 - Attachment is obsolete: true
Attachment #527855 - Flags: review?(catlee)
Attached patch support channel changing (obsolete) — Splinter Review
Same as attachment 527855 [details] [diff] [review], but take out that "error_log" line I said I would remove in comment 14 (I think it's a fine thing to do, but should be done in a different patch on a different bug).
Attachment #527855 - Attachment is obsolete: true
Attachment #527855 - Flags: review?(catlee)
Attachment #527856 - Flags: review?(catlee)
Comment on attachment 527856 [details] [diff] [review]
support channel changing

changes to index.php and config-dist.php look good

>@@ -231,6 +239,7 @@
>     function findPatch($product,$platform,$locale,$version,$build,$channel=null) {
> 
>         // If a specific update exists for the specified channel, it takes priority over the branch update.
>+        $this->setPath($product,$platform,$locale,$version,$build,3,$channel);
>         if (!empty($channel) && $this->setPath($product,$platform,$locale,$version,$build,3,$channel) && file_exists($this->path) && filesize($this->path) > 0) {
>             return $this->setSnippet($this->path); 
>         } 

What's this extra call do? Does calling setPath with $channel=null work?

>@@ -257,18 +266,34 @@
>         // Determine the branch of the client's version.
>         $branchVersion = $this->getBranch($product,$version,$channel);
> 
>-        // Otherwise, if it is a complete patch and a nightly channel, force the complete update to take the user to the latest build.
>-        if ($this->isComplete() && $this->isNightlyChannel($channel)) {
>-
>-            // Get the latest build that has an update for this branch.
>-            $latestCompleteBuild = $this->getLatestCompleteBuild($product,$branchVersion,$platform,$locale);
>+        if ($this->isComplete()) {
>+            // If it is a complete patch and a nightly channel, force the complete update to take the user to the latest build.
>+            if ($this->isNightlyChannel($channel) || $this->isChangingChannel()) {
>+                $buildSource = 3;
>+                if ($this->isNightlyChannel($channel)) {
>+                    $buildSource = 2;
>+                }
>+                // Get the latest build that has an update for this branch.
> 
>-            // If we have the latest complete build, the path is valid, the file exists, and the filesize is greater than zero, we have a valid complete patch.
>-            if ($latestCompleteBuild && $this->setPath($product,$platform,$locale,$branchVersion,$latestCompleteBuild,2,$channel) && file_exists($this->path) && filesize($this->path) > 0) {
>-                $this->setSnippet($this->path); 
>-                return $this->isNewerBuild($build);
>+               if (!$this->isNightlyChannel($channel)) {
>+                   $branchVersion = $this->latestRelease[$product][$channel];
>+                   if (!$branchVersion) {
>+                       return false;
>+                   }
>+                }

Is this clearer as an 'else' clause instead of if (!$this->isNightlyChannel($channel))

The rest looks good!
Attachment #527856 - Flags: review?(catlee) → review+
Attached patch support channel changing v2 (obsolete) — Splinter Review
Address review comments from comment 17 (both spot-on, thanks!)

I noticed in testing that the isNewerBuild check in the nightly logic should be adjusted, reverse the order to "if (isChangingChannel) return true/else isNightlyChannel return isNewerBuild" to avoid the isNewerBuild check for channel-changers.

Use isset on latestRelease before trying to index it with potentially random data.

Finally, add a basic test for newchannel param to test suite - we don't have a mapping for $productBranchVersions for test purposes so will need to think about how to automate testing for nightly newchannel, but verify that (a) using newchannel to switch to a release works and (b) not having newchannel set does not break any existing AUS tests
Attachment #527856 - Attachment is obsolete: true
Attachment #528161 - Flags: review?(catlee)
Just wanted to make sure that if there is no update available for the new channel that an empty updates snippet is returned?
What should have been attachment 528161 [details] [diff] [review]
Attachment #528161 - Attachment is obsolete: true
Attachment #528161 - Flags: review?(catlee)
Attachment #528178 - Flags: review?(catlee)
(In reply to comment #19)
> Just wanted to make sure that if there is no update available for the new
> channel that an empty updates snippet is returned?

I was a little confused by the question, clarified in IRC:

<rs> just want to make sure that if there isn't and update available for the new channel that it doesn't offer one for the current channel

Yes "newchannel" supersedes the regular request, so if you are on e.g. 4.0 and 4.0.1 is available, "newchannel=aurora" will override the normal request and take you directly to the latest aurora.

However the only way you'd get an empty update response is if there is no valid branch/channel to update to, for example if AUS is configured to offer "6.0 release" to channel-changers for "release", and that does not exist, the update response will be empty. If there have been any 6.0 releases, then "newchannel=release" will automatically select the latest and return a complete update response for the latest.
Attachment #528178 - Flags: review?(catlee) → review+
Checking in tests/Verify.py;
/cvsroot/mozilla/webtools/aus/tests/Verify.py,v  <--  Verify.py
new revision: 1.9; previous revision: 1.8
done
Checking in tests/Verify.txt;
/cvsroot/mozilla/webtools/aus/tests/Verify.txt,v  <--  Verify.txt
new revision: 1.19; previous revision: 1.18
done
Checking in xml/index.php;
/cvsroot/mozilla/webtools/aus/xml/index.php,v  <--  index.php
new revision: 1.32; previous revision: 1.31
done
Checking in xml/inc/config-dist.php;
/cvsroot/mozilla/webtools/aus/xml/inc/config-dist.php,v  <--  config-dist.php
new revision: 1.135; previous revision: 1.134
done
Checking in xml/inc/patch.class.php;
/cvsroot/mozilla/webtools/aus/xml/inc/patch.class.php,v  <--  patch.class.php
new revision: 1.26; previous revision: 1.25
done
Filed bug 653216 as a followup to make it easier to track releases (right now, config must be bumped for each release)

Updated test config to fix test failures:
Checking in config-test.php;
/cvsroot/mozilla/webtools/aus/xml/inc/config-test.php,v  <--  config-test.php
new revision: 1.3; previous revision: 1.2
done

All tests pass: https://hudson.mozilla.org/job/AUSv2/103/
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Found bug 653312 testing this in staging, looks ok otherwise so far.
Depends on: 653312
Depends on: 653781
Whiteboard: [channel-switcher]
You need to log in before you can comment on or make changes to this bug.