Implement review feedback

- ShouldUse.command is renamed to ShouldUse.dst_command
- Always write ShouldUse params in the same order
- Improvements to test parametrization

Co-authored-by: Kurt McKee <[email protected]>

changelog.d/20231204_165730_sirosen_introduce_gcs_command.md

@@ -1,6 +1,4 @@
 ### Enhancements
 
 * Introduce a new command, `globus gcs`, for GCSv5 Collection, Storage Gateway, and
-  User Credential management. `globus gcs` will eventually replace several existing
-  commands like `globus endpoint user-credential`, but for the time being both
-  variants are available.
+  User Credential management.

src/globus_cli/endpointish/errors.py

@@ -7,59 +7,59 @@
 
 @dataclasses.dataclass
 class ShouldUse:
-    command: str
     if_types: tuple[EntityType, ...]
     src_commands: tuple[str, ...]
+    dst_command: str
 
 
 # listed in precedence order; matching uses `if_types`+`src_commands`
 SHOULD_USE_MAPPINGS = (
     # update [gcp]
     ShouldUse(
-        command="globus gcp update mapped",
         if_types=(EntityType.GCP_MAPPED,),
         src_commands=("globus collection update", "globus gcs collection update"),
+        dst_command="globus gcp update mapped",
     ),
     ShouldUse(
-        command="globus gcp update guest",
         if_types=(EntityType.GCP_GUEST,),
         src_commands=("globus collection update", "globus gcs collection update"),
+        dst_command="globus gcp update guest",
     ),
     # update [gcsv4 endpoints (host/share)]
     ShouldUse(
-        command="globus endpoint update",
         if_types=EntityType.traditional_endpoints(),
         src_commands=("globus collection update", "globus gcs collection update"),
+        dst_command="globus endpoint update",
     ),
     # update [gcsv5 collections]
     ShouldUse(
-        command="globus gcs collection update",
-        src_commands=("globus endpoint update",),
         if_types=EntityType.gcsv5_collections(),
+        src_commands=("globus endpoint update",),
+        dst_command="globus gcs collection update",
     ),
     # delete [gcsv4 endpoints (host/share)]
     ShouldUse(
-        command="globus endpoint delete",
-        src_commands=("globus collection delete", "globus gcs collection delete"),
         if_types=EntityType.traditional_endpoints(),
+        src_commands=("globus collection delete", "globus gcs collection delete"),
+        dst_command="globus endpoint delete",
     ),
     # delete [gcsv5 collections]
     ShouldUse(
-        command="globus gcs collection delete",
-        src_commands=("globus endpoint delete",),
         if_types=EntityType.gcsv5_collections(),
+        src_commands=("globus endpoint delete",),
+        dst_command="globus gcs collection delete",
     ),
     # show [gcsv4 endpoints (host/share) + gcp + gcsv5 endpoint]
     ShouldUse(
-        command="globus endpoint show",
-        src_commands=("globus collection show", "globus gcs collection show"),
         if_types=EntityType.non_gcsv5_collection_types(),
+        src_commands=("globus collection show", "globus gcs collection show"),
+        dst_command="globus endpoint show",
     ),
     # show [gcsv5 collections]
     ShouldUse(
-        command="globus gcs collection show",
-        src_commands=("globus endpoint show",),
         if_types=EntityType.gcsv5_collections(),
+        src_commands=("globus endpoint show",),
+        dst_command="globus gcs collection show",
     ),
 )
 
@@ -98,7 +98,7 @@ def should_use_command(self) -> str | None:
                 self.from_command in should_use_data.src_commands
                 and self.actual_type in should_use_data.if_types
             ):
-                return should_use_data.command
+                return should_use_data.dst_command
         return None
 
 

tests/functional/collection/test_collection_delete.py

@@ -3,12 +3,9 @@
 
 
 # toggle between the (newer) 'gcs' variant and the 'bare' variant
-@pytest.fixture(params=(True, False), ids=("gcs-collection", "collection"))
+@pytest.fixture(params=("gcs collection", "collection"))
 def base_command(request):
-    if request.param:
-        return ["globus", "gcs", "collection", "delete"]
-    else:
-        return ["globus", "collection", "delete"]
+    return f"globus {request.param} delete"
 
 
 def test_guest_collection_delete(run_line, add_gcs_login, base_command):
@@ -17,7 +14,7 @@ def test_guest_collection_delete(run_line, add_gcs_login, base_command):
     cid = meta["guest_collection_id"]
     add_gcs_login(epid)
 
-    result = run_line(base_command + [cid])
+    result = run_line(f"{base_command} {cid}")
     assert "success" in result.output
 
 
@@ -27,7 +24,7 @@ def test_mapped_collection_delete(run_line, add_gcs_login, base_command):
     cid = meta["mapped_collection_id"]
     add_gcs_login(epid)
 
-    result = run_line(base_command + [cid])
+    result = run_line(f"{base_command} {cid}")
     assert "success" in result.output
 
 
@@ -36,7 +33,7 @@ def test_collection_delete_missing_login(run_line, base_command):
     epid = meta["endpoint_id"]
     cid = meta["guest_collection_id"]
 
-    result = run_line(base_command + [cid], assert_exit_code=4)
+    result = run_line(f"{base_command} {cid}", assert_exit_code=4)
     assert "success" not in result.output
     assert f"Missing login for {epid}" in result.stderr
     assert f"  globus login --gcs {epid}" in result.stderr
@@ -46,7 +43,7 @@ def test_collection_delete_on_gcsv5_host(run_line, base_command):
     meta = load_response_set("cli.collection_operations").metadata
     epid = meta["endpoint_id"]
 
-    result = run_line(base_command + [epid], assert_exit_code=3)
+    result = run_line(f"{base_command} {epid}", assert_exit_code=3)
     assert "success" not in result.output
     assert (
         f"Expected {epid} to be a collection ID.\n"
@@ -59,7 +56,7 @@ def test_collection_delete_on_gcp(run_line, base_command):
     meta = load_response_set("cli.collection_operations").metadata
     epid = meta["gcp_endpoint_id"]
 
-    result = run_line(base_command + [epid], assert_exit_code=3)
+    result = run_line(f"{base_command} {epid}", assert_exit_code=3)
     assert "success" not in result.output
     assert (
         f"Expected {epid} to be a collection ID.\n"

tests/functional/collection/test_collection_list.py

@@ -4,12 +4,9 @@
 
 
 # toggle between the (newer) 'gcs' variant and the 'bare' variant
-@pytest.fixture(params=(True, False), ids=("gcs-collection", "collection"))
+@pytest.fixture(params=("gcs collection", "collection"))
 def base_command(request):
-    if request.param:
-        return ["globus", "gcs", "collection", "list"]
-    else:
-        return ["globus", "collection", "list"]
+    return f"globus {request.param} list"
 
 
 def get_last_gcs_call(gcs_addr):
@@ -27,7 +24,7 @@ def test_collection_list(run_line, add_gcs_login, base_command):
     meta = load_response_set("cli.collection_operations").metadata
     epid = meta["endpoint_id"]
     add_gcs_login(epid)
-    result = run_line(base_command + [epid])
+    result = run_line(f"{base_command} {epid}")
     collection_names = ["Happy Fun Collection Name 1", "Happy Fun Collection Name 2"]
     for name in collection_names:
         assert name in result.stdout
@@ -38,7 +35,7 @@ def test_collection_list_opts(run_line, add_gcs_login, base_command):
     epid = meta["endpoint_id"]
     add_gcs_login(epid)
     cid = meta["mapped_collection_id"]
-    run_line(base_command + ["--mapped-collection-id", cid, epid])
+    run_line(f"{base_command} --mapped-collection-id {cid} {epid}")
 
     gcs_addr = meta["gcs_hostname"]
     last_call = get_last_gcs_call(gcs_addr)
@@ -53,7 +50,7 @@ def test_collection_list_on_gcp(run_line, base_command):
     meta = load_response_set("cli.collection_operations").metadata
     epid = meta["gcp_endpoint_id"]
 
-    result = run_line(base_command + [epid], assert_exit_code=3)
+    result = run_line(f"{base_command} {epid}", assert_exit_code=3)
     assert "success" not in result.output
     assert (
         f"Expected {epid} to be a Globus Connect Server v5 Endpoint.\n"
@@ -66,7 +63,7 @@ def test_collection_list_on_mapped_collection(run_line, base_command):
     meta = load_response_set("cli.collection_operations").metadata
     epid = meta["mapped_collection_id"]
 
-    result = run_line(base_command + [epid], assert_exit_code=3)
+    result = run_line(f"{base_command} {epid}", assert_exit_code=3)
     assert "success" not in result.output
     assert (
         f"Expected {epid} to be a Globus Connect Server v5 Endpoint.\n"
@@ -97,7 +94,7 @@ def test_collection_list_filters(run_line, add_gcs_login, filter_val, base_comma
     for f in filter_val:
         filters.extend(["--filter", f])
 
-    run_line(base_command + filters + [epid])
+    run_line(base_command.split() + filters + [epid])
     filter_params = {v.lower().replace("-", "_") for v in filter_val}
 
     gcs_addr = meta["gcs_hostname"]

tests/functional/collection/test_collection_show.py

@@ -3,12 +3,9 @@
 
 
 # toggle between the (newer) 'gcs' variant and the 'bare' variant
-@pytest.fixture(params=(True, False), ids=("gcs-collection", "collection"))
+@pytest.fixture(params=("gcs collection", "collection"))
 def base_command(request):
-    if request.param:
-        return ["globus", "gcs", "collection", "show"]
-    else:
-        return ["globus", "collection", "show"]
+    return f"globus {request.param} show"
 
 
 def test_collection_show(run_line, add_gcs_login, base_command):
@@ -19,7 +16,7 @@ def test_collection_show(run_line, add_gcs_login, base_command):
     add_gcs_login(epid)
 
     run_line(
-        base_command + [cid],
+        f"{base_command} {cid}",
         search_stdout=[
             ("Display Name", "Happy Fun Collection Name"),
             ("Owner", username),
@@ -38,7 +35,7 @@ def test_collection_show_private_policies(run_line, add_gcs_login, base_command)
     add_gcs_login(epid)
 
     run_line(
-        base_command + ["--include-private-policies", cid],
+        f"{base_command} --include-private-policies {cid}",
         search_stdout=[
             ("Display Name", "Happy Fun Collection Name"),
             ("Owner", username),
@@ -65,7 +62,7 @@ def test_collection_show_on_non_collection(run_line, base_command, epid_key, ep_
     meta = load_response_set("cli.collection_operations").metadata
     epid = meta[epid_key]
 
-    result = run_line(base_command + [epid], assert_exit_code=3)
+    result = run_line(f"{base_command} {epid}", assert_exit_code=3)
     assert (
         f"Expected {epid} to be a collection ID.\n"
         f"Instead, found it was of type '{ep_type}'."

tests/functional/collection/test_collection_update.py

@@ -6,12 +6,9 @@
 
 
 # toggle between the (newer) 'gcs' variant and the 'bare' variant
-@pytest.fixture(params=(True, False), ids=("gcs-collection", "collection"))
+@pytest.fixture(params=("gcs collection", "collection"))
 def base_command(request):
-    if request.param:
-        return ["globus", "gcs", "collection", "update"]
-    else:
-        return ["globus", "collection", "update"]
+    return f"globus {request.param} update"
 
 
 @pytest.mark.parametrize("cid_key", ["mapped_collection_id", "guest_collection_id"])
@@ -28,7 +25,7 @@ def test_collection_update(run_line, add_gcs_login, base_command, cid_key):
         ep_type_specific_opts = ["--sharing-user-allow", ""]
 
     result = run_line(
-        base_command
+        base_command.split()
         + [
             cid,
             "--description",
@@ -67,7 +64,7 @@ def test_collection_update_verify_opts(
     epid = meta["endpoint_id"]
     add_gcs_login(epid)
 
-    result = run_line(base_command + [cid, "--verify", verify_str])
+    result = run_line(f"{base_command} {cid} --verify {verify_str}")
     assert "success" in result.output
 
     sent = json.loads(responses.calls[-1].request.body)
@@ -80,7 +77,7 @@ def test_collection_update_on_gcp(run_line, base_command):
     meta = load_response_set("cli.collection_operations").metadata
     epid = meta["gcp_endpoint_id"]
 
-    result = run_line(base_command + [epid, "--description", "foo"], assert_exit_code=3)
+    result = run_line(f"{base_command} {epid} --description foo", assert_exit_code=3)
     assert (
         f"Expected {epid} to be a collection ID.\n"
         "Instead, found it was of type 'Globus Connect Personal Mapped Collection'."
@@ -95,7 +92,7 @@ def test_collection_update_on_gcsv5_host(run_line, base_command):
     meta = load_response_set("cli.collection_operations").metadata
     epid = meta["endpoint_id"]
 
-    result = run_line(base_command + [epid, "--description", "foo"], assert_exit_code=3)
+    result = run_line(f"{base_command} {epid} --description foo", assert_exit_code=3)
     assert "success" not in result.output
     assert (
         f"Expected {epid} to be a collection ID.\n"
@@ -113,7 +110,7 @@ def test_guest_collection_update_rejects_invalid_opts(
     add_gcs_login(epid)
 
     result = run_line(
-        base_command + [cid, "--sharing-user-allow", ""],
+        f"{base_command} {cid} --sharing-user-allow ''",
         assert_exit_code=2,
     )
     assert "success" not in result.output

tests/functional/test_gcs_aliasing.py

@@ -8,30 +8,17 @@
 
 
 @pytest.mark.parametrize(
-    "subcommand",
+    "from_command, to_command",
     (
-        ["collection", "delete"],
-        ["collection", "list"],
-        (
-            ["endpoint", "storage-gateway", "list"],
-            ["gcs", "storage-gateway", "list"],
-        ),
-        (
-            ["endpoint", "user-credential", "list"],
-            ["gcs", "user-credential", "list"],
-        ),
+        ("collection delete", "gcs collection delete"),
+        ("collection list", "gcs collection list"),
+        ("endpoint storage-gateway list", "gcs storage-gateway list"),
+        ("endpoint user-credential list", "gcs user-credential list"),
     ),
 )
-def test_aliased_commands_have_unique_usage_lines(run_line, subcommand):
-    if isinstance(subcommand, tuple):
-        from_command = subcommand[0]
-        to_command = subcommand[1]
-    else:
-        from_command = subcommand
-        to_command = ["gcs"] + subcommand
-
-    unaliased_help = run_line(["globus", *from_command, "--help"]).stdout
-    aliased_help = run_line(["globus", *to_command, "--help"]).stdout
+def test_aliased_commands_have_unique_usage_lines(run_line, from_command, to_command):
+    unaliased_help = run_line(f"globus {from_command} --help").stdout
+    aliased_help = run_line(f"globus {to_command} --help").stdout
 
     unaliased_usage_line = unaliased_help.splitlines()[0]
     aliased_usage_line = aliased_help.splitlines()[0]
@@ -40,5 +27,5 @@ def test_aliased_commands_have_unique_usage_lines(run_line, subcommand):
     # - they aren't the same
     assert unaliased_usage_line != aliased_usage_line
     # - they each start correctly
-    assert " ".join(["globus", *from_command]) in unaliased_usage_line
-    assert " ".join(["globus", *to_command]) in aliased_usage_line
+    assert f"globus {from_command}" in unaliased_usage_line
+    assert f"globus {to_command}" in aliased_usage_line