Merge ~andersson123/autopkgtest-cloud:exit_code_suite_autopkgtest-web into autopkgtest-cloud:master
- Git
- lp:~andersson123/autopkgtest-cloud
- exit_code_suite_autopkgtest-web
- Merge into master
Status: | Merged |
---|---|
Merged at revision: | c294f93456e2661ce104e396b25da3e3b1c471b1 |
Proposed branch: | ~andersson123/autopkgtest-cloud:exit_code_suite_autopkgtest-web |
Merge into: | autopkgtest-cloud:master |
Diff against target: |
343 lines (+128/-67) 3 files modified
charms/focal/autopkgtest-web/webcontrol/helpers/exceptions.py (+80/-0) charms/focal/autopkgtest-web/webcontrol/request/app.py (+6/-9) charms/focal/autopkgtest-web/webcontrol/request/submit.py (+42/-58) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Paride Legovini | Approve | ||
Brian Murray | Needs Information | ||
Review via email:
|
Commit message
Description of the change
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tim Andersson (andersson123) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brian Murray (brian-murray) wrote : | # |
This generally looks great but I have a couple of questions in line.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Paride Legovini (paride) : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tim Andersson (andersson123) wrote : | # |
Amended. Please take another look.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brian Murray (brian-murray) wrote : | # |
I noticed an issue which needs addressing (commented in-line) which would end up "fixing" bug 2028801.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tim Andersson (andersson123) wrote : | # |
I'm going to test this in staging now.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brian Murray (brian-murray) wrote : | # |
Has this been tested in staging now?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tim Andersson (andersson123) wrote : | # |
I can't remember how far I got with this, so I'll re-test now
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brian Murray (brian-murray) wrote : | # |
This seems to have failed some CI checks:
:: ************* Module request.submit
:: charms/
:: charms/
I also have one in line comment.
While we just talked about utilizing the unit testing in autopkgtest-web, I think waiting to merge this until we have unit testing for the changes would be appropriate.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tim Andersson (andersson123) wrote : | # |
Totally understandable, let's do that :)
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Brian Murray (brian-murray) wrote : | # |
I see a conflict in this branch due to the changes merge in https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tim Andersson (andersson123) wrote : | # |
Amended
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tim Andersson (andersson123) wrote : | # |
I just tested this again and I think it's good to go.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tim Andersson (andersson123) wrote : | # |
We're waiting for the unit testing MP to be merged before merging this. Once it's merged we'll add a unit test for this and then look to merge this one.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Paride Legovini (paride) wrote : | # |
We should definitely add tests for new features/bugfixes, but given that the unit test suite needs work, maybe we can land this and then concentrate on fixing that.
I'd prefer input validation commit to be in a separate MP. That should be easy to do.
I have an inline comment on the input validation comment, but overall this LGTM.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tim Andersson (andersson123) wrote : | # |
separate mp for input validation:
https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Paride Legovini (paride) wrote : | # |
This LGTM, but I'd like CI to pass, so let's not merge right away. I want to figure out why we're getting flaky CI now.
Preview Diff
1 | diff --git a/charms/focal/autopkgtest-web/webcontrol/helpers/exceptions.py b/charms/focal/autopkgtest-web/webcontrol/helpers/exceptions.py |
2 | new file mode 100644 |
3 | index 0000000..9f4f745 |
4 | --- /dev/null |
5 | +++ b/charms/focal/autopkgtest-web/webcontrol/helpers/exceptions.py |
6 | @@ -0,0 +1,80 @@ |
7 | +""" |
8 | +Http exceptions for autopkgtest-web |
9 | +""" |
10 | + |
11 | + |
12 | +class WebControlException(Exception): |
13 | + def __init__(self, message, exit_code): |
14 | + super().__init__(message) |
15 | + self._code = exit_code |
16 | + |
17 | + def exit_code(self): |
18 | + return self._code |
19 | + |
20 | + |
21 | +class RequestInQueue(WebControlException): |
22 | + def __init__(self, release, package, arch, triggers): |
23 | + super().__init__( |
24 | + "Test already queued:\nrelease: %s\npkg: %s\narch: %s\ntriggers: %s" |
25 | + % (release, package, arch, ",".join(triggers)), |
26 | + 403, |
27 | + ) |
28 | + |
29 | + |
30 | +class RequestRunning(WebControlException): |
31 | + def __init__(self, release, package, arch, triggers): |
32 | + super().__init__( |
33 | + "Test already running:\nrelease: %s\npkg: %s\narch: %s\ntriggers: %s" |
34 | + % (release, package, arch, ",".join(triggers)), |
35 | + 403, |
36 | + ) |
37 | + |
38 | + |
39 | +class BadRequest(WebControlException): |
40 | + def __init__(self, msg=None): |
41 | + if msg is None: |
42 | + super().__init__( |
43 | + "Bad request - unacceptable passed variables", 400 |
44 | + ) |
45 | + else: |
46 | + super().__init__(msg, 400) |
47 | + |
48 | + |
49 | +class Unauthorized(WebControlException): |
50 | + def __init__(self): |
51 | + super().__init__("Authorization failure", 401) |
52 | + |
53 | + |
54 | +class ForbiddenRequest(WebControlException): |
55 | + def __init__(self, package, trigger): |
56 | + super().__init__( |
57 | + ( |
58 | + "You are not allowed to upload %s or %s to Ubuntu, " |
59 | + + "thus you are not allowed to use this service." |
60 | + ) |
61 | + % (package, trigger), |
62 | + 403, |
63 | + ) |
64 | + |
65 | + |
66 | +class NotFound(WebControlException): |
67 | + def __init__(self, element_name, element, msg=None): |
68 | + if msg is None: |
69 | + super().__init__( |
70 | + "%s %s not found" % (element_name, element), |
71 | + 404, |
72 | + ) |
73 | + else: |
74 | + super().__init__( |
75 | + "%s %s %s" % (element_name, element, msg), |
76 | + 404, |
77 | + ) |
78 | + |
79 | + |
80 | +class TooManyRequests(WebControlException): |
81 | + def __init__(self, requester): |
82 | + super().__init__( |
83 | + "You, %s, have requested too many tests. Please try again later." |
84 | + % requester, |
85 | + 429, |
86 | + ) |
87 | diff --git a/charms/focal/autopkgtest-web/webcontrol/request/app.py b/charms/focal/autopkgtest-web/webcontrol/request/app.py |
88 | index 758f05e..73fb0dc 100644 |
89 | --- a/charms/focal/autopkgtest-web/webcontrol/request/app.py |
90 | +++ b/charms/focal/autopkgtest-web/webcontrol/request/app.py |
91 | @@ -8,8 +8,9 @@ from html import escape as _escape |
92 | |
93 | from flask import Flask, redirect, request, session |
94 | from flask_openid import OpenID |
95 | +from helpers.exceptions import WebControlException |
96 | from helpers.utils import setup_key |
97 | -from request.submit import DuplicateRequestException, Submit |
98 | +from request.submit import Submit |
99 | from werkzeug.middleware.proxy_fix import ProxyFix |
100 | |
101 | # map multiple GET vars to AMQP JSON request parameter list |
102 | @@ -184,12 +185,10 @@ def index_root(): |
103 | github_params["number"], |
104 | ) |
105 | s.validate_git_request(**params) |
106 | - except (ValueError, TypeError) as e: |
107 | - return invalid(e) |
108 | + except WebControlException as e: |
109 | + return invalid(e, e.exit_code()) |
110 | except KeyError as e: |
111 | return invalid("Missing field in JSON data: %s" % e) |
112 | - except DuplicateRequestException as e: |
113 | - return invalid(e, 403) |
114 | |
115 | s.send_amqp_request(context="upstream", **params) |
116 | # write status file for pending test |
117 | @@ -237,10 +236,8 @@ def index_root(): |
118 | s = Submit() |
119 | try: |
120 | s.validate_distro_request(**params) |
121 | - except (ValueError, TypeError) as e: |
122 | - return invalid(e) |
123 | - except DuplicateRequestException as e: |
124 | - return invalid(e, 403) |
125 | + except WebControlException as e: |
126 | + return invalid(e, e.exit_code()) |
127 | |
128 | if params.get("delete"): |
129 | del params["delete"] |
130 | diff --git a/charms/focal/autopkgtest-web/webcontrol/request/submit.py b/charms/focal/autopkgtest-web/webcontrol/request/submit.py |
131 | index f0eca73..79018ea 100644 |
132 | --- a/charms/focal/autopkgtest-web/webcontrol/request/submit.py |
133 | +++ b/charms/focal/autopkgtest-web/webcontrol/request/submit.py |
134 | @@ -17,11 +17,13 @@ from urllib.error import HTTPError |
135 | |
136 | import amqplib.client_0_8 as amqp |
137 | from distro_info import UbuntuDistroInfo |
138 | - |
139 | - |
140 | -class DuplicateRequestException(Exception): |
141 | - pass |
142 | - |
143 | +from helpers.exceptions import ( |
144 | + BadRequest, |
145 | + ForbiddenRequest, |
146 | + NotFound, |
147 | + RequestInQueue, |
148 | + RequestRunning, |
149 | +) |
150 | |
151 | # Launchpad REST API base |
152 | LP = "https://api.launchpad.net/1.0/" |
153 | @@ -94,11 +96,7 @@ class Submit: |
154 | Raise ValueError with error messsage if the request is invalid, |
155 | otherwise return. |
156 | """ |
157 | - not_needed, msg = self.is_request_queued_or_running( |
158 | - release, arch, package, triggers, kwargs |
159 | - ) |
160 | - if not_needed: |
161 | - raise DuplicateRequestException(msg) |
162 | + self.is_request_queued_or_running(release, arch, package, triggers) |
163 | |
164 | can_upload_any_trigger = False |
165 | |
166 | @@ -125,12 +123,12 @@ class Submit: |
167 | raise ValueError("Invalid argument %s" % list(kwargs)[0]) |
168 | |
169 | if release not in self.releases: |
170 | - raise ValueError("Unknown release " + release) |
171 | + raise NotFound("release", release) |
172 | if arch not in self.architectures: |
173 | - raise ValueError("Unknown architecture " + arch) |
174 | + raise NotFound("arch", arch) |
175 | for ppa in ppas: |
176 | if not self.is_valid_ppa(ppa): |
177 | - raise ValueError("Unknown PPA " + ppa) |
178 | + raise NotFound("ppa", ppa) |
179 | # allow kernel tests for EOL vivid |
180 | skip_result_check = ( |
181 | release == "vivid" and triggers and triggers[0].startswith("linux") |
182 | @@ -138,32 +136,33 @@ class Submit: |
183 | if not self.is_valid_package_with_results( |
184 | None if (ppas or skip_result_check) else release, arch, package |
185 | ): |
186 | - raise ValueError( |
187 | - "Package %s does not have any test results for %s on %s" |
188 | - % (package, release, arch) |
189 | + raise NotFound( |
190 | + "package", package, "does not have any test results" |
191 | ) |
192 | |
193 | if "migration-reference/0" in triggers: |
194 | if len(triggers) != 1: |
195 | - raise ValueError( |
196 | + raise BadRequest( |
197 | "Cannot use additional triggers with migration-reference/0" |
198 | ) |
199 | if ppas: |
200 | - raise ValueError("Cannot use PPAs with migration-reference/0") |
201 | + raise BadRequest("Cannot use PPAs with migration-reference/0") |
202 | if "all-proposed" in kwargs: |
203 | - raise ValueError( |
204 | + raise BadRequest( |
205 | 'Cannot use "all-proposed" with migration-reference/0' |
206 | ) |
207 | for trigger in triggers: |
208 | try: |
209 | trigsrc, trigver = trigger.split("/") |
210 | - except ValueError as exc: |
211 | - raise ValueError( |
212 | + except ValueError as e: |
213 | + raise BadRequest( |
214 | "Malformed trigger, must be srcpackage/version" |
215 | - ) from exc |
216 | + ) from e |
217 | # Debian Policy 5.6.1 and 5.6.12 |
218 | if not NAME.match(trigsrc) or not VERSION.match(trigver): |
219 | - raise ValueError("Malformed trigger") |
220 | + raise BadRequest( |
221 | + "Malformed trigger: %s\nversion: %s" % (trigsrc, trigver) |
222 | + ) |
223 | |
224 | # Special snowflake |
225 | if trigger in ("qemu-efi-noacpi/0", "migration-reference/0"): |
226 | @@ -173,7 +172,7 @@ class Submit: |
227 | if not self.is_valid_package_version( |
228 | release, trigsrc, trigver, ppas and ppas[-1] or None |
229 | ): |
230 | - raise ValueError( |
231 | + raise BadRequest( |
232 | "%s is not published in PPA %s %s" |
233 | % (trigger, ppas[-1], release) |
234 | ) |
235 | @@ -188,7 +187,7 @@ class Submit: |
236 | release, trigsrc, trigver |
237 | ) |
238 | if not trigsrc_component: |
239 | - raise ValueError( |
240 | + raise BadRequest( |
241 | "%s is not published in %s" % (trigger, release) |
242 | ) |
243 | |
244 | @@ -205,7 +204,7 @@ class Submit: |
245 | release, package, None |
246 | ) |
247 | if not package_component: |
248 | - raise ValueError( |
249 | + raise BadRequest( |
250 | "%s is not published in %s" % (package, release) |
251 | ) |
252 | |
253 | @@ -216,11 +215,7 @@ class Submit: |
254 | and requester not in ALLOWED_USERS_PERPACKAGE.get(package, []) |
255 | and not self.in_allowed_team(requester, package) |
256 | ): |
257 | - raise ValueError( |
258 | - "You are not allowed to upload %s or %s to " |
259 | - "Ubuntu, thus you are not allowed to use this " |
260 | - "service." % (package, trigsrc) |
261 | - ) |
262 | + raise ForbiddenRequest(package, ",".join(triggers)) |
263 | |
264 | # pylint: disable=dangerous-default-value |
265 | def validate_git_request( |
266 | @@ -246,40 +241,36 @@ class Submit: |
267 | else: |
268 | triggers.append(env_var.split("=")[1]) |
269 | |
270 | - not_needed, msg = self.is_request_queued_or_running( |
271 | - release, arch, package, triggers, kwargs |
272 | - ) |
273 | - if not_needed: |
274 | - raise DuplicateRequestException(msg) |
275 | + self.is_request_queued_or_running(release, arch, package, triggers) |
276 | |
277 | if release not in self.releases: |
278 | - raise ValueError("Unknown release " + release) |
279 | + raise NotFound("release", release) |
280 | if arch not in self.architectures: |
281 | - raise ValueError("Unknown architecture " + arch) |
282 | + raise NotFound("arch", arch) |
283 | if not NAME.match(package): |
284 | - raise ValueError("Malformed package") |
285 | + raise NotFound("package", package) |
286 | if not ppas: |
287 | - raise ValueError( |
288 | + raise BadRequest( |
289 | "Must specify at least one PPA (to associate results with)" |
290 | ) |
291 | for ppa in ppas: |
292 | if not self.is_valid_ppa(ppa): |
293 | - raise ValueError("Unknown PPA " + ppa) |
294 | + raise NotFound("ppa", ppa) |
295 | for e in env: |
296 | if not ENV.match(e): |
297 | - raise ValueError( |
298 | + raise BadRequest( |
299 | 'Invalid environment variable format "%s"' % e |
300 | ) |
301 | # we should only be called in this mode |
302 | assert "build-git" in kwargs |
303 | if not GIT.match(kwargs["build-git"]): |
304 | - raise ValueError("Malformed build-git") |
305 | + raise BadRequest("Malformed build-git") |
306 | if "testname" in kwargs and not NAME.match(kwargs["testname"]): |
307 | - raise ValueError("Malformed testname") |
308 | + raise BadRequest("Malformed testname") |
309 | |
310 | unsupported_keys = set(kwargs.keys()) - {"build-git", "testname"} |
311 | if unsupported_keys: |
312 | - raise ValueError( |
313 | + raise BadRequest( |
314 | "Unsupported arguments: %s" % " ".join(unsupported_keys) |
315 | ) |
316 | |
317 | @@ -634,20 +625,13 @@ class Submit: |
318 | if self.is_test_running( |
319 | req_series, req_arch, req_package, req_triggers, kwargs |
320 | ): |
321 | - o_msg = "Requested test is already running." |
322 | - o_msg += "\nRelease: " + req_series |
323 | - o_msg += "\nArchitecture: " + req_arch |
324 | - o_msg += "\nPackage: " + req_package |
325 | - o_msg += "\nTriggers: " + ",".join(req_triggers) |
326 | - return True, o_msg |
327 | + raise RequestRunning( |
328 | + req_series, req_package, req_arch, req_triggers |
329 | + ) |
330 | |
331 | if self.is_test_in_queue( |
332 | req_series, req_arch, req_package, req_triggers, kwargs |
333 | ): |
334 | - o_msg = "Requested test is already queued." |
335 | - o_msg += "\nRelease: " + req_series |
336 | - o_msg += "\nArchitecture: " + req_arch |
337 | - o_msg += "\nPackage: " + req_package |
338 | - o_msg += "\nTriggers: " + ",".join(req_triggers) |
339 | - return True, o_msg |
340 | - return False, "" |
341 | + raise RequestInQueue( |
342 | + req_series, req_package, req_arch, req_triggers |
343 | + ) |
Tested in staging and works!