Merge lp:~cjwatson/launchpad/github-use-issue-number into lp:launchpad

[Colin Watson]

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

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.

[Tom Wardill (twom)]

Change lgtm. Wonder if the id field is now a global ID, rather than a repo specific id?

review: Approve

[Colin Watson (cjwatson)]

Yeah, it seems to be exactly that. Just confusing that this didn't come up before.

=== 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()