Merge ppa-dev-tools:error-handling-cleanup into ppa-dev-tools:main

[Bryce Harrington]

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.

[Athos Ribeiro (athos-ribeiro)]

Thanks, Bryce.

This LGTM.

I left a small comment nitpick regarding moving a magic number to a variable so it is more readable.

review: Approve

[Bryce Harrington (bryce)]

Thanks Athos, that's a good idea. I've added it defined as EX_KEYBOARD_INTERRUPT. In fact while adding it I spotted a couple other places for it.

$ git merge --ff-only error-handling-cleanup
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://git.launchpad.net/ppa-dev-tools
   1a457e7..03b736c main -> main

diff --git a/ppa/ppa.py b/ppa/ppa.py
index bb28c21..5fd8056 100755
--- a/ppa/ppa.py
+++ b/ppa/ppa.py
@@ -35,7 +35,7 @@ class PendingReason(enum.Enum):
     BUILD_MISSING = enum.auto()  # Build was expected, but missing
 
 
-class PpaDoesNotExist(BaseException):
+class PpaNotFoundError(Exception):
     """Exception indicating a requested PPA could not be found."""
 
     def __init__(self, ppa_name, owner_name, message=None):
@@ -124,7 +124,7 @@ class Ppa:
 
         :rtype: archive
         :returns: The Launchpad archive object.
-        :raises PpaDoesNotExist: Raised if a PPA does not exist in Launchpad.
+        :raises PpaNotFoundError: Raised if a PPA does not exist in Launchpad.
         """
         if not self._service:
             raise AttributeError("Ppa object not connected to the Launchpad service")
@@ -132,7 +132,7 @@ class Ppa:
             owner = self._service.people[self.owner_name]
             return owner.getPPAByName(name=self.ppa_name)
         except NotFound:
-            raise PpaDoesNotExist(self.ppa_name, self.owner_name)
+            raise PpaNotFoundError(self.ppa_name, self.owner_name)
 
     @lru_cache
     def exists(self) -> bool:
@@ -140,7 +140,7 @@ class Ppa:
         try:
             self.archive
             return True
-        except PpaDoesNotExist:
+        except PpaNotFoundError:
             return False
 
     @property
@@ -189,7 +189,7 @@ class Ppa:
         self.ppa_description = description
         try:
             archive = self.archive
-        except PpaDoesNotExist as e:
+        except PpaNotFoundError as e:
             print(e)
             return False
         archive.description = description
@@ -243,7 +243,7 @@ class Ppa:
         """
         try:
             return [proc.name for proc in self.archive.processors]
-        except PpaDoesNotExist as e:
+        except PpaNotFoundError as e:
             sys.stderr.write(e)
             return None
 
@@ -268,7 +268,7 @@ class Ppa:
         try:
             self.archive.setProcessors(processors=procs)
             return True
-        except PpaDoesNotExist as e:
+        except PpaNotFoundError as e:
             sys.stderr.write(e)
             return False
 
@@ -345,7 +345,7 @@ class Ppa:
                     self.archive.getPublishedBinaries(
                         created_since_date=created_since_date, status="Published",
                         binary_name=name))
-            except PpaDoesNotExist as e:
+            except PpaNotFoundError as e:
                 print(e)
                 return None
         # elif series:
@@ -386,7 +386,7 @@ class Ppa:
                     created_since_date=created_since_date,
                     status="Published",
                     source_name=name))
-        except PpaDoesNotExist as e:
+        except PpaNotFoundError as e:
             print(e)
             return None
 
@@ -403,7 +403,7 @@ class Ppa:
         """
         try:
             return self.archive.lp_delete()
-        except PpaDoesNotExist as e:
+        except PpaNotFoundError as e:
             print(e)
             return True
         except BadRequest:
diff --git a/scripts/ppa b/scripts/ppa
index 76c32c5..2d2937e 100755
--- a/scripts/ppa
+++ b/scripts/ppa
@@ -82,7 +82,7 @@ from ppa.ppa import (
     get_ppa,
     ppa_address_split,
     Ppa,
-    PpaDoesNotExist,
+    PpaNotFoundError,
     PendingReason
 )
 from ppa.ppa_group import PpaGroup, PpaAlreadyExists
@@ -94,6 +94,8 @@ from ppa.trigger import get_triggers, show_triggers
 import ppa.debug
 from ppa.debug import dbg, warn, error
 
+EX_KEYBOARD_INTERRUPT = 130
+
 
 def UNIMPLEMENTED():
     """Marks functionality that's not yet been coded"""
@@ -482,6 +484,12 @@ def create_config(lp: Lp, args: argparse.Namespace) -> dict[str, Any]:
         # Assume defaults
         dbg("Using default config since no config file found at {config_path}")
         config = dict(DEFAULT_CONFIG)
+    except OSError as err:
+        error(f"Could not open {config_path}: {str(err)}")
+        return None
+    except YAMLError as err:
+        error(f"Invalid config file {config_path}: {str(err)}")
+        return None
 
     # Map all non-empty elements from argparse Namespace into config dict
     config.update({k: v for k, v in vars(args).items() if v is not None})
@@ -538,6 +546,9 @@ def command_create(lp: Lp, config: dict[str, str]) -> int:
     :rtype: int
     :returns: Status code OK (0) on success, non-zero on error.
     """
+    if not lp:
+        return os.EX_UNAVAILABLE
+
     # Take description from stdin if it's not a tty
     description = config.get('description')
     if not description and not sys.stdin.isatty():
@@ -599,9 +610,9 @@ def command_create(lp: Lp, config: dict[str, str]) -> int:
         return os.EX_NOUSER
     except PpaAlreadyExists as e:
         warn(o2str(e.message))
-        return 3
+        return os.EX_CANTCREAT
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     print("Unhandled error")
     return 1
 
@@ -614,6 +625,9 @@ def command_credentials(lp: Lp, config: dict[str, str]) -> int:
     :rtype: int
     :returns: Status code OK (0) on success, non-zero on error.
     """
