From ee9a7ac70e4dd37b1a0f08ca3c476384a36d5227 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Tue, 5 Dec 2023 16:40:12 -0600 Subject: [PATCH] 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 <39996+kurtmckee@users.noreply.github.com> --- ...04_165730_sirosen_introduce_gcs_command.md | 4 +-- src/globus_cli/endpointish/errors.py | 30 ++++++++--------- .../collection/test_collection_delete.py | 17 ++++------ .../collection/test_collection_list.py | 17 ++++------ .../collection/test_collection_show.py | 13 +++----- .../collection/test_collection_update.py | 17 ++++------ tests/functional/test_gcs_aliasing.py | 33 ++++++------------- 7 files changed, 52 insertions(+), 79 deletions(-) diff --git a/changelog.d/20231204_165730_sirosen_introduce_gcs_command.md b/changelog.d/20231204_165730_sirosen_introduce_gcs_command.md index 2e08acc63..539a54860 100644 --- a/changelog.d/20231204_165730_sirosen_introduce_gcs_command.md +++ b/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. diff --git a/src/globus_cli/endpointish/errors.py b/src/globus_cli/endpointish/errors.py index f2a8d9a51..ca09c31de 100644 --- a/src/globus_cli/endpointish/errors.py +++ b/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 diff --git a/tests/functional/collection/test_collection_delete.py b/tests/functional/collection/test_collection_delete.py index 2a9c0047a..94e88facb 100644 --- a/tests/functional/collection/test_collection_delete.py +++ b/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" diff --git a/tests/functional/collection/test_collection_list.py b/tests/functional/collection/test_collection_list.py index de37b921e..84ef09321 100644 --- a/tests/functional/collection/test_collection_list.py +++ b/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"] diff --git a/tests/functional/collection/test_collection_show.py b/tests/functional/collection/test_collection_show.py index 440c13fce..7bf3bf96e 100644 --- a/tests/functional/collection/test_collection_show.py +++ b/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}'." diff --git a/tests/functional/collection/test_collection_update.py b/tests/functional/collection/test_collection_update.py index 197e0dbd6..8bbbfb804 100644 --- a/tests/functional/collection/test_collection_update.py +++ b/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 diff --git a/tests/functional/test_gcs_aliasing.py b/tests/functional/test_gcs_aliasing.py index 076d9678e..9bc3eb6e4 100644 --- a/tests/functional/test_gcs_aliasing.py +++ b/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