tickets/DM-42599: Improve command-line environment parsing

src/rubin_google_filestore_tools/cli.py

@@ -15,22 +15,19 @@ def _add_generic_options(
         "--project",
         help="Google project containing filestore",
         default=os.environ.get(f"{ENV_PREFIX}PROJECT"),
-        required=True,
     )
     parser.add_argument(
         "-z",
         "--zone",
         "--location",
         help="GCP zone containing filestore",
         default=os.environ.get(f"{ENV_PREFIX}ZONE"),
-        required=True,
     )
     parser.add_argument(
         "-i",
         "--instance",
         help="Filestore instance name",
         default=os.environ.get(f"{ENV_PREFIX}INSTANCE"),
-        required=True,
     )
     parser.add_argument(
         "-s",
@@ -42,6 +39,7 @@ def _add_generic_options(
         "-d",
         "--debug",
         "--verbose",
+        action="store_true",
         default=str_bool(os.environ.get(f"{ENV_PREFIX}DEBUG", "")),
         help="Verbose debugging output",
     )
@@ -78,6 +76,15 @@ def _parse_backup_args() -> argparse.Namespace:
     return parser.parse_args()
 
 
+def _validate_args(args: argparse.Namespace) -> None:
+    if not args.project:
+        raise ValueError("Project is required")
+    if not args.zone:
+        raise ValueError("Zone is required")
+    if not args.instance:
+        raise ValueError("Instance is required")
+
+
 def _backup_tool(args: argparse.Namespace) -> FilestoreTool:
     return FilestoreTool(
         project=args.project,
@@ -91,12 +98,16 @@ def _backup_tool(args: argparse.Namespace) -> FilestoreTool:
 def create_backup() -> None:
     """Create a backup."""
     args = _parse_backup_args()
+    _validate_args(args)
     tool = _backup_tool(args)
     tool.backup(prefix=args.prefix)
 
 
 def purge_backups() -> None:
     """Purge all but 'keep' backups."""
     args = _parse_backup_args()
+    _validate_args(args)
+    if args.keep < 1:
+        raise ValueError("Keep must be at least 1")
     tool = _backup_tool(args)
     tool.purge_backups(prefix=args.prefix, keep=args.keep)

tests/cli_test.py

@@ -1,10 +1,60 @@
 """Test the CLI."""
 
+import argparse
+import os
 import subprocess
 
+import rubin_google_filestore_tools
+import rubin_google_filestore_tools.cli
+import rubin_google_filestore_tools.util
+
+_SHORTENVS = (
+    "PROJECT",
+    "ZONE",
+    "INSTANCE",
+    "SHARE_NAME",
+    "DEBUG",
+    "BACKUP_PREFIX",
+    "KEEP_BACKUPS",
+)
+ENVVARS = [
+    f"{rubin_google_filestore_tools.util.ENV_PREFIX}{x}" for x in _SHORTENVS
+]
+
 
 def test_backup_cli() -> None:
     cp = subprocess.run("create_backup", check=False)
-    assert cp.returncode == 2  # It won't have the arguments
+    assert cp.returncode == 1  # It won't have the arguments
     cp = subprocess.run("purge_backups", check=False)
-    assert cp.returncode == 2
+    assert cp.returncode == 1
+
+
+def test_env_args() -> None:
+    envvals = {
+        "RUBIN_GOOGLE_FILESTORE_TOOLS_PROJECT": "myproject-4381",
+        "RUBIN_GOOGLE_FILESTORE_TOOLS_ZONE": "us-central1-a",
+        "RUBIN_GOOGLE_FILESTORE_TOOLS_INSTANCE": "myinstance",
+        "RUBIN_GOOGLE_FILESTORE_TOOLS_SHARE_NAME": "fake_share",
+        "RUBIN_GOOGLE_FILESTORE_TOOLS_KEEP_BACKUPS": "4",
+        "RUBIN_GOOGLE_FILESTORE_TOOLS_BACKUP_PREFIX": "w-bk-",
+        "RUBIN_GOOGLE_FILESTORE_TOOLS_DEBUG": "true",
+    }
+    for k in envvals:
+        os.environ[k] = envvals[k]
+    parser = argparse.ArgumentParser()
+    rubin_google_filestore_tools.cli._add_generic_options(parser)
+    rubin_google_filestore_tools.cli._add_backup_options(parser)
+    args = parser.parse_args([])
+    assert args.project == envvals["RUBIN_GOOGLE_FILESTORE_TOOLS_PROJECT"]
+    assert args.zone == envvals["RUBIN_GOOGLE_FILESTORE_TOOLS_ZONE"]
+    assert args.instance == envvals["RUBIN_GOOGLE_FILESTORE_TOOLS_INSTANCE"]
+    assert (
+        args.share_name == envvals["RUBIN_GOOGLE_FILESTORE_TOOLS_SHARE_NAME"]
+    )
+    assert args.keep == int(
+        envvals["RUBIN_GOOGLE_FILESTORE_TOOLS_KEEP_BACKUPS"]
+    )
+    assert args.debug
+    # Reset environment
+    for k in envvals:
+        del os.environ[k]