launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/github-use-issue-number into lp:launchpad.

Commit message:
Expect the upstream bug ID in the "number" field of GitHub issue objects, not the "id" field.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1824728 in Launchpad itself: "GitHub bug watches fail due to "id" vs. "number" confusion"
  https://bugs.launchpad.net/launchpad/+bug/1824728

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/github-use-issue-number/+merge/366083

I'm not sure how this ever worked.  Possibly GitHub changed the semantics of the "id" field?  It seems to now be a fairly arbitrary integer that we shouldn't be using for anything meaningful on our end, much like the distinction between "id" and "iid" in GitLab:

  $ curl -s https://api.github.com/repos/calamares/calamares/issues?state=all | jq -c '.[:3] | .[] | {url, id, number}'
  {"url":"https://api.github.com/repos/calamares/calamares/issues/1123","id":433299827,"number":1123}
  {"url":"https://api.github.com/repos/calamares/calamares/issues/1122","id":433268843,"number":1122}
  {"url":"https://api.github.com/repos/calamares/calamares/issues/1121","id":433115193,"number":1121}

https://developer.github.com/v3/issues/ documents the number field, albeit only in its example responses.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/github-use-issue-number into lp:launchpad.

=== modified file 'lib/lp/bugs/externalbugtracker/github.py'
--- lib/lp/bugs/externalbugtracker/github.py	2018-07-13 12:48:19 +0000
+++ lib/lp/bugs/externalbugtracker/github.py	2019-04-15 23:23:49 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """GitHub ExternalBugTracker utility."""
@@ -183,10 +183,10 @@
         for remote_bug in self._getCollection(page):
             # We're only interested in the bug if it's one of the ones in
             # bug_ids.
-            if remote_bug["id"] not in bug_ids:
+            if remote_bug["number"] not in bug_ids:
                 continue
-            bugs[remote_bug["id"]] = remote_bug
-            self.cached_bugs[remote_bug["id"]] = remote_bug
+            bugs[remote_bug["number"]] = remote_bug
+            self.cached_bugs[remote_bug["number"]] = remote_bug
         return bugs
 
     def getRemoteImportance(self, bug_id):

