Merge ppa-dev-tools:error-handling-cleanup into ppa-dev-tools:main
- Git
- lp:ppa-dev-tools
- error-handling-cleanup
- Merge into main
Status: | Merged |
---|---|
Merged at revision: | 03b736c6a37efe9f7fd81238c7a6ec451d7e5018 |
Proposed branch: | ppa-dev-tools:error-handling-cleanup |
Merge into: | ppa-dev-tools:main |
Diff against target: |
385 lines (+65/-36) 2 files modified
ppa/ppa.py (+10/-10) scripts/ppa (+55/-26) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Athos Ribeiro (community) | Approve | ||
Canonical Server Reporter | Pending | ||
Review via email: mp+461580@code.launchpad.net |
Commit message
Description of the change
Several cleanups relating to exception handling and error exit codes.
These aren't fixing specific issues that have come up in practice, but rather were noticed in recent refactoring and code cleanup efforts on other branches. I think these are worth getting reviewed and landed first.
Bryce Harrington (bryce) wrote : | # |
Thanks Athos, that's a good idea. I've added it defined as EX_KEYBOARD_
$ git merge --ff-only error-handling-
Updating 1a457e7..03b736c
Fast-forward
ppa/ppa.py | 20 +++++++
scripts/ppa | 81 +++++++
2 files changed, 65 insertions(+), 36 deletions(-)
$ git push
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
To git+ssh:
1a457e7..03b736c main -> main
Preview Diff
1 | diff --git a/ppa/ppa.py b/ppa/ppa.py |
2 | index bb28c21..5fd8056 100755 |
3 | --- a/ppa/ppa.py |
4 | +++ b/ppa/ppa.py |
5 | @@ -35,7 +35,7 @@ class PendingReason(enum.Enum): |
6 | BUILD_MISSING = enum.auto() # Build was expected, but missing |
7 | |
8 | |
9 | -class PpaDoesNotExist(BaseException): |
10 | +class PpaNotFoundError(Exception): |
11 | """Exception indicating a requested PPA could not be found.""" |
12 | |
13 | def __init__(self, ppa_name, owner_name, message=None): |
14 | @@ -124,7 +124,7 @@ class Ppa: |
15 | |
16 | :rtype: archive |
17 | :returns: The Launchpad archive object. |
18 | - :raises PpaDoesNotExist: Raised if a PPA does not exist in Launchpad. |
19 | + :raises PpaNotFoundError: Raised if a PPA does not exist in Launchpad. |
20 | """ |
21 | if not self._service: |
22 | raise AttributeError("Ppa object not connected to the Launchpad service") |
23 | @@ -132,7 +132,7 @@ class Ppa: |
24 | owner = self._service.people[self.owner_name] |
25 | return owner.getPPAByName(name=self.ppa_name) |
26 | except NotFound: |
27 | - raise PpaDoesNotExist(self.ppa_name, self.owner_name) |
28 | + raise PpaNotFoundError(self.ppa_name, self.owner_name) |
29 | |
30 | @lru_cache |
31 | def exists(self) -> bool: |
32 | @@ -140,7 +140,7 @@ class Ppa: |
33 | try: |
34 | self.archive |
35 | return True |
36 | - except PpaDoesNotExist: |
37 | + except PpaNotFoundError: |
38 | return False |
39 | |
40 | @property |
41 | @@ -189,7 +189,7 @@ class Ppa: |
42 | self.ppa_description = description |
43 | try: |
44 | archive = self.archive |
45 | - except PpaDoesNotExist as e: |
46 | + except PpaNotFoundError as e: |
47 | print(e) |
48 | return False |
49 | archive.description = description |
50 | @@ -243,7 +243,7 @@ class Ppa: |
51 | """ |
52 | try: |
53 | return [proc.name for proc in self.archive.processors] |
54 | - except PpaDoesNotExist as e: |
55 | + except PpaNotFoundError as e: |
56 | sys.stderr.write(e) |
57 | return None |
58 | |
59 | @@ -268,7 +268,7 @@ class Ppa: |
60 | try: |
61 | self.archive.setProcessors(processors=procs) |
62 | return True |
63 | - except PpaDoesNotExist as e: |
64 | + except PpaNotFoundError as e: |
65 | sys.stderr.write(e) |
66 | return False |
67 | |
68 | @@ -345,7 +345,7 @@ class Ppa: |
69 | self.archive.getPublishedBinaries( |
70 | created_since_date=created_since_date, status="Published", |
71 | binary_name=name)) |
72 | - except PpaDoesNotExist as e: |
73 | + except PpaNotFoundError as e: |
74 | print(e) |
75 | return None |
76 | # elif series: |
77 | @@ -386,7 +386,7 @@ class Ppa: |
78 | created_since_date=created_since_date, |
79 | status="Published", |
80 | source_name=name)) |
81 | - except PpaDoesNotExist as e: |
82 | + except PpaNotFoundError as e: |
83 | print(e) |
84 | return None |
85 | |
86 | @@ -403,7 +403,7 @@ class Ppa: |
87 | """ |
88 | try: |
89 | return self.archive.lp_delete() |
90 | - except PpaDoesNotExist as e: |
91 | + except PpaNotFoundError as e: |
92 | print(e) |
93 | return True |
94 | except BadRequest: |
95 | diff --git a/scripts/ppa b/scripts/ppa |
96 | index 76c32c5..2d2937e 100755 |
97 | --- a/scripts/ppa |
98 | +++ b/scripts/ppa |
99 | @@ -82,7 +82,7 @@ from ppa.ppa import ( |
100 | get_ppa, |
101 | ppa_address_split, |
102 | Ppa, |
103 | - PpaDoesNotExist, |
104 | + PpaNotFoundError, |
105 | PendingReason |
106 | ) |
107 | from ppa.ppa_group import PpaGroup, PpaAlreadyExists |
108 | @@ -94,6 +94,8 @@ from ppa.trigger import get_triggers, show_triggers |
109 | import ppa.debug |
110 | from ppa.debug import dbg, warn, error |
111 | |
112 | +EX_KEYBOARD_INTERRUPT = 130 |
113 | + |
114 | |
115 | def UNIMPLEMENTED(): |
116 | """Marks functionality that's not yet been coded""" |
117 | @@ -482,6 +484,12 @@ def create_config(lp: Lp, args: argparse.Namespace) -> dict[str, Any]: |
118 | # Assume defaults |
119 | dbg("Using default config since no config file found at {config_path}") |
120 | config = dict(DEFAULT_CONFIG) |
121 | + except OSError as err: |
122 | + error(f"Could not open {config_path}: {str(err)}") |
123 | + return None |
124 | + except YAMLError as err: |
125 | + error(f"Invalid config file {config_path}: {str(err)}") |
126 | + return None |
127 | |
128 | # Map all non-empty elements from argparse Namespace into config dict |
129 | config.update({k: v for k, v in vars(args).items() if v is not None}) |
130 | @@ -538,6 +546,9 @@ def command_create(lp: Lp, config: dict[str, str]) -> int: |
131 | :rtype: int |
132 | :returns: Status code OK (0) on success, non-zero on error. |
133 | """ |
134 | + if not lp: |
135 | + return os.EX_UNAVAILABLE |
136 | + |
137 | # Take description from stdin if it's not a tty |
138 | description = config.get('description') |
139 | if not description and not sys.stdin.isatty(): |
140 | @@ -599,9 +610,9 @@ def command_create(lp: Lp, config: dict[str, str]) -> int: |
141 | return os.EX_NOUSER |
142 | except PpaAlreadyExists as e: |
143 | warn(o2str(e.message)) |
144 | - return 3 |
145 | + return os.EX_CANTCREAT |
146 | except KeyboardInterrupt: |
147 | - return 2 |
148 | + return EX_KEYBOARD_INTERRUPT |
149 | print("Unhandled error") |
150 | return 1 |
151 | |
152 | @@ -614,6 +625,9 @@ def command_credentials(lp: Lp, config: dict[str, str]) -> int: |
153 | :rtype: int |
154 | :returns: Status code OK (0) on success, non-zero on error. |
155 | """ |
156 | + if not lp: |
157 | + return os.EX_UNAVAILABLE |
158 | + |
159 | try: |
160 | credentials_filename = config.get( |
161 | 'credentials_filename', |
162 | @@ -623,7 +637,7 @@ def command_credentials(lp: Lp, config: dict[str, str]) -> int: |
163 | print(f"Launchpad credentials written to {credentials_filename}") |
164 | return os.EX_OK |
165 | except KeyboardInterrupt: |
166 | - return 2 |
167 | + return EX_KEYBOARD_INTERRUPT |
168 | print("Unhandled error") |
169 | return 1 |
170 | |
171 | @@ -635,6 +649,9 @@ def command_desc(lp: Lp, config: dict[str, str]) -> int: |
172 | :rtype: int |
173 | :returns: Status code OK (0) on success, non-zero on error. |
174 | """ |
175 | + if not lp: |
176 | + return os.EX_UNAVAILABLE |
177 | + |
178 | if not sys.stdin.isatty(): |
179 | description = sys.stdin.read() |
180 | else: |
181 | @@ -652,7 +669,7 @@ def command_desc(lp: Lp, config: dict[str, str]) -> int: |
182 | |
183 | return the_ppa.set_description(description) |
184 | except KeyboardInterrupt: |
185 | - return 2 |
186 | + return EX_KEYBOARD_INTERRUPT |
187 | print("Unhandled error") |
188 | return 1 |
189 | |
190 | @@ -665,6 +682,9 @@ def command_destroy(lp: Lp, config: dict[str, str]) -> int: |
191 | :rtype: int |
192 | :returns: Status code OK (0) on success, non-zero on error. |
193 | """ |
194 | + if not lp: |
195 | + return os.EX_UNAVAILABLE |
196 | + |
197 | try: |
198 | the_ppa = get_ppa(lp, config) |
199 | if not config.get('dry_run'): |
200 | @@ -672,7 +692,7 @@ def command_destroy(lp: Lp, config: dict[str, str]) -> int: |
201 | the_ppa.destroy() |
202 | return os.EX_OK |
203 | except KeyboardInterrupt: |
204 | - return 2 |
205 | + return EX_KEYBOARD_INTERRUPT |
206 | print("Unhandled error") |
207 | return 1 |
208 | |
209 | @@ -694,7 +714,7 @@ def command_list(lp: Lp, config: dict[str, str], filter_func=None) -> int: |
210 | # - filter_older: Ones older than a given date |
211 | # - Status of the PPAs |
212 | if not lp: |
213 | - return 1 |
214 | + return os.EX_UNAVAILABLE |
215 | |
216 | owner_name = config.get('owner_name') |
217 | if not owner_name: |
218 | @@ -710,7 +730,7 @@ def command_list(lp: Lp, config: dict[str, str], filter_func=None) -> int: |
219 | print(p.address) |
220 | return os.EX_OK |
221 | except KeyboardInterrupt: |
222 | - return 2 |
223 | + return EX_KEYBOARD_INTERRUPT |
224 | print("Unhandled error") |
225 | return 1 |
226 | |
227 | @@ -723,12 +743,15 @@ def command_exists(lp: Lp, config: dict[str, str]) -> int: |
228 | :rtype: int |
229 | :returns: Status code OK (0) on success, non-zero on error. |
230 | """ |
231 | + if not lp: |
232 | + return os.EX_UNAVAILABLE |
233 | + |
234 | try: |
235 | the_ppa = get_ppa(lp, config) |
236 | if the_ppa.archive is not None: |
237 | return os.EX_OK |
238 | except KeyboardInterrupt: |
239 | - return 2 |
240 | + return EX_KEYBOARD_INTERRUPT |
241 | return 1 |
242 | |
243 | |
244 | @@ -740,6 +763,9 @@ def command_set(lp: Lp, config: dict[str, str]) -> int: |
245 | :rtype: int |
246 | :returns: Status code OK (0) on success, non-zero on error. |
247 | """ |
248 | + if not lp: |
249 | + return os.EX_UNAVAILABLE |
250 | + |
251 | try: |
252 | the_ppa = get_ppa(lp, config) |
253 | |
254 | @@ -772,14 +798,14 @@ def command_set(lp: Lp, config: dict[str, str]) -> int: |
255 | else: |
256 | error(f"Insufficient authorization to modify PPA '{the_ppa.name}'.") |
257 | return os.EX_NOPERM |
258 | - except PpaDoesNotExist as e: |
259 | + except PpaNotFoundError as e: |
260 | print(e) |
261 | - return 1 |
262 | + return os.EX_NOTFOUND |
263 | except ValueError as e: |
264 | print(f"Error: {e}") |
265 | return os.EX_USAGE |
266 | except KeyboardInterrupt: |
267 | - return 2 |
268 | + return EX_KEYBOARD_INTERRUPT |
269 | print("Unhandled error") |
270 | return 1 |
271 | |
272 | @@ -792,6 +818,9 @@ def command_show(lp: Lp, config: dict[str, str]) -> int: |
273 | :rtype: int |
274 | :returns: Status code OK (0) on success, non-zero on error. |
275 | """ |
276 | + if not lp: |
277 | + return os.EX_UNAVAILABLE |
278 | + |
279 | distro = None |
280 | series = None |
281 | arch = None |
282 | @@ -833,11 +862,11 @@ def command_show(lp: Lp, config: dict[str, str]) -> int: |
283 | total_downloads += binary.getDownloadCount() |
284 | print("downloads: %d" % (total_downloads)) |
285 | return os.EX_OK |
286 | - except PpaDoesNotExist as e: |
287 | + except PpaNotFoundError as e: |
288 | print(e) |
289 | - return 1 |
290 | + return os.EX_NOTFOUND |
291 | except KeyboardInterrupt: |
292 | - return 2 |
293 | + return EX_KEYBOARD_INTERRUPT |
294 | print("Unhandled error") |
295 | return 1 |
296 | |
297 | @@ -863,7 +892,7 @@ def command_wait(lp: Lp, config: dict[str, str]) -> int: |
298 | :returns: Status code OK (0) on success, non-zero on error. |
299 | """ |
300 | if not lp: |
301 | - return 1 |
302 | + return os.EX_UNAVAILABLE |
303 | |
304 | try: |
305 | wait_max_age_hours = config.get('wait_max_age_hours') |
306 | @@ -904,14 +933,14 @@ def command_wait(lp: Lp, config: dict[str, str]) -> int: |
307 | return os.EX_TEMPFAIL |
308 | time.sleep(config['wait_seconds']) |
309 | return os.EX_OK |
310 | - except PpaDoesNotExist as e: |
311 | + except PpaNotFoundError as e: |
312 | print(e) |
313 | - return 1 |
314 | + return os.EX_NOTFOUND |
315 | except ValueError as e: |
316 | print(f"Error: {e}") |
317 | return os.EX_USAGE |
318 | except KeyboardInterrupt: |
319 | - return 2 |
320 | + return EX_KEYBOARD_INTERRUPT |
321 | print("Unhandled error") |
322 | return 1 |
323 | |
324 | @@ -925,7 +954,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int: |
325 | :returns: Status code OK (0) on success, non-zero on error. |
326 | """ |
327 | if not lp: |
328 | - return 1 |
329 | + return os.EX_UNAVAILABLE |
330 | |
331 | apt_repository = None |
332 | if config.get("show_rdepends"): |
333 | @@ -934,7 +963,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int: |
334 | apt_repository = Repository(cache_dir=local_dists_path) |
335 | except FileNotFoundError as e: |
336 | error(f'Missing checkout\n{LOCAL_REPOSITORY_MIRRORING_DIRECTIONS}: {e}') |
337 | - return 1 |
338 | + return os.EX_NOTFOUND |
339 | |
340 | releases = config.get('releases', None) |
341 | if releases is None: |
342 | @@ -956,7 +985,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int: |
343 | the_ppa = get_ppa(lp, config) |
344 | if not the_ppa.exists(): |
345 | error(f"PPA {the_ppa.name} does not exist for user {the_ppa.owner_name}") |
346 | - return 1 |
347 | + return os.EX_NOTFOUND |
348 | |
349 | architectures = config.get('architectures', ARCHES_AUTOPKGTEST) |
350 | if isinstance(architectures, str): |
351 | @@ -1035,7 +1064,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int: |
352 | |
353 | return os.EX_OK |
354 | except KeyboardInterrupt: |
355 | - return 2 |
356 | + return EX_KEYBOARD_INTERRUPT |
357 | print("Unhandled error") |
358 | return 1 |
359 | |
360 | @@ -1068,7 +1097,7 @@ def main(args: argparse.Namespace) -> int: |
361 | lp = create_lp('ppa-dev-tools', args) |
362 | config = create_config(lp, args) |
363 | except KeyboardInterrupt: |
364 | - return 2 |
365 | + return EX_KEYBOARD_INTERRUPT |
366 | except ValueError as e: |
367 | error(e) |
368 | return os.EX_CONFIG |
369 | @@ -1086,7 +1115,7 @@ def main(args: argparse.Namespace) -> int: |
370 | func, param = COMMANDS[command] |
371 | except KeyError: |
372 | parser.error(f"No such command {args.command}") |
373 | - return 1 |
374 | + return os.EX_USAGE |
375 | if param: |
376 | return func(lp, config, param) |
377 | return func(lp, config) |
378 | @@ -1101,6 +1130,6 @@ if __name__ == "__main__": |
379 | if retval == os.EX_USAGE: |
380 | print() |
381 | parser.print_help() |
382 | - elif retval == 2: |
383 | + elif retval == EX_KEYBOARD_INTERRUPT: |
384 | sys.stderr.write(" (user interrupt)\n") |
385 | sys.exit(retval) |
Thanks, Bryce.
This LGTM.
I left a small comment nitpick regarding moving a magic number to a variable so it is more readable.