Last Comment Bug 1140262 - Release diffs are wrong
: Release diffs are wrong
Status: RESOLVED FIXED
[lang=js]
:
Product: Release Engineering
Classification: Other
Component: Balrog: Backend (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: ---
Assigned To: Kumar Rishabh
: Ben Hearsum (:bhearsum)
:
Mentors: Ben Hearsum (:bhearsum)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-05 20:12 PST by Nick Thomas [:nthomas]
Modified: 2016-02-29 06:26 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch (1.75 KB, patch)
2016-02-23 16:59 PST, Kumar Rishabh
no flags Details | Diff | Splinter Review
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch (4.07 KB, patch)
2016-02-24 16:07 PST, Kumar Rishabh
bhearsum: feedback+
Details | Diff | Splinter Review
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch (5.08 KB, patch)
2016-02-25 07:44 PST, Kumar Rishabh
bhearsum: review+
bhearsum: checked‑in+
Details | Diff | Splinter Review

Description User image Nick Thomas [:nthomas] 2015-03-05 20:12:31 PST
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 User image Nick Thomas [:nthomas] 2015-03-05 20:13:15 PST
The decrementing is at https://github.com/mozilla/balrog/blob/master/auslib/admin/views/history.py#L59.
Comment 2 User image Kumar Rishabh 2016-02-23 16:59:33 PST
Created attachment 8722775 [details] [diff] [review]
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch
Comment 3 User image Ben Hearsum (:bhearsum) 2016-02-24 07:09:28 PST
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 User image Kumar Rishabh 2016-02-24 16:07:27 PST
Created attachment 8723301 [details] [diff] [review]
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch
Comment 5 User image Ben Hearsum (:bhearsum) 2016-02-25 06:20:14 PST
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 User image Kumar Rishabh 2016-02-25 07:44:05 PST
Created attachment 8723580 [details] [diff] [review]
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch
Comment 7 User image [github robot] 2016-02-25 08:32:12 PST
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 User image Ben Hearsum (:bhearsum) 2016-02-25 08:33:06 PST
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 User image Ben Hearsum (:bhearsum) 2016-02-25 09:38:54 PST
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 User image Ben Hearsum (:bhearsum) 2016-02-25 12:10:24 PST
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 User image Nick Thomas [:nthomas] 2016-02-25 13:43:34 PST
(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 User image Ben Hearsum (:bhearsum) 2016-02-29 06:26:31 PST
Looking good in production. Thanks a lot Kumar!

Note You need to log in before you can comment on or make changes to this bug.