canonical-ubuntu-qa team mailing list archive

[Tim Andersson <mp+447647@xxxxxxxxxxxxxxxxxx>]

Tim Andersson has proposed merging ~andersson123/autopkgtest-cloud:exit_code_suite_autopkgtest-web into autopkgtest-cloud:master.

Requested reviews:
  Canonical's Ubuntu QA (canonical-ubuntu-qa)

For more details, see:
https://code.launchpad.net/~andersson123/autopkgtest-cloud/+git/autopkgtest-cloud/+merge/447647
-- 
Your team Canonical's Ubuntu QA is requested to review the proposed merge of ~andersson123/autopkgtest-cloud:exit_code_suite_autopkgtest-web into autopkgtest-cloud:master.

diff --git a/charms/focal/autopkgtest-web/webcontrol/helpers/exceptions.py b/charms/focal/autopkgtest-web/webcontrol/helpers/exceptions.py
new file mode 100644
index 0000000..302ea26
--- /dev/null
+++ b/charms/focal/autopkgtest-web/webcontrol/helpers/exceptions.py
@@ -0,0 +1,81 @@
+"""
+Http exceptions for autopkgtest-web
+"""
+
+
+class WebControlException(Exception):
+    def __init__(self, message, exit_code):
+        super().__init__(message)
+        self._code = exit_code
+
+    def exit_code(self):
+        return self._code
+
+
+class DuplicateRequestException(WebControlException):
+    def __init__(self, release, package, arch, triggers):
+        super().__init__(
+            (
+                "Test already queued:\nrelease: %s\npkg: %s\narch: %s\ntriggers: %s",
+                release,
+                package,
+                arch,
+                ",".join(triggers),
+            ),
+            429,
+        )
+
+
+class BadRequest(WebControlException):
+    def __init__(self, msg=None):
+        if msg is None:
+            super().__init__(
+                "Bad request - unnacceptable passed variables", 400
+            )
+        else:
+            super().__init__(msg, 400)
+
+
+class Unauthorized(WebControlException):
+    def __init__(self):
+        super().__init__("Authorization failure", 401)
+
+
+class ForbiddenRequest(WebControlException):
+    def __init__(self, package, trigger):
+        super().__init__(
+            (
+                "You are not allowed to upload %s or %s to Ubuntu, "
+                + "thus you are not allowed to use this service."
+            )
+            % (package, trigger),
+            403,
+        )
+
+
+class NotFound(WebControlException):
+    def __init__(self, element_name, element, msg=None):
+        if msg is None:
+            super().__init__(
+                "%s %s not found" % (element_name, element),
+                404,
+            )
+        else:
+            super().__init__(
+                "%s %s %s" % (element_name, element, msg),
+                404,
+            )
+
+
+class TooManyRequests(WebControlException):
+    def __init__(self, requester):
+        super().__init__(
+            "You, %s, have requested too many tests. Please try again later."
+            % requester,
+            429,
+        )
+
+
+class Teapot(WebControlException):
+    def __init__(self):
+        super().__init__("Sorry, I'm a teapot", 418)
diff --git a/charms/focal/autopkgtest-web/webcontrol/request/app.py b/charms/focal/autopkgtest-web/webcontrol/request/app.py
index 8ad5270..73fb0dc 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/app.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/app.py
@@ -8,8 +8,9 @@ from html import escape as _escape
 
 from flask import Flask, redirect, request, session
 from flask_openid import OpenID
+from helpers.exceptions import WebControlException
 from helpers.utils import setup_key
-from request.submit import DuplicateRequestException, Submit
+from request.submit import Submit
 from werkzeug.middleware.proxy_fix import ProxyFix
 
 # map multiple GET vars to AMQP JSON request parameter list
@@ -184,8 +185,8 @@ def index_root():
                     github_params["number"],
                 )
             s.validate_git_request(**params)
-        except (ValueError, TypeError) as e:
-            return invalid(e)
+        except WebControlException as e:
+            return invalid(e, e.exit_code())
         except KeyError as e:
             return invalid("Missing field in JSON data: %s" % e)
 
