1140262 - Release diffs are wrong

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), defect)

Description

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

I was modifying Firefox-29.0b8-build1-schema2, adding
    "Darwin_x86_64-gcc3": {
      "alias": "Darwin_x86-gcc3-u-i386-x86_64"
    },

The history for that release is at https://aus4-admin.mozilla.org/releases/Firefox-29.0b8-build1-schema2, and the View Diff button has these examples of wrongness:

  {
+   "actions": "showURL", 
+   "appVersion": "29.0", 
+   "bouncerProducts": {
+     "complete": "firefox-29.0b8-complete", 
+     "partial": "firefox-29.0b8-partial-29.0b7"
+   }, 

-   "name": "B2G-mozilla-b2g34_v2_1-kitkat-nightly-latest", 
+   "name": "Firefox-29.0b8-build1", 

-   "schema_version": 4
?                     ^

+   "schema_version": 2
?                     ^

A 'View Data' the old blob is
https://aus4-admin.mozilla.org/api/history/view/release/655803/data

And on the modified blob is
https://aus4-admin.mozilla.org/api/history/view/release/1126639/data

Comparing those two gives only:
+    "Darwin_x86_64-gcc3": {
+      "alias": "Darwin_x86-gcc3-u-i386-x86_64"
+    },

The diff requests https://aus4-admin.mozilla.org/api/history/diff/release/1126639/data, and I think we calculate the previous change_id incorrectly (simply decrementing it).

Sure enough, https://aus4-admin.mozilla.org/api/history/view/release/1126638/data contains 'B2G-mozilla-b2g34_v2_1-kitkat-nightly-latest'.

Comment 1

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

The decrementing is at https://github.com/mozilla/balrog/blob/master/auslib/admin/views/history.py#L59.

Comment 2

[Kumar Rishabh]

Attachment: 0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch

Comment 3

[[e-mail] (:bhearsum)]

Comment on attachment 8722775 [details] [diff] [review]
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch

Review of attachment 8722775 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you so much, this looks like it works well! Is there any chance you can add a test to make sure we don't regress this in the future? I think we need a test similar to https://github.com/mozilla/balrog/blob/master/auslib/test/admin/views/test_history.py#L34 which manipulates multiple releases instead of just one.

Comment 4

[Kumar Rishabh]

Attachment: 0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 8723301 [details] [diff] [review]
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch

Review of attachment 8723301 [details] [diff] [review]:
-----------------------------------------------------------------

::: auslib/test/admin/views/test_history.py
@@ +91,4 @@
>          self.assertTrue('"fakePartials": true' in ret.data)
>          self.assertTrue('"fakePartials": false' in ret.data)
>  
> +        # Let's add a separate release say for b(already present in the setUp)

The contents of this test look OK, but I'd like to see it in its own test case. We generally try to test one thing per test, and this new code is a regression test for this bug. r+ with this in its own test.

Comment 6

[Kumar Rishabh]

Attachment: 0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch

Comment 7

[[github robot]]

Commit pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/10629fa3d243beca9ad222472ca67a354c0bd08e
bug 1140262: release diffs are wrong. r=bhearsum

Comment 8

[[e-mail] (:bhearsum)]

Comment on attachment 8723580 [details] [diff] [review]
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch

Looks good now, thank you so much! I pushed this as https://github.com/mozilla/balrog/commit/10629fa3d243beca9ad222472ca67a354c0bd08e. We'll do some additional verification in dev, and put it in production shortly thereafter.

Comment 9

[[e-mail] (:bhearsum)]

This seems to work fine in dev for me, modulo a different bug I found, where change_ids are calculated wrong for releases like Firefox-mozilla-central-nightly-latest (which have tons and tons of revisions). I can repro that one in production, so I'm pretty sure this patch didn't cause it...

Nick, do you want to have a look in dev, too?

Comment 10

[[e-mail] (:bhearsum)]

Good news! Turns out that this patch actually fixes the bug I thought I discovered in comment #9. dev didn't automatically restart Apache when it deployed this, which is why I hit the exception I did. The reason I thought it was active was because I was looking at a diff that happened to correctly find the old change id (because multiple changes to the same blob were submitted in a row). I'm seeing diffs like this in Firefox-mozilla-central-nightly-latest now:
-             "fileUrl": "http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/06/2013-06-20-03-10-10-mozilla-central-l10n/firefox-24.0a1.lt.linux-i686.partial.20130619031048-20130620031010.mar", 
?                                                                                                                                                                            ^^^^^^^^

+             "fileUrl": "http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/06/2013-06-20-03-10-10-mozilla-central-l10n/firefox-24.0a1.lt.linux-i686.partial.20130620031010-20130620031010.mar", 
?                                                                                                                                                                            ^^^^^^^^

-             "filesize": "5733300", 
?                          ^^^^^^^

+             "filesize": "16821", 
?                          ^^^^^

-             "from": "Firefox-mozilla-central-nightly-20130619031048", 
?                                                            ^^    ^^

+             "from": "Firefox-mozilla-central-nightly-20130620031010", 
?                                                            ^^    ^^

-             "hashValue": "04808c3a7bbe57fda4e95cba87dff9274af3545e10db46fe777cb7c85a0f429f0eed2933e8c42a270c92e27d028677e584eb419887be9753c323e4e3ceee166e"
+             "hashValue": "ae0dc28689a32e1f35e7b5b4e8d0935606ea49cd173115a962dd6f87a126ee6a47fb9b32b6458a3a34199ef66e3f424de6c74ccdd4204933e11c7f3993232954"
            }
          }, 


The "?" looks a little funny to me at first, but AFAICT that's just a way of showing column diffs, so I think it's correct?

Comment 11

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

(In reply to Ben Hearsum (:bhearsum) from comment #9)
> Nick, do you want to have a look in dev, too?

Bug 1206972 makes it a bit hard to tell (because the UI shows you really old revisions rather than new ones) but I think this looks good.

Comment 12

[[e-mail] (:bhearsum)]

Looking good in production. Thanks a lot Kumar!