The default bug view has changed. See this FAQ.

Status

Release Engineering
Balrog: Backend
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: nthomas, Assigned: Kumar Rishabh, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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'.
(Reporter)

Comment 1

2 years ago
The decrementing is at https://github.com/mozilla/balrog/blob/master/auslib/admin/views/history.py#L59.
Mentor: bhearsum@mozilla.com
Whiteboard: [lang=js]
(Assignee)

Comment 2

a year ago
Created attachment 8722775 [details] [diff] [review]
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch
Attachment #8722775 - Flags: review?(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.
(Assignee)

Comment 4

a year ago
Created attachment 8723301 [details] [diff] [review]
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch
Attachment #8722775 - Attachment is obsolete: true
Attachment #8722775 - Flags: review?(bhearsum)
Attachment #8723301 - Flags: review?(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.
Attachment #8723301 - Flags: review?(bhearsum) → feedback+
(Assignee)

Comment 6

a year ago
Created attachment 8723580 [details] [diff] [review]
0001-Bug-1140262-Solves-wrong-Release-Diffs-issue.patch
Attachment #8723301 - Attachment is obsolete: true
Attachment #8723580 - Flags: review?(bhearsum)

Comment 7

a year ago
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 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.
Attachment #8723580 - Flags: review?(bhearsum)
Attachment #8723580 - Flags: review+
Attachment #8723580 - Flags: checked-in+
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?
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?
(Reporter)

Comment 11

a year ago
(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.
Assignee: nobody → shailrishabh
Looking good in production. Thanks a lot Kumar!
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.