launchpad-reviewers team mailing list archive

[Colin Watson <mp+408167@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:charmhub-upload-fix-check-status into launchpad:master.

Commit message:
Fix CharmhubClient.checkStatus calls in CharmhubUploadJob

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/408167

The interface and the calling code in the job disagreed with the method signature in the implementation.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:charmhub-upload-fix-check-status into launchpad:master.

diff --git a/lib/lp/charms/interfaces/charmhubclient.py b/lib/lp/charms/interfaces/charmhubclient.py
index 57c069f..c1b4a37 100644
--- a/lib/lp/charms/interfaces/charmhubclient.py
+++ b/lib/lp/charms/interfaces/charmhubclient.py
@@ -105,9 +105,10 @@ class ICharmhubClient(Interface):
             failed.
         """
 
-    def checkStatus(status_url):
+    def checkStatus(build, status_url):
         """Poll Charmhub once for upload scan status.
 
+        :param build: The `ICharmRecipeBuild` being uploaded.
         :param status_url: A URL as returned by `upload`.
         :raises UploadNotReviewedYetResponse: if the upload has not yet been
             reviewed.
diff --git a/lib/lp/charms/model/charmrecipebuildjob.py b/lib/lp/charms/model/charmrecipebuildjob.py
index 54b6dbb..bafe2ba 100644
--- a/lib/lp/charms/model/charmrecipebuildjob.py
+++ b/lib/lp/charms/model/charmrecipebuildjob.py
@@ -316,7 +316,8 @@ class CharmhubUploadJob(CharmRecipeBuildJobDerived):
                     # We made progress, so reset attempt_count.
                     self.attempt_count = 1
                 if self.store_revision is None:
-                    self.store_revision = client.checkStatus(self.status_url)
+                    self.store_revision = client.checkStatus(
+                        self.build, self.status_url)
                     if self.store_revision is None:
                         raise AssertionError(
                             "checkStatus returned successfully but with no "
diff --git a/lib/lp/charms/tests/test_charmrecipebuildjob.py b/lib/lp/charms/tests/test_charmrecipebuildjob.py
index 33dc021..6c7aaa9 100644
--- a/lib/lp/charms/tests/test_charmrecipebuildjob.py
+++ b/lib/lp/charms/tests/test_charmrecipebuildjob.py
@@ -70,6 +70,10 @@ def run_isolated_jobs(jobs):
         transaction.abort()
 
 
+# XXX cjwatson 2021-09-06: This approach makes it too easy to commit
+# type-safety errors.  Perhaps we should fake the HTTP responses instead, or
+# otherwise ensure that the signatures of the faked methods match the real
+# ones?
 @implementer(ICharmhubClient)
 class FakeCharmhubClient:
 
@@ -183,7 +187,8 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
         with dbuser(config.ICharmhubUploadJobSource.dbuser):
             run_isolated_jobs([job])
         self.assertEqual([((build,), {})], client.upload.calls)
-        self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
+        self.assertEqual(
+            [((build, self.status_url), {})], client.checkStatus.calls)
         self.assertEqual([], client.release.calls)
         self.assertContentEqual([job], build.store_upload_jobs)
         self.assertEqual(1, job.store_revision)
@@ -299,7 +304,8 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
         with dbuser(config.ICharmhubUploadJobSource.dbuser):
             run_isolated_jobs([job])
         self.assertEqual([((build,), {})], client.upload.calls)
-        self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
+        self.assertEqual(
+            [((build, self.status_url), {})], client.checkStatus.calls)
         self.assertEqual([], client.release.calls)
         self.assertContentEqual([job], build.store_upload_jobs)
         self.assertEqual(1, job.store_revision)
@@ -379,7 +385,8 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
         with dbuser(config.ICharmhubUploadJobSource.dbuser):
             run_isolated_jobs([job])
         self.assertEqual([((build,), {})], client.upload.calls)
-        self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
+        self.assertEqual(
+            [((build, self.status_url), {})], client.checkStatus.calls)
         self.assertEqual([], client.release.calls)
         self.assertContentEqual([job], build.store_upload_jobs)
         self.assertIsNone(job.store_revision)
@@ -397,7 +404,8 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
         with dbuser(config.ICharmhubUploadJobSource.dbuser):
             run_isolated_jobs([job])
         self.assertEqual([], client.upload.calls)
-        self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
+        self.assertEqual(
+            [((build, self.status_url), {})], client.checkStatus.calls)
         self.assertEqual([], client.release.calls)
         self.assertContentEqual([job], build.store_upload_jobs)
         self.assertEqual(1, job.store_revision)
@@ -426,7 +434,8 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
         with dbuser(config.ICharmhubUploadJobSource.dbuser):
             run_isolated_jobs([job])
         self.assertEqual([((build,), {})], client.upload.calls)
-        self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
+        self.assertEqual(
+            [((build, self.status_url), {})], client.checkStatus.calls)
         self.assertEqual([], client.release.calls)
         self.assertContentEqual([job], build.store_upload_jobs)
         self.assertIsNone(job.store_revision)
@@ -474,7 +483,8 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
         with dbuser(config.ICharmhubUploadJobSource.dbuser):
             run_isolated_jobs([job])
         self.assertEqual([((build,), {})], client.upload.calls)
-        self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
+        self.assertEqual(
+            [((build, self.status_url), {})], client.checkStatus.calls)
         self.assertEqual([((build, 1), {})], client.release.calls)
         self.assertContentEqual([job], build.store_upload_jobs)
         self.assertEqual(1, job.store_revision)
@@ -503,7 +513,8 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
         with dbuser(config.ICharmhubUploadJobSource.dbuser):
             JobRunner([job]).runAll()
         self.assertEqual([((build,), {})], client.upload.calls)
-        self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
+        self.assertEqual(
+            [((build, self.status_url), {})], client.checkStatus.calls)
         self.assertEqual([((build, 1), {})], client.release.calls)
         self.assertContentEqual([job], build.store_upload_jobs)
         self.assertEqual(1, job.store_revision)