From 10dabc5e23f4edeec0e7d6df680475010883daa5 Mon Sep 17 00:00:00 2001 From: Marc LeBlanc <7050295+marcleblanc2@users.noreply.github.com> Date: Tue, 9 Jun 2026 18:23:22 -0600 Subject: [PATCH] Update CLI helper text arg order, and make set fail without args --- .gitignore | 1 + dev/test-cli-pypi-install.sh | 32 +++++++++++-- src/src_auth_perms_sync/cli.py | 84 ++++++++++++++++++++++++---------- tests/unit/test_cli_config.py | 61 +++++++++++++++++++++--- 4 files changed, 145 insertions(+), 33 deletions(-) diff --git a/.gitignore b/.gitignore index 996e666..3fe70d5 100644 --- a/.gitignore +++ b/.gitignore @@ -12,6 +12,7 @@ __pycache__ *.py[cod] *.py[oc] *.swp +*.tsv *.yaml build/ dist/ diff --git a/dev/test-cli-pypi-install.sh b/dev/test-cli-pypi-install.sh index 66e7664..fa1cb4e 100755 --- a/dev/test-cli-pypi-install.sh +++ b/dev/test-cli-pypi-install.sh @@ -1,24 +1,50 @@ #!/usr/bin/env bash # Description: Tests CLI mode install -set -euo pipefail +set -euox pipefail # Set the working directory -working_directory="${TMPDIR:-/tmp}/src-auth-perms-sync-pypi-install" +tmp_root="${TMPDIR:-/tmp}" +working_directory="${tmp_root%/}/src-auth-perms-sync-pypi-install" # Delete, recreate, and cd to working directory rm -rf "${working_directory}" && mkdir -p "${working_directory}" && cd "${working_directory}" +log_file="${working_directory}/test-cli-pypi-install.log" +exec > >(tee "${log_file}") 2>&1 +echo "Writing output to ${log_file}" +echo "" +echo "Dir contents in ${working_directory} before" +ls -al + # Use python3.13 to create and activate a venv # shellcheck disable=SC1091 +echo "" python3.13 -m venv .venv && source .venv/bin/activate +which python +python --version + +# Ensure pip is up to date +echo "" +python -m pip install --upgrade pip # pip install latest from https://pypi.org/project/src-auth-perms-sync -python3.13 -m pip install --upgrade pip src-auth-perms-sync +echo "" +python -m pip install src-auth-perms-sync # Run commands +echo "" src-auth-perms-sync --help +echo "" src-auth-perms-sync get --help +echo "" src-auth-perms-sync set --help +echo "" src-auth-perms-sync restore --help +echo "" src-auth-perms-sync sync-saml-orgs --help + +echo "" +echo "Dir contents in ${working_directory} after" +ls -al +echo "" diff --git a/src/src_auth_perms_sync/cli.py b/src/src_auth_perms_sync/cli.py index f3b63b2..4308303 100644 --- a/src/src_auth_perms_sync/cli.py +++ b/src/src_auth_perms_sync/cli.py @@ -33,8 +33,10 @@ CommandName: TypeAlias = Literal["get", "set", "restore", "sync_saml_orgs"] DEFAULT_MAPS_FILE_NAME = "maps.yaml" -COMMON_CONFIG_FIELDS = src.config_field_names( +COMMON_CONFIG_FIELDS_BEFORE = src.config_field_names( src.SourcegraphClientConfig, +) +COMMON_CONFIG_FIELDS_AFTER = src.config_field_names( src.LoggingConfig, src.OpenTelemetryConfig, "parallelism", @@ -44,6 +46,7 @@ "fetch_sg_traces", ) GET_CONFIG_FIELDS = src.config_field_names( + *COMMON_CONFIG_FIELDS_BEFORE, "users", "users_without_explicit_perms", "created_after", @@ -52,9 +55,10 @@ "repos_created_after", "no_backup", "explicit_permissions_batch_size", - *COMMON_CONFIG_FIELDS, + *COMMON_CONFIG_FIELDS_AFTER, ) SET_CONFIG_FIELDS = src.config_field_names( + *COMMON_CONFIG_FIELDS_BEFORE, "maps_path", "full", "users", @@ -67,19 +71,22 @@ "apply", "no_backup", "explicit_permissions_batch_size", - *COMMON_CONFIG_FIELDS, + *COMMON_CONFIG_FIELDS_AFTER, ) RESTORE_CONFIG_FIELDS = src.config_field_names( + *COMMON_CONFIG_FIELDS_BEFORE, "restore_path", "apply", "no_backup", "explicit_permissions_batch_size", - *COMMON_CONFIG_FIELDS, + *COMMON_CONFIG_FIELDS_AFTER, ) SYNC_SAML_ORGS_CONFIG_FIELDS = src.config_field_names( + *COMMON_CONFIG_FIELDS_BEFORE, "apply", "no_backup", - *COMMON_CONFIG_FIELDS, + "parallelism", + *COMMON_CONFIG_FIELDS_AFTER, ) LogCommandName: TypeAlias = Literal[ "get", @@ -187,9 +194,9 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo cli_flag="--maps-path", metavar="FILE", help=( - "Maps YAML file for the set command.\n" - "If omitted, set uses maps.yaml under src-auth-perms-sync-runs//.\n" - "Relative paths are resolved from the current working directory." + "Maps YAML file for the set command\n" + "(default: ./src-auth-perms-sync-runs//maps.yaml)\n" + "Relative paths are resolved from the current working directory" ), help_group="Permission sync", ) @@ -199,8 +206,8 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo cli_flag="--restore-path", metavar="FILE", help=( - "Snapshot JSON file for the restore command.\n" - "Relative paths are resolved from the current working directory." + "Snapshot JSON file for the restore command\n" + "Relative paths are resolved from the current working directory" ), help_group="Restore", ) @@ -210,8 +217,8 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo cli_flag="--full", cli_action="store_true", help=( - "With the set command: run full overwrite reconciliation " - "(default only when no user filter is set)" + "Full overwrite of all explicit perms for the repos in scope\n" + "Must be passed explicitly when no user or repo filter args are provided" ), help_group="Permission sync", ) @@ -220,7 +227,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo env_var="SRC_AUTH_PERMS_SYNC_USERS", cli_flag="--users", metavar="USERS", - help="Process comma-delimited Sourcegraph usernames and/or email addresses", + help="Process a comma-delimited list of Sourcegraph usernames and/or email addresses", help_group="User filters", ) users_without_explicit_perms: bool = src.config_field( @@ -245,7 +252,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo env_var="SRC_AUTH_PERMS_SYNC_REPOS", cli_flag="--repos", metavar="REPOS", - help="Process comma-delimited Sourcegraph repository names", + help="Process a comma-delimited list of Sourcegraph repository names", help_group="Repo filters", ) repos_without_explicit_perms: bool = src.config_field( @@ -253,7 +260,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo env_var="SRC_AUTH_PERMS_SYNC_REPOS_WITHOUT_EXPLICIT_PERMS", cli_flag="--repos-without-explicit-perms", cli_action="store_true", - help="Process Sourcegraph repositories without explicit permissions", + help="Process repositories without explicit permissions", help_group="Repo filters", ) repos_created_after: str | None = src.config_field( @@ -262,7 +269,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo cli_flag="--repos-created-after", metavar="YYYY-MM-DD", pattern=r"^\d{4}-\d{2}-\d{2}$", - help="Process Sourcegraph repositories created on or after this date", + help="Process repositories cloned to the Sourcegraph instance on or after this date", help_group="Repo filters", ) sync_saml_organizations: bool = src.config_field( @@ -278,7 +285,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo env_var="SRC_AUTH_PERMS_SYNC_APPLY", cli_flag="--apply", cli_action="store_true", - help="With mutating commands: actually mutate state. Default is dry-run", + help="Apply changes (default is dry run)", help_group="Mutation", ) no_backup: bool = src.config_field( @@ -296,7 +303,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo metavar="N", ge=1, help="Concurrent Sourcegraph API worker threads (default: 16)", - help_group="Runtime", + help_group="Performance", ) explicit_permissions_batch_size: int = src.config_field( default=25, @@ -307,7 +314,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo help=( "Users per GraphQL request when capturing explicit repository permissions (default: 25)" ), - help_group="Runtime", + help_group="Performance", ) max_attempts: int = src.config_field( default=5, @@ -316,7 +323,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo metavar="N", ge=1, help="Max attempts per HTTP request before giving up (default: 5)", - help_group="Runtime", + help_group="Performance", ) http_timeout_seconds: float = src.config_field( default=60.0, @@ -325,7 +332,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo metavar="SECONDS", gt=0, help="HTTP read timeout per request in seconds (default: 60)", - help_group="Runtime", + help_group="Performance", ) sample_interval: float = src.config_field( default=10.0, @@ -334,7 +341,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo metavar="SECONDS", ge=0, help="Seconds between logging compute resource samples; set 0 to disable (default: 10)", - help_group="Runtime", + help_group="Performance", ) fetch_sg_traces: bool = src.config_field( default=False, @@ -342,7 +349,7 @@ class Config(src.SourcegraphClientConfig, src.LoggingConfig, src.OpenTelemetryCo cli_flag="--fetch-sg-traces", cli_action="store_true", help="Ask Sourcegraph to retain GraphQL traces and return debug trace metadata", - help_group="Runtime", + help_group="Performance", ) @@ -479,6 +486,24 @@ def validate_set_mode_selection(command_name: CommandName, config: Config) -> No "--repos-without-explicit-perms, or --repos-created-after" ) + set_mode_selected = any( + ( + config.full, + bool(config.users), + config.users_without_explicit_perms, + config.created_after is not None, + bool(config.repos), + config.repos_without_explicit_perms, + config.repos_created_after is not None, + ) + ) + if not set_mode_selected: + config_error( + "set requires one of --full, --users, --users-without-explicit-perms, " + "--created-after, --repos, --repos-without-explicit-perms, or " + "--repos-created-after" + ) + def set_command_options(config: Config) -> permission_types.SetCommandOptions: """Return the validated set mode options.""" @@ -600,6 +625,9 @@ def load_cli(argv: Sequence[str] | None = None) -> CliInput: parser.error(str(exception)) command_name = cast(CommandName, arguments.command_name) validate_config(command_name, config) + if command_name == "restore": + assert config.restore_path is not None + require_restore_input_file(config.restore_path) return CliInput(command_name=command_name, config=config) @@ -629,6 +657,15 @@ def require_set_input_file(maps_path: Path) -> None: ) +def require_restore_input_file(restore_path: Path) -> None: + """Exit with a clear error if the selected restore snapshot is missing.""" + if restore_path.is_file(): + return + if restore_path.exists(): + raise SystemExit(f"restore snapshot path is not a file: {restore_path}") + raise SystemExit(f"restore snapshot file does not exist: {restore_path}") + + def run_fields(config: Config, command: ResolvedCommand, endpoint: str) -> dict[str, object]: """Return run-level fields for structured logging.""" fields: dict[str, object] = { @@ -761,6 +798,7 @@ def run_restore( ) -> None: """Run the selected repo-permission restore command.""" assert config.restore_path is not None + require_restore_input_file(config.restore_path) permissions_command.cmd_restore( client, config.restore_path, diff --git a/tests/unit/test_cli_config.py b/tests/unit/test_cli_config.py index c071505..ffca6b2 100644 --- a/tests/unit/test_cli_config.py +++ b/tests/unit/test_cli_config.py @@ -111,7 +111,7 @@ def test_maps_path_is_none_until_defaulted_for_an_endpoint(self) -> None: ): env_file = Path(directory) / ".env" env_file.write_text("") - cli_input = cli.load_cli(["set", "--env-file", str(env_file)]) + cli_input = cli.load_cli(["set", "--env-file", str(env_file), "--full"]) self.assertEqual("set", cli_input.command_name) self.assertIsNone(cli_input.config.maps_path) @@ -133,6 +133,35 @@ def test_restore_path_config_loads_without_selecting_a_command(self) -> None: self.assertEqual(Path.cwd() / "snapshot.json", config.restore_path) + def test_load_cli_rejects_missing_restore_snapshot_file(self) -> None: + with ( + tempfile.TemporaryDirectory() as directory, + mock.patch.dict( + os.environ, + { + "SRC_ENDPOINT": "https://sourcegraph.example.com", + "SRC_ACCESS_TOKEN": "secret", + }, + clear=True, + ), + ): + env_file = Path(directory) / ".env" + env_file.write_text("") + missing_snapshot = Path(directory) / "missing-before.json" + + with self.assertRaises(SystemExit) as exit_context: + cli.load_cli( + [ + "restore", + "--env-file", + str(env_file), + "--restore-path", + str(missing_snapshot), + ] + ) + + self.assertIn("restore snapshot file does not exist", str(exit_context.exception)) + def test_users_config_loads_comma_delimited_values(self) -> None: config = load_config_from_env(SRC_AUTH_PERMS_SYNC_USERS="alice, bob@example.com,,carol") @@ -151,7 +180,7 @@ def test_repos_config_loads_comma_delimited_values(self) -> None: def test_set_command_options_match_each_incremental_mode(self) -> None: self.assertEqual( "full", - cli.set_command_options(make_config(maps_path=Path("maps.yaml"))).mode, + cli.set_command_options(make_config(maps_path=Path("maps.yaml"), full=True)).mode, ) self.assertEqual( ("users", ("alice", "bob@example.com")), @@ -201,7 +230,10 @@ def test_resolve_command_includes_set_mode_names(self) -> None: "set", make_config(maps_path=Path("maps.yaml"), users=("alice",), apply=True), ) - full_command = cli.resolve_command("set", make_config(maps_path=Path("maps.yaml"))) + full_command = cli.resolve_command( + "set", + make_config(maps_path=Path("maps.yaml"), full=True), + ) created_after_command = cli.resolve_command( "set", make_config(maps_path=Path("maps.yaml"), created_after="2026-01-01"), @@ -236,6 +268,7 @@ def test_resolve_command_includes_combined_set_sync_names(self) -> None: maps_path=Path("maps.yaml"), apply=True, sync_saml_organizations=True, + full=True, ), ) @@ -247,7 +280,7 @@ def test_resolve_command_includes_combined_set_sync_names(self) -> None: def test_validate_config_allows_sync_saml_orgs_with_set(self) -> None: cli.validate_config( "set", - make_config(maps_path=Path("maps.yaml"), sync_saml_organizations=True), + make_config(maps_path=Path("maps.yaml"), sync_saml_organizations=True, full=True), ) def test_validate_config_rejects_sync_saml_orgs_without_set(self) -> None: @@ -339,8 +372,12 @@ def test_validate_config_rejects_repo_filters_on_non_get_set_commands(self) -> N "require get or set", ) - def test_validate_config_allows_set_without_explicit_mode(self) -> None: - cli.validate_config("set", make_config(maps_path=Path("maps.yaml"))) + def test_validate_config_rejects_set_without_explicit_mode(self) -> None: + self.assert_config_error( + "set", + make_config(maps_path=Path("maps.yaml")), + "set requires one of --full", + ) def test_created_after_config_accepts_yyyy_mm_dd_date_arguments(self) -> None: config = load_config_from_env(SRC_AUTH_PERMS_SYNC_CREATED_AFTER="2026-01-01") @@ -471,6 +508,16 @@ def test_require_set_input_file_reports_missing_maps_file(self) -> None: cli.require_set_input_file(Path(directory) / "missing.yaml") self.assertIn("set input file does not exist", str(exit_context.exception)) + def test_require_restore_input_file_reports_missing_snapshot_file(self) -> None: + with tempfile.TemporaryDirectory() as directory: + existing_path = Path(directory) / "before.json" + existing_path.write_text("{}\n") + cli.require_restore_input_file(existing_path) + + with self.assertRaises(SystemExit) as exit_context: + cli.require_restore_input_file(Path(directory) / "missing.json") + self.assertIn("restore snapshot file does not exist", str(exit_context.exception)) + def test_config_with_default_paths_only_defaults_omitted_maps_path(self) -> None: endpoint_directory = Path.cwd() / backups.ARTIFACTS_DIR_NAME / "sourcegraph.example.com" @@ -631,7 +678,7 @@ def test_cmd_set_dispatches_repo_filters_to_full_set(self) -> None: self.assertIsNone(cmd_set_full.call_args.kwargs["repository_created_after"]) def test_run_command_passes_set_data_to_combined_sync(self) -> None: - configuration = make_config(sync_saml_organizations=True) + configuration = make_config(sync_saml_organizations=True, full=True) command = cli.resolve_command("set", configuration) client = cast(src.SourcegraphClient, object()) sourcegraph_site_config = object()