launchpad-reviewers team mailing list archive

[Colin Watson <mp+408182@xxxxxxxxxxxxxxxxxx>]

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

Commit message:
Fix up URL in CharmhubClient.checkStatus

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

Charmhub's `/v1/charm/:name/revisions` endpoint in fact returns a *relative* URL, not an absolute one.  Fix `CharmhubClient.checkStatus` to make it absolute so that we can actually make requests to it.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:charmhub-upload-fix-check-status-url into launchpad:master.

diff --git a/lib/lp/charms/model/charmhubclient.py b/lib/lp/charms/model/charmhubclient.py
index a376097..112a73e 100644
--- a/lib/lp/charms/model/charmhubclient.py
+++ b/lib/lp/charms/model/charmhubclient.py
@@ -223,6 +223,8 @@ class CharmhubClient:
     @classmethod
     def checkStatus(cls, build, status_url):
         """See `ICharmhubClient`."""
+        status_url = urlappend(
+            config.charms.charmhub_url, status_url.lstrip("/"))
         macaroon_raw = _get_macaroon(build.recipe)
         request = get_current_browser_request()
         timeline_action = get_request_timeline(request).start(
diff --git a/lib/lp/charms/tests/test_charmhubclient.py b/lib/lp/charms/tests/test_charmhubclient.py
index ce61daa..176b14c 100644
--- a/lib/lp/charms/tests/test_charmhubclient.py
+++ b/lib/lp/charms/tests/test_charmhubclient.py
@@ -156,8 +156,8 @@ class TestCharmhubClient(TestCaseWithFactory):
             status=200,
             json={
                 "status-url": (
-                    "http://charmhub.example/v1/charm/{}/revisions/review";
-                    "?upload-id=123".format(quote(name))),
+                    "/v1/charm/{}/revisions/review?upload-id=123".format(
+                        quote(name))),
                 })
 
     def _addCharmReleaseResponse(self, name):
@@ -311,8 +311,7 @@ class TestCharmhubClient(TestCaseWithFactory):
         # config.ICharmhubUploadJobSource.dbuser once that job exists.
         with dbuser("charm-build-job"):
             self.assertEqual(
-                "http://charmhub.example/v1/charm/test-charm/revisions/review";
-                "?upload-id=123",
+                "/v1/charm/test-charm/revisions/review?upload-id=123",
                 self.client.upload(build))
         requests = [call.request for call in responses.calls]
         self.assertThat(requests, MatchesListwise([
@@ -380,11 +379,9 @@ class TestCharmhubClient(TestCaseWithFactory):
     def test_checkStatus_pending(self):
         self._setUpSecretStorage()
         build = self.makeUploadableCharmRecipeBuild()
-        status_url = (
-            "http://charmhub.example/v1/charm/test-charm/revisions/review";
-            "?upload-id=123")
+        status_url = "/v1/charm/test-charm/revisions/review?upload-id=123"
         responses.add(
-            "GET", status_url,
+            "GET", "http://charmhub.example"; + status_url,
             json={
                 "revisions": [
                     {
@@ -403,11 +400,9 @@ class TestCharmhubClient(TestCaseWithFactory):
     def test_checkStatus_error(self):
         self._setUpSecretStorage()
         build = self.makeUploadableCharmRecipeBuild()
-        status_url = (
-            "http://charmhub.example/v1/charm/test-charm/revisions/review";
-            "?upload-id=123")
+        status_url = "/v1/charm/test-charm/revisions/review?upload-id=123"
         responses.add(
-            "GET", status_url,
+            "GET", "http://charmhub.example"; + status_url,
             json={
                 "revisions": [
                     {
@@ -428,11 +423,9 @@ class TestCharmhubClient(TestCaseWithFactory):
     def test_checkStatus_approved_no_revision(self):
         self._setUpSecretStorage()
         build = self.makeUploadableCharmRecipeBuild()
-        status_url = (
-            "http://charmhub.example/v1/charm/test-charm/revisions/review";
-            "?upload-id=123")
+        status_url = "/v1/charm/test-charm/revisions/review?upload-id=123"
         responses.add(
-            "GET", status_url,
+            "GET", "http://charmhub.example"; + status_url,
             json={
                 "revisions": [
                     {
@@ -452,11 +445,9 @@ class TestCharmhubClient(TestCaseWithFactory):
     def test_checkStatus_approved(self):
         self._setUpSecretStorage()
         build = self.makeUploadableCharmRecipeBuild()
-        status_url = (
-            "http://charmhub.example/v1/charm/test-charm/revisions/review";
-            "?upload-id=123")
+        status_url = "/v1/charm/test-charm/revisions/review?upload-id=123"
         responses.add(
-            "GET", status_url,
+            "GET", "http://charmhub.example"; + status_url,
             json={
                 "revisions": [
                     {
@@ -471,7 +462,7 @@ class TestCharmhubClient(TestCaseWithFactory):
         requests = [call.request for call in responses.calls]
         self.assertThat(requests, MatchesListwise([
             RequestMatches(
-                url=Equals(status_url),
+                url=Equals("http://charmhub.example"; + status_url),
                 method=Equals("GET"),
                 auth=("Macaroon", MacaroonVerifies(self.exchanged_key))),
             ]))
@@ -480,10 +471,9 @@ class TestCharmhubClient(TestCaseWithFactory):
     def test_checkStatus_404(self):
         self._setUpSecretStorage()
         build = self.makeUploadableCharmRecipeBuild()
-        status_url = (
-            "http://charmhub.example/v1/charm/test-charm/revisions/review";
-            "?upload-id=123")
-        responses.add("GET", status_url, status=404)
+        status_url = "/v1/charm/test-charm/revisions/review?upload-id=123"
+        responses.add(
+            "GET", "http://charmhub.example"; + status_url, status=404)
         self.assertRaisesWithContent(
             BadReviewStatusResponse, "404 Client Error: Not Found",
             self.client.checkStatus, build, status_url)
diff --git a/lib/lp/charms/tests/test_charmrecipebuildjob.py b/lib/lp/charms/tests/test_charmrecipebuildjob.py
index 6c7aaa9..9bc020d 100644
--- a/lib/lp/charms/tests/test_charmrecipebuildjob.py
+++ b/lib/lp/charms/tests/test_charmrecipebuildjob.py
@@ -110,9 +110,7 @@ class TestCharmhubUploadJob(TestCaseWithFactory):
             CHARM_RECIPE_ALLOW_CREATE: "on",
             CHARM_RECIPE_WEBHOOKS_FEATURE_FLAG: "on",
             }))
-        self.status_url = (
-            "http://charmhub.example/v1/charm/test-charm/revisions/review";
-            "?upload-id=123")
+        self.status_url = "/v1/charm/test-charm/revisions/review?upload-id=123"
 
     def test_provides_interface(self):
         # `CharmhubUploadJob` objects provide `ICharmhubUploadJob`.