launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #21634
[Merge] lp:~cjwatson/launchpad/snap-better-upload-failures into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-better-upload-failures into lp:launchpad.
Commit message:
Extract more useful error messages when snap store operations fail.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1650461 in Launchpad itself: "Unable to tell what's wrong when fails to upload snap to the store"
https://bugs.launchpad.net/launchpad/+bug/1650461
Bug #1687068 in Launchpad itself: "uGet is not publishing to the snap store after building successfully"
https://bugs.launchpad.net/launchpad/+bug/1687068
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-better-upload-failures/+merge/325492
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-better-upload-failures into lp:launchpad.
=== modified file 'lib/lp/snappy/interfaces/snapstoreclient.py'
--- lib/lp/snappy/interfaces/snapstoreclient.py 2017-04-18 17:54:03 +0000
+++ lib/lp/snappy/interfaces/snapstoreclient.py 2017-06-12 13:13:33 +0000
@@ -8,17 +8,16 @@
__metaclass__ = type
__all__ = [
'BadRefreshResponse',
- 'BadReleaseResponse',
'BadRequestPackageUploadResponse',
'BadScanStatusResponse',
'BadSearchResponse',
- 'BadUploadResponse',
'ISnapStoreClient',
'NeedsRefreshResponse',
'ReleaseFailedResponse',
'ScanFailedResponse',
'SnapStoreError',
'UnauthorizedUploadResponse',
+ 'UploadFailedResponse',
'UploadNotScannedYetResponse',
]
@@ -41,7 +40,7 @@
pass
-class BadUploadResponse(SnapStoreError):
+class UploadFailedResponse(SnapStoreError):
pass
@@ -73,10 +72,6 @@
pass
-class BadReleaseResponse(SnapStoreError):
- pass
-
-
class ReleaseFailedResponse(SnapStoreError):
pass
@@ -103,6 +98,8 @@
:param snapbuild: The `ISnapBuild` to upload.
:return: A URL to poll for upload processing status.
+ :raises UploadFailedResponse: if uploading the build to the store
+ failed.
"""
def refreshDischargeMacaroon(snap):
@@ -139,6 +136,6 @@
:param snapbuild: The `ISnapBuild` to release.
:param revision: The revision returned by the store when uploading
the build.
- :raises BadReleaseResponse: if the store failed to release the
+ :raises ReleaseFailedResponse: if the store failed to release the
build.
"""
=== modified file 'lib/lp/snappy/model/snapbuildjob.py'
--- lib/lp/snappy/model/snapbuildjob.py 2017-04-19 12:32:42 +0000
+++ lib/lp/snappy/model/snapbuildjob.py 2017-06-12 13:13:33 +0000
@@ -55,14 +55,13 @@
ISnapStoreUploadJobSource,
)
from lp.snappy.interfaces.snapstoreclient import (
- BadReleaseResponse,
BadScanStatusResponse,
- BadUploadResponse,
ISnapStoreClient,
ReleaseFailedResponse,
ScanFailedResponse,
SnapStoreError,
UnauthorizedUploadResponse,
+ UploadFailedResponse,
UploadNotScannedYetResponse,
)
from lp.snappy.mail.snapbuild import SnapBuildMailer
@@ -306,7 +305,7 @@
if isinstance(e, UnauthorizedUploadResponse):
mailer = SnapBuildMailer.forUnauthorizedUpload(self.snapbuild)
mailer.sendAll()
- elif isinstance(e, BadUploadResponse):
+ elif isinstance(e, UploadFailedResponse):
mailer = SnapBuildMailer.forUploadFailure(self.snapbuild)
mailer.sendAll()
elif isinstance(e, (BadScanStatusResponse, ScanFailedResponse)):
@@ -315,7 +314,7 @@
elif isinstance(e, ManualReview):
mailer = SnapBuildMailer.forManualReview(self.snapbuild)
mailer.sendAll()
- elif isinstance(e, (BadReleaseResponse, ReleaseFailedResponse)):
+ elif isinstance(e, ReleaseFailedResponse):
mailer = SnapBuildMailer.forReleaseFailure(self.snapbuild)
mailer.sendAll()
# The normal job infrastructure will abort the transaction, but
=== modified file 'lib/lp/snappy/model/snapstoreclient.py'
--- lib/lp/snappy/model/snapstoreclient.py 2017-04-18 17:54:03 +0000
+++ lib/lp/snappy/model/snapstoreclient.py 2017-06-12 13:13:33 +0000
@@ -36,16 +36,15 @@
from lp.services.webapp.url import urlappend
from lp.snappy.interfaces.snapstoreclient import (
BadRefreshResponse,
- BadReleaseResponse,
BadRequestPackageUploadResponse,
BadScanStatusResponse,
BadSearchResponse,
- BadUploadResponse,
ISnapStoreClient,
NeedsRefreshResponse,
ReleaseFailedResponse,
ScanFailedResponse,
UnauthorizedUploadResponse,
+ UploadFailedResponse,
UploadNotScannedYetResponse,
)
@@ -137,7 +136,26 @@
return None
return get_request_timeline(request)
- def requestPackageUploadPermission(self, snappy_series, snap_name):
+ @classmethod
+ def _makeSnapStoreError(cls, error_class, requests_error):
+ error_message = requests_error.args[0]
+ if requests_error.response.content:
+ try:
+ response_data = requests_error.response.json()
+ except JSONDecodeError:
+ pass
+ else:
+ if "error_list" in response_data:
+ error_message = "\n".join(
+ error["message"]
+ for error in response_data["error_list"])
+ detail = requests_error.response.content
+ if isinstance(detail, bytes):
+ detail = detail.decode("UTF-8", errors="replace")
+ return error_class(error_message, detail=detail)
+
+ @classmethod
+ def requestPackageUploadPermission(cls, snappy_series, snap_name):
assert config.snappy.store_url is not None
request_url = urlappend(config.snappy.store_url, "dev/api/acl/")
request = get_current_browser_request()
@@ -157,11 +175,12 @@
raise BadRequestPackageUploadResponse(response.text)
return response_data["macaroon"]
except requests.HTTPError as e:
- raise BadRequestPackageUploadResponse(e.args[0])
+ raise cls._makeSnapStoreError(BadRequestPackageUploadResponse, e)
finally:
timeline_action.finish()
- def _uploadFile(self, lfa, lfc):
+ @classmethod
+ def _uploadFile(cls, lfa, lfc):
"""Upload a single file."""
assert config.snappy.store_upload_url is not None
unscanned_upload_url = urlappend(
@@ -185,20 +204,15 @@
})
response_data = response.json()
if not response_data.get("successful", False):
- raise BadUploadResponse(response.text)
+ raise UploadFailedResponse(response.text)
return {"upload_id": response_data["upload_id"]}
except requests.HTTPError as e:
- if e.response is not None:
- detail = e.response.content
- if isinstance(detail, bytes):
- detail = detail.decode("UTF-8", errors="replace")
- else:
- detail = None
- raise BadUploadResponse(e.args[0], detail=detail)
+ raise cls._makeSnapStoreError(UploadFailedResponse, e)
finally:
lfa.close()
- def _uploadApp(self, snap, upload_data):
+ @classmethod
+ def _uploadApp(cls, snap, upload_data):
"""Create a new store upload based on the uploaded file."""
assert config.snappy.store_url is not None
assert snap.store_name is not None
@@ -226,22 +240,23 @@
raise NeedsRefreshResponse()
else:
raise UnauthorizedUploadResponse("Authorization failed.")
- raise BadUploadResponse(e.args[0])
+ raise cls._makeSnapStoreError(UploadFailedResponse, e)
- def upload(self, snapbuild):
+ @classmethod
+ def upload(cls, snapbuild):
"""See `ISnapStoreClient`."""
assert snapbuild.snap.can_upload_to_store
for _, lfa, lfc in snapbuild.getFiles():
if not lfa.filename.endswith(".snap"):
continue
- upload_data = self._uploadFile(lfa, lfc)
+ upload_data = cls._uploadFile(lfa, lfc)
try:
- return self._uploadApp(snapbuild.snap, upload_data)
+ return cls._uploadApp(snapbuild.snap, upload_data)
except NeedsRefreshResponse:
# Try to automatically refresh the discharge macaroon and
# retry the upload.
- self.refreshDischargeMacaroon(snapbuild.snap)
- return self._uploadApp(snapbuild.snap, upload_data)
+ cls.refreshDischargeMacaroon(snapbuild.snap)
+ return cls._uploadApp(snapbuild.snap, upload_data)
@classmethod
def refreshDischargeMacaroon(cls, snap):
@@ -261,7 +276,7 @@
new_secrets["discharge"] = response_data["discharge_macaroon"]
snap.store_secrets = new_secrets
except requests.HTTPError as e:
- raise BadRefreshResponse(e.args[0])
+ raise cls._makeSnapStoreError(BadRefreshResponse, e)
@classmethod
def checkStatus(cls, status_url):
@@ -280,7 +295,7 @@
else:
return response_data["url"], response_data["revision"]
except requests.HTTPError as e:
- raise BadScanStatusResponse(e.args[0])
+ raise cls._makeSnapStoreError(BadScanStatusResponse, e)
@classmethod
def listChannels(cls):
@@ -311,7 +326,7 @@
response = urlfetch(
channels_url, headers={"Accept": "application/hal+json"})
except requests.HTTPError as e:
- raise BadSearchResponse(e.args[0])
+ raise cls._makeSnapStoreError(BadSearchResponse, e)
finally:
if timeline is not None:
action.finish()
@@ -351,12 +366,4 @@
snap.store_secrets["root"],
snap.store_secrets.get("discharge")))
except requests.HTTPError as e:
- if e.response is not None:
- error = None
- try:
- error = e.response.json()["errors"]
- except Exception:
- pass
- if error is not None:
- raise ReleaseFailedResponse(error)
- raise BadReleaseResponse(e.args[0])
+ raise cls._makeSnapStoreError(ReleaseFailedResponse, e)
=== modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
--- lib/lp/snappy/tests/test_snapbuildjob.py 2017-04-19 12:32:42 +0000
+++ lib/lp/snappy/tests/test_snapbuildjob.py 2017-06-12 13:13:33 +0000
@@ -28,11 +28,11 @@
ISnapStoreUploadJob,
)
from lp.snappy.interfaces.snapstoreclient import (
- BadReleaseResponse,
BadScanStatusResponse,
- BadUploadResponse,
ISnapStoreClient,
+ ReleaseFailedResponse,
UnauthorizedUploadResponse,
+ UploadFailedResponse,
UploadNotScannedYetResponse,
)
from lp.snappy.model.snapbuildjob import (
@@ -241,7 +241,7 @@
self.assertContentEqual([], snapbuild.store_upload_jobs)
job = SnapStoreUploadJob.create(snapbuild)
client = FakeSnapStoreClient()
- client.upload.failure = BadUploadResponse(
+ client.upload.failure = UploadFailedResponse(
"Failed to upload", detail="The proxy exploded.\n")
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
@@ -463,7 +463,7 @@
client = FakeSnapStoreClient()
client.upload.result = self.status_url
client.checkStatus.result = (self.store_url, 1)
- client.release.failure = BadReleaseResponse("Failed to publish")
+ client.release.failure = ReleaseFailedResponse("Failed to publish")
self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
JobRunner([job]).runAll()
=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py 2017-04-18 17:54:03 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py 2017-06-12 13:13:33 +0000
@@ -44,15 +44,14 @@
from lp.services.timeline.requesttimeline import get_request_timeline
from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS
from lp.snappy.interfaces.snapstoreclient import (
- BadReleaseResponse,
BadRequestPackageUploadResponse,
BadScanStatusResponse,
BadSearchResponse,
- BadUploadResponse,
ISnapStoreClient,
ReleaseFailedResponse,
ScanFailedResponse,
UnauthorizedUploadResponse,
+ UploadFailedResponse,
UploadNotScannedYetResponse,
)
from lp.snappy.model.snapstoreclient import (
@@ -311,6 +310,21 @@
self.client.requestPackageUploadPermission,
snappy_series, "test-snap")
+ def test_requestPackageUploadPermission_error(self):
+ @all_requests
+ def handler(url, request):
+ return {
+ "status_code": 503,
+ "content": {"error_list": [{"message": "Failed"}]},
+ }
+
+ snappy_series = self.factory.makeSnappySeries()
+ with HTTMock(handler):
+ self.assertRaisesWithContent(
+ BadRequestPackageUploadResponse, "Failed",
+ self.client.requestPackageUploadPermission,
+ snappy_series, "test-snap")
+
def test_requestPackageUploadPermission_404(self):
@all_requests
def handler(url, request):
@@ -446,6 +460,30 @@
store_secrets["discharge"],
snapbuild.snap.store_secrets["discharge"])
+ def test_upload_unsigned_agreement(self):
+ @urlmatch(path=r".*/snap-push/$")
+ def snap_push_handler(url, request):
+ self.snap_push_request = request
+ return {
+ "status_code": 403,
+ "content": {
+ "error_list": [
+ {"message": "Developer has not signed agreement."},
+ ],
+ },
+ }
+
+ store_secrets = self._make_store_secrets()
+ snapbuild = self.makeUploadableSnapBuild(store_secrets=store_secrets)
+ transaction.commit()
+ with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+ with HTTMock(self._unscanned_upload_handler, snap_push_handler,
+ self._macaroon_refresh_handler):
+ err = self.assertRaises(
+ UploadFailedResponse, self.client.upload, snapbuild)
+ self.assertEqual(
+ "Developer has not signed agreement.", str(err))
+
def test_upload_file_error(self):
@urlmatch(path=r".*/unscanned-upload/$")
def unscanned_upload_handler(url, request):
@@ -461,7 +499,7 @@
with dbuser(config.ISnapStoreUploadJobSource.dbuser):
with HTTMock(unscanned_upload_handler):
err = self.assertRaises(
- BadUploadResponse, self.client.upload, snapbuild)
+ UploadFailedResponse, self.client.upload, snapbuild)
self.assertEqual("502 Server Error: Proxy Error", str(err))
self.assertEqual(b"The proxy exploded.\n", err.detail)
@@ -667,7 +705,7 @@
def handler(url, request):
return {
"status_code": 503,
- "content": {"success": False, "errors": "Failed to publish"},
+ "content": {"error_list": [{"message": "Failed to publish"}]},
}
with HTTMock(self._channels_handler):
@@ -698,5 +736,5 @@
snapbuild = self.factory.makeSnapBuild(snap=snap)
with HTTMock(handler):
self.assertRaisesWithContent(
- BadReleaseResponse, b"404 Client Error: Not found",
+ ReleaseFailedResponse, b"404 Client Error: Not found",
self.client.release, snapbuild, 1)
Follow ups