@@ -235,10 +236,8 @@ def index_root():
         s = Submit()
         try:
             s.validate_distro_request(**params)
-        except (ValueError, TypeError) as e:
-            return invalid(e)
-        except DuplicateRequestException as e:
-            return invalid(e, 403)
+        except WebControlException as e:
+            return invalid(e, e.exit_code())
 
         if params.get("delete"):
             del params["delete"]
diff --git a/charms/focal/autopkgtest-web/webcontrol/request/submit.py b/charms/focal/autopkgtest-web/webcontrol/request/submit.py
index aeeb9a9..b4f654a 100644
--- a/charms/focal/autopkgtest-web/webcontrol/request/submit.py
+++ b/charms/focal/autopkgtest-web/webcontrol/request/submit.py
@@ -17,11 +17,12 @@ from urllib.error import HTTPError
 
 import amqplib.client_0_8 as amqp
 from distro_info import UbuntuDistroInfo
-
-
-class DuplicateRequestException(Exception):
-    pass
-
+from helpers.exceptions import (  # TooManyRequests,; Unauthorized,
+    BadRequest,
+    DuplicateRequestException,
+    ForbiddenRequest,
+    NotFound,
+)
 
 # Launchpad REST API base
 LP = "https://api.launchpad.net/1.0/";