+    if not lp:
+        return os.EX_UNAVAILABLE
+
     try:
         credentials_filename = config.get(
             'credentials_filename',
@@ -623,7 +637,7 @@ def command_credentials(lp: Lp, config: dict[str, str]) -> int:
         print(f"Launchpad credentials written to {credentials_filename}")
         return os.EX_OK
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     print("Unhandled error")
     return 1
 
@@ -635,6 +649,9 @@ def command_desc(lp: Lp, config: dict[str, str]) -> int:
     :rtype: int
     :returns: Status code OK (0) on success, non-zero on error.
     """
+    if not lp:
+        return os.EX_UNAVAILABLE
+
     if not sys.stdin.isatty():
         description = sys.stdin.read()
     else:
@@ -652,7 +669,7 @@ def command_desc(lp: Lp, config: dict[str, str]) -> int:
 
         return the_ppa.set_description(description)
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     print("Unhandled error")
     return 1
 
@@ -665,6 +682,9 @@ def command_destroy(lp: Lp, config: dict[str, str]) -> int:
     :rtype: int
     :returns: Status code OK (0) on success, non-zero on error.
     """
+    if not lp:
+        return os.EX_UNAVAILABLE
+
     try:
         the_ppa = get_ppa(lp, config)
         if not config.get('dry_run'):
@@ -672,7 +692,7 @@ def command_destroy(lp: Lp, config: dict[str, str]) -> int:
             the_ppa.destroy()
         return os.EX_OK
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     print("Unhandled error")
     return 1
 
@@ -694,7 +714,7 @@ def command_list(lp: Lp, config: dict[str, str], filter_func=None) -> int:
     #  - filter_older: Ones older than a given date
     #  - Status of the PPAs
     if not lp:
-        return 1
+        return os.EX_UNAVAILABLE
 
     owner_name = config.get('owner_name')
     if not owner_name:
@@ -710,7 +730,7 @@ def command_list(lp: Lp, config: dict[str, str], filter_func=None) -> int:
             print(p.address)
         return os.EX_OK
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     print("Unhandled error")
     return 1
 
@@ -723,12 +743,15 @@ def command_exists(lp: Lp, config: dict[str, str]) -> int:
     :rtype: int
     :returns: Status code OK (0) on success, non-zero on error.
     """
+    if not lp:
+        return os.EX_UNAVAILABLE
+
     try:
         the_ppa = get_ppa(lp, config)
         if the_ppa.archive is not None:
             return os.EX_OK
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     return 1
 
 
@@ -740,6 +763,9 @@ def command_set(lp: Lp, config: dict[str, str]) -> int:
     :rtype: int
     :returns: Status code OK (0) on success, non-zero on error.
     """
+    if not lp:
+        return os.EX_UNAVAILABLE
+
     try:
         the_ppa = get_ppa(lp, config)
 
@@ -772,14 +798,14 @@ def command_set(lp: Lp, config: dict[str, str]) -> int:
         else:
             error(f"Insufficient authorization to modify PPA '{the_ppa.name}'.")
         return os.EX_NOPERM
-    except PpaDoesNotExist as e:
+    except PpaNotFoundError as e:
         print(e)
-        return 1
+        return os.EX_NOTFOUND
     except ValueError as e:
         print(f"Error: {e}")
         return os.EX_USAGE
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     print("Unhandled error")
     return 1
 
@@ -792,6 +818,9 @@ def command_show(lp: Lp, config: dict[str, str]) -> int:
     :rtype: int
     :returns: Status code OK (0) on success, non-zero on error.
     """
+    if not lp:
+        return os.EX_UNAVAILABLE
+
     distro = None
     series = None
     arch = None
@@ -833,11 +862,11 @@ def command_show(lp: Lp, config: dict[str, str]) -> int:
             total_downloads += binary.getDownloadCount()
         print("downloads: %d" % (total_downloads))
         return os.EX_OK
-    except PpaDoesNotExist as e:
+    except PpaNotFoundError as e:
         print(e)
-        return 1
+        return os.EX_NOTFOUND
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     print("Unhandled error")
     return 1
 
@@ -863,7 +892,7 @@ def command_wait(lp: Lp, config: dict[str, str]) -> int:
     :returns: Status code OK (0) on success, non-zero on error.
     """
     if not lp:
-        return 1
+        return os.EX_UNAVAILABLE
 
     try:
         wait_max_age_hours = config.get('wait_max_age_hours')
@@ -904,14 +933,14 @@ def command_wait(lp: Lp, config: dict[str, str]) -> int:
                     return os.EX_TEMPFAIL
             time.sleep(config['wait_seconds'])
         return os.EX_OK
-    except PpaDoesNotExist as e:
+    except PpaNotFoundError as e:
         print(e)
-        return 1
+        return os.EX_NOTFOUND
     except ValueError as e:
         print(f"Error: {e}")
         return os.EX_USAGE
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     print("Unhandled error")
     return 1
 
@@ -925,7 +954,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
     :returns: Status code OK (0) on success, non-zero on error.
     """
     if not lp:
-        return 1
+        return os.EX_UNAVAILABLE
 
     apt_repository = None
     if config.get("show_rdepends"):
@@ -934,7 +963,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
             apt_repository = Repository(cache_dir=local_dists_path)
         except FileNotFoundError as e:
             error(f'Missing checkout\n{LOCAL_REPOSITORY_MIRRORING_DIRECTIONS}: {e}')
-            return 1
+            return os.EX_NOTFOUND
 
     releases = config.get('releases', None)
     if releases is None:
@@ -956,7 +985,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
     the_ppa = get_ppa(lp, config)
     if not the_ppa.exists():
         error(f"PPA {the_ppa.name} does not exist for user {the_ppa.owner_name}")
-        return 1
+        return os.EX_NOTFOUND
 
     architectures = config.get('architectures', ARCHES_AUTOPKGTEST)
     if isinstance(architectures, str):
@@ -1035,7 +1064,7 @@ def command_tests(lp: Lp, config: dict[str, str]) -> int:
 
         return os.EX_OK
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     print("Unhandled error")
     return 1
 
@@ -1068,7 +1097,7 @@ def main(args: argparse.Namespace) -> int:
         lp = create_lp('ppa-dev-tools', args)
         config = create_config(lp, args)
     except KeyboardInterrupt:
-        return 2
+        return EX_KEYBOARD_INTERRUPT
     except ValueError as e:
         error(e)
         return os.EX_CONFIG
@@ -1086,7 +1115,7 @@ def main(args: argparse.Namespace) -> int:
         func, param = COMMANDS[command]
     except KeyError:
         parser.error(f"No such command {args.command}")
-        return 1
+        return os.EX_USAGE
     if param:
         return func(lp, config, param)
     return func(lp, config)
@@ -1101,6 +1130,6 @@ if __name__ == "__main__":
     if retval == os.EX_USAGE:
         print()
         parser.print_help()
-    elif retval == 2:
+    elif retval == EX_KEYBOARD_INTERRUPT:
         sys.stderr.write(" (user interrupt)\n")
     sys.exit(retval)