=== modified file 'lib/lp/bugs/externalbugtracker/tests/test_github.py'
--- lib/lp/bugs/externalbugtracker/tests/test_github.py	2018-06-22 21:10:36 +0000
+++ lib/lp/bugs/externalbugtracker/tests/test_github.py	2019-04-15 23:23:49 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the GitHub Issues BugTracker."""
@@ -138,12 +138,14 @@
         super(TestGitHub, self).setUp()
         self.addCleanup(getUtility(IGitHubRateLimit).clearCache)
         self.sample_bugs = [
-            {"id": 1, "state": "open", "labels": []},
-            {"id": 2, "state": "open", "labels": [{"name": "feature"}]},
-            {"id": 3, "state": "open",
+            {"id": 101, "number": 1, "state": "open", "labels": []},
+            {"id": 102, "number": 2, "state": "open",
+             "labels": [{"name": "feature"}]},
+            {"id": 103, "number": 3, "state": "open",
              "labels": [{"name": "feature"}, {"name": "ui"}]},
-            {"id": 4, "state": "closed", "labels": []},
-            {"id": 5, "state": "closed", "labels": [{"name": "feature"}]},
+            {"id": 104, "number": 4, "state": "closed", "labels": []},
+            {"id": 105, "number": 5, "state": "closed",
+             "labels": [{"name": "feature"}]},
             ]
 
     def test_implements_interface(self):
@@ -220,7 +222,7 @@
         self._addIssuesResponse()
         tracker = GitHub("https://github.com/user/repository/issues";)
         self.assertEqual(
-            {bug["id"]: bug for bug in self.sample_bugs[:2]},
+            {bug["number"]: bug for bug in self.sample_bugs[:2]},
             tracker.getRemoteBugBatch(["1", "2"]))
         self.assertEqual(
             "https://api.github.com/repos/user/repository/issues?state=all";,
@@ -233,7 +235,7 @@
         tracker = GitHub("https://github.com/user/repository/issues";)
         since = datetime(2015, 1, 1, 12, 0, 0, tzinfo=pytz.UTC)
         self.assertEqual(
-            {bug["id"]: bug for bug in self.sample_bugs[:2]},
+            {bug["number"]: bug for bug in self.sample_bugs[:2]},
             tracker.getRemoteBugBatch(["1", "2"], last_accessed=since))
         self.assertEqual(
             "https://api.github.com/repos/user/repository/issues?";
@@ -246,10 +248,10 @@
         self._addIssuesResponse()
         tracker = GitHub("https://github.com/user/repository/issues";)
         tracker.initializeRemoteBugDB(
-            [str(bug["id"]) for bug in self.sample_bugs])
+            [str(bug["number"]) for bug in self.sample_bugs])
         responses.reset()
         self.assertEqual(
-            {bug["id"]: bug for bug in self.sample_bugs[:2]},
+            {bug["number"]: bug for bug in self.sample_bugs[:2]},
             tracker.getRemoteBugBatch(["1", "2"]))
         self.assertEqual(0, len(responses.calls))
 
@@ -278,9 +280,9 @@
             callback=issues_callback, content_type="application/json")
         tracker = GitHub("https://github.com/user/repository/issues";)
         self.assertEqual(
-            {bug["id"]: bug for bug in self.sample_bugs},
+            {bug["number"]: bug for bug in self.sample_bugs},
             tracker.getRemoteBugBatch(
-                [str(bug["id"]) for bug in self.sample_bugs]))
+                [str(bug["number"]) for bug in self.sample_bugs]))
         expected_urls = [
             "https://api.github.com/rate_limit";,
             "https://api.github.com/repos/user/repository/issues?state=all";,
@@ -293,9 +295,9 @@
     @responses.activate
     def test_status_open(self):
         self.sample_bugs = [
-            {"id": 1, "state": "open", "labels": []},
+            {"id": 101, "number": 1, "state": "open", "labels": []},
             # Labels do not affect status, even if names collide.
-            {"id": 2, "state": "open",
+            {"id": 102, "number": 2, "state": "open",
              "labels": [{"name": "feature"}, {"name": "closed"}]},
             ]
         _add_rate_limit_response("api.github.com")
@@ -314,9 +316,9 @@
     @responses.activate
     def test_status_closed(self):
         self.sample_bugs = [
-            {"id": 1, "state": "closed", "labels": []},
+            {"id": 101, "number": 1, "state": "closed", "labels": []},
             # Labels do not affect status, even if names collide.
-            {"id": 2, "state": "closed",
+            {"id": 102, "number": 2, "state": "closed",
              "labels": [{"name": "feature"}, {"name": "open"}]},
             ]
         _add_rate_limit_response("api.github.com")
@@ -339,7 +341,9 @@
 
     @responses.activate
     def test_process_one(self):
-        remote_bug = {"id": 1234, "state": "open", "labels": []}
+        remote_bug = {
+            "id": 12345, "number": 1234, "state": "open", "labels": [],
+            }
         _add_rate_limit_response("api.github.com")
         responses.add(
             "GET", "https://api.github.com/repos/user/repository/issues/1234";,
@@ -371,7 +375,7 @@
     @responses.activate
     def test_process_many(self):
         remote_bugs = [
-            {"id": bug_id,
+            {"id": bug_id + 1, "number": bug_id,
              "state": "open" if (bug_id % 2) == 0 else "closed",
              "labels": []}
             for bug_id in range(1000, 1010)]
@@ -385,7 +389,7 @@
             bugtrackertype=BugTrackerType.GITHUB)
         for remote_bug in remote_bugs:
             bug.addWatch(
-                bug_tracker, str(remote_bug["id"]),
+                bug_tracker, str(remote_bug["number"]),
                 getUtility(ILaunchpadCelebrities).janitor)
         transaction.commit()
         logger = BufferLogger()