@@ -87,11 +88,11 @@ class Submit:
         Raise ValueError with error messsage if the request is invalid,
         otherwise return.
         """
-        not_needed, msg = self.is_request_queued_or_running(
+        not_needed = self.is_request_queued_or_running(
             release, arch, package, triggers
         )
         if not_needed:
-            raise DuplicateRequestException(msg)
+            raise DuplicateRequestException(release, package, arch, triggers)
 
         can_upload_any_trigger = False
 
@@ -118,12 +119,12 @@ class Submit:
             raise ValueError("Invalid argument %s" % list(kwargs)[0])
 
         if release not in self.releases:
-            raise ValueError("Unknown release " + release)
+            raise NotFound("release", release)
         if arch not in self.architectures:
-            raise ValueError("Unknown architecture " + arch)
+            raise NotFound("arch", arch)
         for ppa in ppas:
             if not self.is_valid_ppa(ppa):
-                raise ValueError("Unknown PPA " + ppa)
+                raise NotFound("ppa", ppa)
         # allow kernel tests for EOL vivid
         skip_result_check = (
             release == "vivid" and triggers and triggers[0].startswith("linux")
@@ -131,31 +132,33 @@ class Submit:
         if not self.is_valid_package_with_results(
             None if (ppas or skip_result_check) else release, arch, package
         ):
-            raise ValueError(
-                "Package %s does not have any test results" % package
+            raise NotFound(
+                "package", package, "does not have any test results"
             )
 
         if "migration-reference/0" in triggers:
             if len(triggers) != 1:
-                raise ValueError(
+                raise BadRequest(
                     "Cannot use additional triggers with migration-reference/0"
                 )
             if ppas:
-                raise ValueError("Cannot use PPAs with migration-reference/0")
+                raise BadRequest("Cannot use PPAs with migration-reference/0")
             if "all-proposed" in kwargs:
-                raise ValueError(
+                raise BadRequest(
                     'Cannot use "all-proposed" with migration-reference/0'
                 )
         for trigger in triggers:
             try:
                 trigsrc, trigver = trigger.split("/")
-            except ValueError as exc:
-                raise ValueError(
+            except ValueError as e:
+                raise BadRequest(
                     "Malformed trigger, must be srcpackage/version"
-                ) from exc
+                ) from e
             # Debian Policy 5.6.1 and 5.6.12
             if not NAME.match(trigsrc) or not VERSION.match(trigver):
-                raise ValueError("Malformed trigger")
+                raise BadRequest(
+                    "Malformed trigger: %s\nversion: %s" % (trigsrc, trigver)
+                )
 
             # Special snowflake
             if trigger in ("qemu-efi-noacpi/0", "migration-reference/0"):
@@ -165,7 +168,7 @@ class Submit:
                 if not self.is_valid_package_version(
                     release, trigsrc, trigver, ppas and ppas[-1] or None
                 ):
-                    raise ValueError(
+                    raise BadRequest(
                         "%s is not published in PPA %s %s"
                         % (trigger, ppas[-1], release)
                     )
@@ -180,7 +183,7 @@ class Submit:
                     release, trigsrc, trigver
                 )
                 if not trigsrc_component:
-                    raise ValueError(
+                    raise BadRequest(
                         "%s is not published in %s" % (trigger, release)
                     )
 
@@ -197,7 +200,7 @@ class Submit:
                 release, package, None
             )
             if not package_component:
-                raise ValueError(
+                raise BadRequest(
                     "%s is not published in %s" % (package, release)
                 )
 
@@ -208,11 +211,7 @@ class Submit:
             and requester not in ALLOWED_USERS_PERPACKAGE.get(package, [])
             and not self.in_allowed_team(requester, package)
         ):
-            raise ValueError(
-                "You are not allowed to upload %s or %s to "
-                "Ubuntu, thus you are not allowed to use this "
-                "service." % (package, trigsrc)
-            )
+            raise ForbiddenRequest(package, ",".join(triggers))
 
     # pylint: disable=dangerous-default-value
     def validate_git_request(
@@ -238,40 +237,41 @@ class Submit:
                 else:
                     triggers.append(env_var.split("=")[1])
 
-        not_needed, msg = self.is_request_queued_or_running(
+        not_needed = self.is_request_queued_or_running(
             release, arch, package, triggers
         )
         if not_needed:
-            raise DuplicateRequestException(msg)
+            raise DuplicateRequestException(release, package, arch, triggers)
 
         if release not in self.releases:
-            raise ValueError("Unknown release " + release)
+            raise NotFound("release", release)
         if arch not in self.architectures:
-            raise ValueError("Unknown architecture " + arch)
+            raise NotFound("arch", arch)
         if not NAME.match(package):
-            raise ValueError("Malformed package")
+            raise NotFound("package", package)
+            # raise ValueError("Malformed package")
         if not ppas:
-            raise ValueError(
+            raise BadRequest(
                 "Must specify at least one PPA (to associate results with)"
             )
         for ppa in ppas:
             if not self.is_valid_ppa(ppa):
-                raise ValueError("Unknown PPA " + ppa)
+                raise NotFound("ppa", ppa)
         for e in env:
             if not ENV.match(e):
-                raise ValueError(
+                raise BadRequest(
                     'Invalid environment variable format "%s"' % e
                 )
         # we should only be called in this mode
         assert "build-git" in kwargs
         if not GIT.match(kwargs["build-git"]):
-            raise ValueError("Malformed build-git")
+            raise BadRequest("Malformed build-git")
         if "testname" in kwargs and not NAME.match(kwargs["testname"]):
-            raise ValueError("Malformed testname")
+            raise BadRequest("Malformed testname")
 
         unsupported_keys = set(kwargs.keys()) - {"build-git", "testname"}
         if unsupported_keys:
-            raise ValueError(
+            raise BadRequest(
                 "Unsupported arguments: %s" % " ".join(unsupported_keys)
             )
 
@@ -607,20 +607,10 @@ class Submit:
         if self.is_test_running(
             req_series, req_arch, req_package, req_triggers
         ):
-            o_msg = "Requested test is already running."
-            o_msg += "\nRelease: " + req_series
-            o_msg += "\nArchitecture: " + req_arch
-            o_msg += "\nPackage: " + req_package
-            o_msg += "\nTriggers: " + ",".join(req_triggers)
-            return True, o_msg
+            return True
 
         if self.is_test_in_queue(
             req_series, req_arch, req_package, req_triggers
         ):
-            o_msg = "Requested test is already queued."
-            o_msg += "\nRelease: " + req_series
-            o_msg += "\nArchitecture: " + req_arch
-            o_msg += "\nPackage: " + req_package
-            o_msg += "\nTriggers: " + ",".join(req_triggers)
-            return True, o_msg
-        return False, ""
+            return True
+        return False