From b177d247a5be22e1c4c1107b2fe9d95afc2b6f9d Mon Sep 17 00:00:00 2001 From: David Lynch Date: Tue, 13 Oct 2026 16:50:24 +0100 Subject: [PATCH] feat(ENG-15090): creating default credential helper dir in user's home directory If an install directory is not specified in the install command, the installer will fall back to the directory the CLI is installed in via which. This causes issues when the CLI is installed via a package manager (brew, apt, nix), as credential helpers will be overwritten on package upgrade, or inaccessible in read-only packaging distributions. - Adding default install path for credential helper at `~/.cloudsmith/bin/` - Cloudsmith managed credential helpers should not interfere with directories managed by package managers. - Default path for cloudsmith credential helpers means all cloudsmith related configs and software are under the same `.cloudsmith` directory. - Adding hint to warning message to add install dir to path - Adding a generic hint to add the credential helper to the path. - Random lints via `ruff check --fix` --- .../cli/commands/credential_helper/manage.py | 28 ++++++++++++++++--- cloudsmith_cli/cli/tests/test_decorators.py | 2 -- cloudsmith_cli/cli/tests/test_org_option.py | 1 - .../credential_helpers/cargo/installer.py | 13 ++++++--- cloudsmith_cli/credential_helpers/common.py | 16 +++++++++++ .../credential_helpers/docker/installer.py | 12 ++++++-- .../credential_helpers/launchers.py | 8 +++--- 7 files changed, 62 insertions(+), 18 deletions(-) diff --git a/cloudsmith_cli/cli/commands/credential_helper/manage.py b/cloudsmith_cli/cli/commands/credential_helper/manage.py index 6935d64e..ad980d0e 100644 --- a/cloudsmith_cli/cli/commands/credential_helper/manage.py +++ b/cloudsmith_cli/cli/commands/credential_helper/manage.py @@ -9,6 +9,7 @@ from __future__ import annotations import sys +from pathlib import Path import click @@ -46,6 +47,17 @@ } +def _resolve_bin_dir(helper: str, bin_dir: str | None) -> str | None: + """Return *bin_dir*, defaulting to ``~/.cloudsmith/bin`` when unset. + + Terraform is left as ``None`` so its installer uses Terraform's plugin + directory (Terraform does not search PATH for credentials helpers). + """ + if bin_dir is not None or helper == "terraform": + return bin_dir + return str(Path.home() / ".cloudsmith" / "bin") + + def _get_installer(name: str): """Return an instantiated installer for *name*, or exit with a clear error. @@ -149,7 +161,10 @@ def _terraform_next_steps(helper_args: tuple[str, ...]) -> list[str]: @click.command("install") @click.argument("helper") @click.option( - "--bin-dir", default=None, help="Directory to install the launcher binary." + "--bin-dir", + default=None, + help="Directory to install the launcher binary. " + "[default: ~/.cloudsmith/bin, or Terraform's plugin directory]", ) @click.option( "--domain", @@ -236,7 +251,7 @@ def install_cmd( installer = _get_installer(helper) install_kwargs = { - "bin_dir": bin_dir, + "bin_dir": _resolve_bin_dir(helper, bin_dir), "domains": domains, "dry_run": dry_run, "discover": not no_discover, @@ -309,7 +324,10 @@ def install_cmd( @click.command("uninstall") @click.argument("helper") @click.option( - "--bin-dir", default=None, help="Directory where the launcher binary was installed." + "--bin-dir", + default=None, + help="Directory where the launcher binary was installed. " + "[default: ~/.cloudsmith/bin, or Terraform's plugin directory]", ) @click.option( "--dry-run", @@ -337,7 +355,9 @@ def uninstall_cmd(ctx, opts, helper: str, bin_dir: str | None, dry_run: bool) -> """ installer = _get_installer(helper) try: - actions = installer.uninstall(bin_dir=bin_dir, dry_run=dry_run) + actions = installer.uninstall( + bin_dir=_resolve_bin_dir(helper, bin_dir), dry_run=dry_run + ) except OSError as exc: raise click.ClickException( f"Failed to uninstall {helper!r} credential helper: {exc}" diff --git a/cloudsmith_cli/cli/tests/test_decorators.py b/cloudsmith_cli/cli/tests/test_decorators.py index 14950bce..7e714db2 100644 --- a/cloudsmith_cli/cli/tests/test_decorators.py +++ b/cloudsmith_cli/cli/tests/test_decorators.py @@ -32,7 +32,6 @@ def test_rejected_sso_session_continues_without_early_exception(): def reject(context): context.keyring_refresh_failed = True context.keyring_refresh_rejected = True - return None with patch( "cloudsmith_cli.cli.decorators.CredentialProviderChain.resolve", @@ -50,7 +49,6 @@ def reject(context): def test_auth_commands_skip_automatic_keyring_refresh(command_name): def resolve(context): assert context.skip_keyring_refresh is True - return None with patch( "cloudsmith_cli.cli.decorators.CredentialProviderChain.resolve", diff --git a/cloudsmith_cli/cli/tests/test_org_option.py b/cloudsmith_cli/cli/tests/test_org_option.py index 470d51ef..ce2c9187 100644 --- a/cloudsmith_cli/cli/tests/test_org_option.py +++ b/cloudsmith_cli/cli/tests/test_org_option.py @@ -139,7 +139,6 @@ def test_workspace_environment_takes_precedence(org_reporting_command, monkeypat def test_credential_warning_writer_uses_stderr_without_debug(org_reporting_command): def emit_warning(context): context.warning_writer("OIDC diagnostic") - return None with patch( "cloudsmith_cli.cli.decorators.CredentialProviderChain.resolve", diff --git a/cloudsmith_cli/credential_helpers/cargo/installer.py b/cloudsmith_cli/credential_helpers/cargo/installer.py index 18d7bc50..32893505 100644 --- a/cloudsmith_cli/credential_helpers/cargo/installer.py +++ b/cloudsmith_cli/credential_helpers/cargo/installer.py @@ -27,9 +27,15 @@ from ...core.cache_utils import merge_config_file from ..backends import BackendKind -from ..common import extract_hostname +from ..common import extract_hostname, path_warning from ..custom_domains import get_format_domains -from ..launchers import is_on_path, remove_launcher, resolve_bin_dir, write_launcher +from ..launchers import ( + is_on_path, + is_windows, + remove_launcher, + resolve_bin_dir, + write_launcher, +) if TYPE_CHECKING: from ...core.credentials.models import CredentialResult @@ -347,8 +353,7 @@ def mutate(config: dict) -> None: if not is_on_path(target_dir): actions.append( - f"WARNING: {target_dir} is not on PATH — " - f"add it to your PATH so Cargo can find {self.LAUNCHER_NAME}" + path_warning(target_dir, "Cargo", self.LAUNCHER_NAME, is_windows()) ) return actions diff --git a/cloudsmith_cli/credential_helpers/common.py b/cloudsmith_cli/credential_helpers/common.py index d79bd0ab..bc8dd54c 100644 --- a/cloudsmith_cli/credential_helpers/common.py +++ b/cloudsmith_cli/credential_helpers/common.py @@ -6,12 +6,28 @@ """ import logging +from pathlib import Path from .custom_domains import get_custom_domains, get_format_domains logger = logging.getLogger(__name__) +def path_warning( + target_dir: Path, helper_format: str, launcher: str, windows: bool +) -> str: + # if not windows, assume POSIX + if windows: + hint = f'$env:Path += ";{target_dir}"' + else: + hint = f'export PATH="$PATH:{target_dir}"' + return ( + f"WARNING: {target_dir} is not on PATH\n" + f"add it to your PATH so {helper_format} can find {launcher}:\n\n" + f" {hint}" + ) + + def extract_hostname(url): """ Extract bare hostname from any URL format. diff --git a/cloudsmith_cli/credential_helpers/docker/installer.py b/cloudsmith_cli/credential_helpers/docker/installer.py index e4aa4df7..0382b9a9 100644 --- a/cloudsmith_cli/credential_helpers/docker/installer.py +++ b/cloudsmith_cli/credential_helpers/docker/installer.py @@ -17,8 +17,15 @@ from ...core.cache_utils import merge_config_file from ..backends import BackendKind +from ..common import path_warning from ..custom_domains import get_format_domains -from ..launchers import is_on_path, remove_launcher, resolve_bin_dir, write_launcher +from ..launchers import ( + is_on_path, + is_windows, + remove_launcher, + resolve_bin_dir, + write_launcher, +) if TYPE_CHECKING: from ...core.credentials.models import CredentialResult @@ -226,8 +233,7 @@ def mutate(config: dict) -> None: if not is_on_path(target_dir): actions.append( - f"WARNING: {target_dir} is not on PATH — " - "add it to your PATH so Docker can find docker-credential-cloudsmith" + path_warning(target_dir, "Docker", self.LAUNCHER_NAME, is_windows()) ) return actions diff --git a/cloudsmith_cli/credential_helpers/launchers.py b/cloudsmith_cli/credential_helpers/launchers.py index 2886879e..26fa967b 100644 --- a/cloudsmith_cli/credential_helpers/launchers.py +++ b/cloudsmith_cli/credential_helpers/launchers.py @@ -19,7 +19,7 @@ from pathlib import Path -def _is_windows() -> bool: +def is_windows() -> bool: """Return True when running on Windows.""" return os.name == "nt" @@ -72,7 +72,7 @@ def write_launcher(bin_dir: Path, name: str, target_cmd: str, dry_run=False) -> Path The path of the written file. """ - windows = _is_windows() + windows = is_windows() if dry_run: if windows: return bin_dir / f"{name}.cmd" @@ -106,7 +106,7 @@ def remove_launcher(bin_dir: Path, name: str, dry_run=False) -> bool: bool ``True`` if a file was removed, ``False`` if no file was found. """ - target = Path(bin_dir) / _launcher_filename(name, windows=_is_windows()) + target = Path(bin_dir) / _launcher_filename(name, windows=is_windows()) if target.exists(): if not dry_run: @@ -156,7 +156,7 @@ def resolve_bin_dir(override: str | None = None) -> Path: return candidate.resolve() # Option 3: user-local bin - return _user_bin_dir(_is_windows()).resolve() + return _user_bin_dir(is_windows()).resolve() def is_on_path(directory: Path) -> bool: