From 996fc1ce1370ab5a5a1035f689da2439e11e6587 Mon Sep 17 00:00:00 2001 From: Kieran Manning Date: Sat, 12 Apr 2025 18:36:13 +0100 Subject: [PATCH 1/5] first attempt, adding warnings in place --- .envrc | 19 +++- cloudsmith_cli/cli/commands/repos.py | 3 + cloudsmith_cli/cli/config.py | 91 +++++++++++++++++-- cloudsmith_cli/cli/decorators.py | 30 +++++- .../cli/tests/commands/test_main.py | 8 +- 5 files changed, 133 insertions(+), 18 deletions(-) diff --git a/.envrc b/.envrc index 9d01ead4..a5ae4ac0 100644 --- a/.envrc +++ b/.envrc @@ -18,9 +18,22 @@ if [ -d ".venv" ]; then fi fi -python -m uv venv .venv --python $PYTHON_VERSION -python -m uv pip install -U pip uv -python -m uv pip install -e . +if [ ! -d ".venv" ] +then + # System python doesn't like us installing packages into it + # Test if uv is already installed (via brew or other package manager etc.) + # and use that if available. Otherwise fall back to previous behvaiour + if command -v uv + then + uv venv .venv --python $PYTHON_VERSION + uv pip install -U pip uv + uv pip install -e . + else + python -m uv venv .venv --python $PYTHON_VERSION + python -m uv pip install -U pip uv + python -m uv pip install -e . + fi +fi source ./.venv/bin/activate diff --git a/cloudsmith_cli/cli/commands/repos.py b/cloudsmith_cli/cli/commands/repos.py index db11c148..200f6867 100644 --- a/cloudsmith_cli/cli/commands/repos.py +++ b/cloudsmith_cli/cli/commands/repos.py @@ -101,6 +101,9 @@ def get(ctx, opts, owner_repo, page, page_size): click.echo("Getting list of repositories ... ", nl=False, err=use_stderr) + repo = None + owner = None + if isinstance(owner_repo, list): if len(owner_repo) == 1: owner = owner_repo[0] diff --git a/cloudsmith_cli/cli/config.py b/cloudsmith_cli/cli/config.py index e7638f8e..c76ed81b 100644 --- a/cloudsmith_cli/cli/config.py +++ b/cloudsmith_cli/cli/config.py @@ -77,6 +77,7 @@ class ConfigReader(ConfigFileReader): config_name = "standard" config_searchpath = list(_CFG_SEARCH_PATHS) config_section_schemas = [ConfigSchema.Default, ConfigSchema.Profile] + config_warning_issued = False @classmethod def select_config_schema_for(cls, section_name): @@ -148,7 +149,20 @@ def has_default_file(cls): return False @classmethod - def load_config(cls, opts, path=None, profile=None): + def config_already_warned(cls): + """ + Check if a configuration file warning has been issued. + This is required as configs are gathered at the root of the + command chain as well as for command verbs + """ + if cls.config_warning_issued: + return True + + cls.config_warning_issued = True + return False + + @classmethod + def load_config(cls, opts, path=None, profile=None, no_warn=False): """Load a configuration file into an options object.""" if path and os.path.exists(path): if os.path.isdir(path): @@ -160,9 +174,36 @@ def load_config(cls, opts, path=None, profile=None): values = config.get("default", {}) cls._load_values_into_opts(opts, values) - if profile and profile != "default": - values = config.get("profile:%s" % profile, {}) - cls._load_values_into_opts(opts, values) + warn = not no_warn and not cls.config_already_warned() + + if profile and profile != "default" and warn: + try: + values = config["profile:%s" % profile] + cls._load_values_into_opts(opts, values) + except KeyError: + if warn: + click.secho( + f"Warning: profile {profile} not found in config files {cls.config_files}", + fg="yellow", + ) + + existing_config_paths = { + path: os.path.exists(path) for path in cls.config_files + } + if not any(existing_config_paths.values()) and warn: + click.secho( + "Warning: No config files found in search paths. Tried the following:", + fg="yellow", + ) + for tested_path, exists in existing_config_paths.items(): + if exists: + click.secho(f"{tested_path} - file exists", fg="green") + else: + click.secho(f"{tested_path} - file does not exist", fg="yellow") + click.secho( + "You may need to run `cloudsmith login` to authenticate and create a config file.", + fg="yellow", + ) return values @@ -206,7 +247,31 @@ class CredentialsReader(ConfigReader): config_searchpath = list(_CFG_SEARCH_PATHS) config_section_schemas = [CredentialsSchema.Default, CredentialsSchema.Profile] + @classmethod + def load_config(cls, opts, path=None, profile=None, no_warn=False): + """ + Load a credentials configuration file into an options object. + We overload the load_config command in CredentialsReader as + credentials files have their own specific default functionality. + """ + if path and os.path.exists(path): + if os.path.isdir(path): + cls.config_searchpath.insert(0, path) + else: + cls.config_files.insert(0, path) + + config = cls.read_config() + values = config.get("default", {}) + cls._load_values_into_opts(opts, values) + + if profile and profile != "default": + values = config.get("profile:%s" % profile, {}) + cls._load_values_into_opts(opts, values) + + return values + +# pylint: disable=too-many-public-methods class Options: """Options object that holds config for the application.""" @@ -227,15 +292,15 @@ def get_creds_reader(): """Get the credentials config reader class.""" return CredentialsReader - def load_config_file(self, path, profile=None): + def load_config_file(self, path, profile=None, no_warn=False): """Load the standard config file.""" config_cls = self.get_config_reader() - return config_cls.load_config(self, path, profile=profile) + return config_cls.load_config(self, path, profile=profile, no_warn=no_warn) - def load_creds_file(self, path, profile=None): + def load_creds_file(self, path, profile=None, no_warn=False): """Load the credentials config file.""" config_cls = self.get_creds_reader() - return config_cls.load_config(self, path, profile=profile) + return config_cls.load_config(self, path, profile=profile, no_warn=no_warn) @property def api_config(self): @@ -268,6 +333,16 @@ def api_host(self, value): """Set value for API host.""" self._set_option("api_host", value) + @property + def no_warn(self): + """Get value for API host.""" + return self._get_option("no_warn") + + @no_warn.setter + def no_warn(self, value): + """Set value for API host.""" + self._set_option("no_warn", value) + @property def api_key(self): """Get value for API key.""" diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index a8b41173..0f0804bf 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -5,6 +5,7 @@ import click from ..core.api.init import initialise_api as _initialise_api +from ..core.api.user import get_user_brief from . import config, utils, validators @@ -86,6 +87,12 @@ def common_cli_config_options(f): envvar="CLOUDSMITH_PROFILE", help="The name of the profile to use for configuration.", ) + @click.option( + "--no-warn", + is_flag=True, + default=None, + help="Don't warn on misconfiguration issues", + ) @click.pass_context @functools.wraps(f) def wrapper(ctx, *args, **kwargs): @@ -94,8 +101,11 @@ def wrapper(ctx, *args, **kwargs): profile = kwargs.pop("profile") config_file = kwargs.pop("config_file") creds_file = kwargs.pop("credentials_file") - opts.load_config_file(path=config_file, profile=profile) - opts.load_creds_file(path=creds_file, profile=profile) + no_warn = kwargs.pop("no_warn") + if no_warn: + opts.no_warn = no_warn + opts.load_config_file(path=config_file, profile=profile, no_warn=opts.no_warn) + opts.load_creds_file(path=creds_file, profile=profile, no_warn=opts.no_warn) kwargs["opts"] = opts return ctx.invoke(f, *args, **kwargs) @@ -305,6 +315,22 @@ def call_print_rate_limit_info_with_opts(rate_info): error_retry_cb=opts.error_retry_cb, ) + cloudsmith_host = kwargs["opts"].opts["api_config"].host + no_warn = opts.no_warn + is_auth, _, _, _ = get_user_brief() + if not is_auth and not no_warn: + click.secho( + "Warning: You are not authenticated with the API. " + "Please verify your config files, API key and " + "run `cloudsmith login` if necessary to authenticate.", + fg="yellow", + ) + click.secho( + f"You're currently attempting to connect to Cloudsmith instance {cloudsmith_host}", + fg="yellow", + ) + opts.no_warn = True + kwargs["opts"] = opts return ctx.invoke(f, *args, **kwargs) diff --git a/cloudsmith_cli/cli/tests/commands/test_main.py b/cloudsmith_cli/cli/tests/commands/test_main.py index f007832f..4a7d21b4 100644 --- a/cloudsmith_cli/cli/tests/commands/test_main.py +++ b/cloudsmith_cli/cli/tests/commands/test_main.py @@ -11,11 +11,9 @@ def test_main_version(self, runner, option): """Test the output of `cloudsmith --version`.""" result = runner.invoke(main, [option]) assert result.exit_code == 0 - assert ( - result.output == "Versions:\n" - "CLI Package Version: " + get_version() + "\n" - "API Package Version: " + get_api_version() + "\n" - ) + + assert "CLI Package Version: " + get_version() in result.output + assert "API Package Version: " + get_api_version() in result.output @pytest.mark.parametrize("option", ["-h", "--help"]) def test_main_help(self, runner, option): From fbc418eedeb2017db03826c8ee61060cb9b1f26d Mon Sep 17 00:00:00 2001 From: Kieran Manning Date: Tue, 20 May 2025 21:29:53 +0100 Subject: [PATCH 2/5] move to warnings object --- cloudsmith_cli/cli/commands/main.py | 12 ++++ cloudsmith_cli/cli/commands/repos.py | 3 - cloudsmith_cli/cli/config.py | 45 ++++++------- cloudsmith_cli/cli/decorators.py | 33 +++------- cloudsmith_cli/cli/warnings.py | 97 ++++++++++++++++++++++++++++ 5 files changed, 136 insertions(+), 54 deletions(-) create mode 100644 cloudsmith_cli/cli/warnings.py diff --git a/cloudsmith_cli/cli/commands/main.py b/cloudsmith_cli/cli/commands/main.py index d6586536..ea86edaa 100644 --- a/cloudsmith_cli/cli/commands/main.py +++ b/cloudsmith_cli/cli/commands/main.py @@ -2,6 +2,8 @@ import click +from cloudsmith_cli.cli.warnings import get_or_create_warnings + from ...core.api.version import get_version as get_api_version from ...core.utils import get_github_website, get_help_website from ...core.version import get_version as get_cli_version @@ -61,3 +63,13 @@ def main(ctx, opts, version): print_version() elif ctx.invoked_subcommand is None: click.echo(ctx.get_help()) + + +@main.result_callback() +@click.pass_context +def result_callback(ctx, opts, **kwargs): + """Callback for main function. Required for saving warnings til the end.""" + + warnings = get_or_create_warnings(ctx) + if warnings: + click.echo(warnings.report()) diff --git a/cloudsmith_cli/cli/commands/repos.py b/cloudsmith_cli/cli/commands/repos.py index 200f6867..db11c148 100644 --- a/cloudsmith_cli/cli/commands/repos.py +++ b/cloudsmith_cli/cli/commands/repos.py @@ -101,9 +101,6 @@ def get(ctx, opts, owner_repo, page, page_size): click.echo("Getting list of repositories ... ", nl=False, err=use_stderr) - repo = None - owner = None - if isinstance(owner_repo, list): if len(owner_repo) == 1: owner = owner_repo[0] diff --git a/cloudsmith_cli/cli/config.py b/cloudsmith_cli/cli/config.py index c76ed81b..68744a55 100644 --- a/cloudsmith_cli/cli/config.py +++ b/cloudsmith_cli/cli/config.py @@ -7,6 +7,8 @@ import click from click_configfile import ConfigFileReader, Param, SectionSchema, matches_section +from cloudsmith_cli.cli.warnings import ConfigLoadWarning, ProfileNotFoundWarning + from ..core.utils import get_data_path, read_file from . import utils, validators @@ -162,8 +164,9 @@ def config_already_warned(cls): return False @classmethod - def load_config(cls, opts, path=None, profile=None, no_warn=False): + def load_config(cls, opts, path=None, warnings=None, profile=None): """Load a configuration file into an options object.""" + if path and os.path.exists(path): if os.path.isdir(path): cls.config_searchpath.insert(0, path) @@ -174,36 +177,22 @@ def load_config(cls, opts, path=None, profile=None, no_warn=False): values = config.get("default", {}) cls._load_values_into_opts(opts, values) - warn = not no_warn and not cls.config_already_warned() - - if profile and profile != "default" and warn: + if profile and profile != "default": try: values = config["profile:%s" % profile] cls._load_values_into_opts(opts, values) except KeyError: - if warn: - click.secho( - f"Warning: profile {profile} not found in config files {cls.config_files}", - fg="yellow", - ) + warning = ProfileNotFoundWarning(path=path, profile=profile) + warnings.append(warning) existing_config_paths = { path: os.path.exists(path) for path in cls.config_files } - if not any(existing_config_paths.values()) and warn: - click.secho( - "Warning: No config files found in search paths. Tried the following:", - fg="yellow", - ) - for tested_path, exists in existing_config_paths.items(): - if exists: - click.secho(f"{tested_path} - file exists", fg="green") - else: - click.secho(f"{tested_path} - file does not exist", fg="yellow") - click.secho( - "You may need to run `cloudsmith login` to authenticate and create a config file.", - fg="yellow", + if not any(list(existing_config_paths.values())): + config_load_warning = ConfigLoadWarning( + paths=existing_config_paths, ) + warnings.append(config_load_warning) return values @@ -248,7 +237,7 @@ class CredentialsReader(ConfigReader): config_section_schemas = [CredentialsSchema.Default, CredentialsSchema.Profile] @classmethod - def load_config(cls, opts, path=None, profile=None, no_warn=False): + def load_config(cls, opts, path=None, warnings=None, profile=None): """ Load a credentials configuration file into an options object. We overload the load_config command in CredentialsReader as @@ -292,15 +281,17 @@ def get_creds_reader(): """Get the credentials config reader class.""" return CredentialsReader - def load_config_file(self, path, profile=None, no_warn=False): + def load_config_file(self, path, warnings=None, profile=None): """Load the standard config file.""" + print("load_config_file") config_cls = self.get_config_reader() - return config_cls.load_config(self, path, profile=profile, no_warn=no_warn) + return config_cls.load_config(self, path, warnings=warnings, profile=profile) - def load_creds_file(self, path, profile=None, no_warn=False): + def load_creds_file(self, path, warnings=None, profile=None): """Load the credentials config file.""" + print("load_creds_file") config_cls = self.get_creds_reader() - return config_cls.load_config(self, path, profile=profile, no_warn=no_warn) + return config_cls.load_config(self, path, warnings=warnings, profile=profile) @property def api_config(self): diff --git a/cloudsmith_cli/cli/decorators.py b/cloudsmith_cli/cli/decorators.py index 0f0804bf..d7f38931 100644 --- a/cloudsmith_cli/cli/decorators.py +++ b/cloudsmith_cli/cli/decorators.py @@ -4,6 +4,8 @@ import click +from cloudsmith_cli.cli.warnings import ApiAuthenticationWarning, get_or_create_warnings + from ..core.api.init import initialise_api as _initialise_api from ..core.api.user import get_user_brief from . import config, utils, validators @@ -87,25 +89,17 @@ def common_cli_config_options(f): envvar="CLOUDSMITH_PROFILE", help="The name of the profile to use for configuration.", ) - @click.option( - "--no-warn", - is_flag=True, - default=None, - help="Don't warn on misconfiguration issues", - ) @click.pass_context @functools.wraps(f) def wrapper(ctx, *args, **kwargs): # pylint: disable=missing-docstring opts = config.get_or_create_options(ctx) + warnings = get_or_create_warnings(ctx) profile = kwargs.pop("profile") config_file = kwargs.pop("config_file") creds_file = kwargs.pop("credentials_file") - no_warn = kwargs.pop("no_warn") - if no_warn: - opts.no_warn = no_warn - opts.load_config_file(path=config_file, profile=profile, no_warn=opts.no_warn) - opts.load_creds_file(path=creds_file, profile=profile, no_warn=opts.no_warn) + opts.load_config_file(path=config_file, profile=profile, warnings=warnings) + opts.load_creds_file(path=creds_file, profile=profile, warnings=warnings) kwargs["opts"] = opts return ctx.invoke(f, *args, **kwargs) @@ -316,20 +310,11 @@ def call_print_rate_limit_info_with_opts(rate_info): ) cloudsmith_host = kwargs["opts"].opts["api_config"].host - no_warn = opts.no_warn is_auth, _, _, _ = get_user_brief() - if not is_auth and not no_warn: - click.secho( - "Warning: You are not authenticated with the API. " - "Please verify your config files, API key and " - "run `cloudsmith login` if necessary to authenticate.", - fg="yellow", - ) - click.secho( - f"You're currently attempting to connect to Cloudsmith instance {cloudsmith_host}", - fg="yellow", - ) - opts.no_warn = True + if not is_auth: + warnings = get_or_create_warnings(ctx) + auth_warning = ApiAuthenticationWarning(cloudsmith_host) + warnings.append(auth_warning) kwargs["opts"] = opts return ctx.invoke(f, *args, **kwargs) diff --git a/cloudsmith_cli/cli/warnings.py b/cloudsmith_cli/cli/warnings.py new file mode 100644 index 00000000..6f427e81 --- /dev/null +++ b/cloudsmith_cli/cli/warnings.py @@ -0,0 +1,97 @@ +from abc import ABC +from typing import Dict, List + + +class CliWarning(ABC): + """ + Abstract base class for all Cloudsmith CLI warnings. + """ + + def __repr__(self): + return self.__str__() + + def __str__(self): + return f"{self.__class__.__name__}" + + +class ConfigLoadWarning(CliWarning): + """ + Warning for issues loading the configuration file. + """ + + def __init__(self, paths: Dict[str, bool]): + self.paths = paths + self.message = "Failed to load config files. Tried the following paths: \n" + for path, exists in paths.items(): + self.message += f" - {path} - exists: {exists})\n" + self.message += "You may need to run `cloudsmith login` to authenticate and create a config file." + + def __str__(self): + return f"{self.__class__.__name__} - {self.paths}" + + +class ProfileNotFoundWarning(CliWarning): + """ + Warning for issues loading the configuration file. + """ + + def __init__(self, path, profile): + self.path = path + self.profile = profile + self.message = f"Failed to load config file: {path} for profile: {profile}" + + def __str__(self): + return f"{self.__class__.__name__} - {self.path} - {self.profile}" + + +class ApiAuthenticationWarning(CliWarning): + """ + Warning for issues with API authentication. + """ + + def __init__(self, cloudsmith_host): + self.cloudsmith_host = cloudsmith_host + self.message = "\n".join( + [ + "Failed to authenticate with Cloudsmith API", + "Please check your credentials and try again", + f"Host: {cloudsmith_host}", + ] + ) + + def __str__(self): + return f"{self.__class__.__name__} - {self.cloudsmith_host}" + + +class CliWarnings(list): + """ + A class to manage warnings in the CLI. + """ + + def __init__(self): + super().__init__() + self.warnings: List[CliWarning] = [] + + def append(self, warning: CliWarning): + self.warnings.append(warning) + + def __dedupe__(self) -> List[CliWarning]: + return list(set(self.warnings)) + + def report(self) -> List[CliWarning]: + return self.__dedupe__() + + def __str__(self) -> str: + return ",".join([str(x) for x in self.warnings]) + + def __repr__(self) -> str: + return ",".join([str(x) for x in self.warnings]) + + def __len__(self) -> int: + return len(self.warnings) + + +def get_or_create_warnings(ctx): + """Get or create the options object.""" + + return ctx.ensure_object(CliWarnings) From f63529ba8a567f1920d3ffedfe40350b7363bedc Mon Sep 17 00:00:00 2001 From: Kieran Manning Date: Tue, 27 May 2025 18:46:55 +0100 Subject: [PATCH 3/5] cleanup, no warn on tests --- cloudsmith_cli/cli/commands/main.py | 24 ++++++++-- cloudsmith_cli/cli/config.py | 26 +++-------- .../cli/tests/commands/policy/test_licence.py | 4 +- .../commands/policy/test_vulnerability.py | 4 +- .../cli/tests/commands/test_main.py | 9 ++-- .../tests/commands/test_package_commands.py | 8 +++- .../cli/tests/commands/test_repos.py | 4 +- .../cli/tests/commands/test_upstream.py | 4 +- cloudsmith_cli/cli/tests/conftest.py | 6 +++ cloudsmith_cli/cli/warnings.py | 45 +++++++++++-------- 10 files changed, 83 insertions(+), 51 deletions(-) diff --git a/cloudsmith_cli/cli/commands/main.py b/cloudsmith_cli/cli/commands/main.py index ea86edaa..4f11fbba 100644 --- a/cloudsmith_cli/cli/commands/main.py +++ b/cloudsmith_cli/cli/commands/main.py @@ -1,7 +1,10 @@ """Main command/entrypoint.""" +from functools import partial + import click +from cloudsmith_cli.cli import config from cloudsmith_cli.cli.warnings import get_or_create_warnings from ...core.api.version import get_version as get_api_version @@ -53,12 +56,22 @@ def print_version(): is_flag=True, is_eager=True, ) +@click.option( + "--no-warn", + help="Don't display auth or config warnings", + envvar="CLOUDSMITH_CLI_NO_WARN", + is_flag=True, + default=None, +) @decorators.common_cli_config_options @click.pass_context -def main(ctx, opts, version): +def main(ctx, opts, version, no_warn): """Handle entrypoint to CLI.""" # pylint: disable=unused-argument + if no_warn: + opts.no_warn = True + if version: print_version() elif ctx.invoked_subcommand is None: @@ -67,9 +80,12 @@ def main(ctx, opts, version): @main.result_callback() @click.pass_context -def result_callback(ctx, opts, **kwargs): +def result_callback(ctx, _, **kwargs): """Callback for main function. Required for saving warnings til the end.""" warnings = get_or_create_warnings(ctx) - if warnings: - click.echo(warnings.report()) + opts = config.get_or_create_options(ctx) + + if warnings and not opts.no_warn: + click_warn_partial = partial(click.secho, fg="yellow") + warnings.display(click_warn_partial) diff --git a/cloudsmith_cli/cli/config.py b/cloudsmith_cli/cli/config.py index 68744a55..07a49e07 100644 --- a/cloudsmith_cli/cli/config.py +++ b/cloudsmith_cli/cli/config.py @@ -79,7 +79,6 @@ class ConfigReader(ConfigFileReader): config_name = "standard" config_searchpath = list(_CFG_SEARCH_PATHS) config_section_schemas = [ConfigSchema.Default, ConfigSchema.Profile] - config_warning_issued = False @classmethod def select_config_schema_for(cls, section_name): @@ -150,19 +149,6 @@ def has_default_file(cls): return False - @classmethod - def config_already_warned(cls): - """ - Check if a configuration file warning has been issued. - This is required as configs are gathered at the root of the - command chain as well as for command verbs - """ - if cls.config_warning_issued: - return True - - cls.config_warning_issued = True - return False - @classmethod def load_config(cls, opts, path=None, warnings=None, profile=None): """Load a configuration file into an options object.""" @@ -176,18 +162,20 @@ def load_config(cls, opts, path=None, warnings=None, profile=None): config = cls.read_config() values = config.get("default", {}) cls._load_values_into_opts(opts, values) + existing_config_paths = { + path: os.path.exists(path) for path in cls.config_files + } if profile and profile != "default": try: values = config["profile:%s" % profile] cls._load_values_into_opts(opts, values) except KeyError: - warning = ProfileNotFoundWarning(path=path, profile=profile) + warning = ProfileNotFoundWarning( + paths=existing_config_paths, profile=profile + ) warnings.append(warning) - existing_config_paths = { - path: os.path.exists(path) for path in cls.config_files - } if not any(list(existing_config_paths.values())): config_load_warning = ConfigLoadWarning( paths=existing_config_paths, @@ -283,13 +271,11 @@ def get_creds_reader(): def load_config_file(self, path, warnings=None, profile=None): """Load the standard config file.""" - print("load_config_file") config_cls = self.get_config_reader() return config_cls.load_config(self, path, warnings=warnings, profile=profile) def load_creds_file(self, path, warnings=None, profile=None): """Load the credentials config file.""" - print("load_creds_file") config_cls = self.get_creds_reader() return config_cls.load_config(self, path, warnings=warnings, profile=profile) diff --git a/cloudsmith_cli/cli/tests/commands/policy/test_licence.py b/cloudsmith_cli/cli/tests/commands/policy/test_licence.py index 75bc388c..7941c73e 100644 --- a/cloudsmith_cli/cli/tests/commands/policy/test_licence.py +++ b/cloudsmith_cli/cli/tests/commands/policy/test_licence.py @@ -85,7 +85,9 @@ def assert_output_matches_policy_config(output, config_file_path): return output_table["Identifier"] -@pytest.mark.usefixtures("set_api_key_env_var", "set_api_host_env_var") +@pytest.mark.usefixtures( + "set_api_key_env_var", "set_api_host_env_var", "set_no_warn_env_var" +) def test_license_policy_commands(runner, organization, tmp_path): """Test CRUD operations for license policies.""" diff --git a/cloudsmith_cli/cli/tests/commands/policy/test_vulnerability.py b/cloudsmith_cli/cli/tests/commands/policy/test_vulnerability.py index de03d046..54d389d9 100644 --- a/cloudsmith_cli/cli/tests/commands/policy/test_vulnerability.py +++ b/cloudsmith_cli/cli/tests/commands/policy/test_vulnerability.py @@ -87,7 +87,9 @@ def assert_output_matches_policy_config(output, config_file_path): return output_table["Identifier"] -@pytest.mark.usefixtures("set_api_key_env_var", "set_api_host_env_var") +@pytest.mark.usefixtures( + "set_api_key_env_var", "set_api_host_env_var", "set_no_warn_env_var" +) def test_vulnerability_policy_commands(runner, organization, tmp_path): """Test CRUD operations for vulnerability policies.""" diff --git a/cloudsmith_cli/cli/tests/commands/test_main.py b/cloudsmith_cli/cli/tests/commands/test_main.py index 4a7d21b4..43398c5d 100644 --- a/cloudsmith_cli/cli/tests/commands/test_main.py +++ b/cloudsmith_cli/cli/tests/commands/test_main.py @@ -6,14 +6,17 @@ class TestMainCommand: + @pytest.mark.usefixtures("set_no_warn_env_var") @pytest.mark.parametrize("option", ["-V", "--version"]) def test_main_version(self, runner, option): """Test the output of `cloudsmith --version`.""" result = runner.invoke(main, [option]) assert result.exit_code == 0 - - assert "CLI Package Version: " + get_version() in result.output - assert "API Package Version: " + get_api_version() in result.output + assert ( + result.output == "Versions:\n" + "CLI Package Version: " + get_version() + "\n" + "API Package Version: " + get_api_version() + "\n" + ) @pytest.mark.parametrize("option", ["-h", "--help"]) def test_main_help(self, runner, option): diff --git a/cloudsmith_cli/cli/tests/commands/test_package_commands.py b/cloudsmith_cli/cli/tests/commands/test_package_commands.py index 0da9879d..3c9af02c 100644 --- a/cloudsmith_cli/cli/tests/commands/test_package_commands.py +++ b/cloudsmith_cli/cli/tests/commands/test_package_commands.py @@ -11,7 +11,9 @@ from ..utils import random_str -@pytest.mark.usefixtures("set_api_key_env_var", "set_api_host_env_var") +@pytest.mark.usefixtures( + "set_api_key_env_var", "set_api_host_env_var", "set_no_warn_env_var" +) @pytest.mark.parametrize( "filesize", [ @@ -80,7 +82,9 @@ def test_push_and_delete_raw_package( assert len(data) == 0 -@pytest.mark.usefixtures("set_api_key_env_var", "set_api_host_env_var") +@pytest.mark.usefixtures( + "set_api_key_env_var", "set_api_host_env_var", "set_no_warn_env_var" +) def test_list_packages_with_sort(runner, organization, tmp_repository, tmp_path): """Test listing packages with different sort options.""" org_repo = f'{organization}/{tmp_repository["slug"]}' diff --git a/cloudsmith_cli/cli/tests/commands/test_repos.py b/cloudsmith_cli/cli/tests/commands/test_repos.py index d879b197..cc601544 100644 --- a/cloudsmith_cli/cli/tests/commands/test_repos.py +++ b/cloudsmith_cli/cli/tests/commands/test_repos.py @@ -72,7 +72,9 @@ def assert_output_is_equal_to_repo_config(output, organisation, repo_config_file ) -@pytest.mark.usefixtures("set_api_key_env_var", "set_api_host_env_var") +@pytest.mark.usefixtures( + "set_api_key_env_var", "set_api_host_env_var", "set_no_warn_env_var" +) def test_repos_commands(runner, organization, tmp_path): """Test CRUD operations for repositories.""" diff --git a/cloudsmith_cli/cli/tests/commands/test_upstream.py b/cloudsmith_cli/cli/tests/commands/test_upstream.py index 4559eef0..5283a9b1 100644 --- a/cloudsmith_cli/cli/tests/commands/test_upstream.py +++ b/cloudsmith_cli/cli/tests/commands/test_upstream.py @@ -6,7 +6,9 @@ from ..utils import random_str -@pytest.mark.usefixtures("set_api_key_env_var", "set_api_host_env_var") +@pytest.mark.usefixtures( + "set_api_key_env_var", "set_api_host_env_var", "set_no_warn_env_var" +) @pytest.mark.parametrize("upstream_format", UPSTREAM_FORMATS) def test_upstream_commands( runner, organization, upstream_format, tmp_repository, tmp_path diff --git a/cloudsmith_cli/cli/tests/conftest.py b/cloudsmith_cli/cli/tests/conftest.py index 5e4936a7..b4e50a4f 100644 --- a/cloudsmith_cli/cli/tests/conftest.py +++ b/cloudsmith_cli/cli/tests/conftest.py @@ -73,3 +73,9 @@ def set_api_host_env_var(api_host): def set_api_key_env_var(api_key): """Set the CLOUDSMITH_API_KEY environment variable.""" os.environ["CLOUDSMITH_API_KEY"] = api_key + + +@pytest.fixture() +def set_no_warn_env_var(): + """Set the CLOUDSMITH_API_KEY environment variable.""" + os.environ["CLOUDSMITH_CLI_NO_WARN"] = "True" diff --git a/cloudsmith_cli/cli/warnings.py b/cloudsmith_cli/cli/warnings.py index 6f427e81..dc230e7e 100644 --- a/cloudsmith_cli/cli/warnings.py +++ b/cloudsmith_cli/cli/warnings.py @@ -10,8 +10,14 @@ class CliWarning(ABC): def __repr__(self): return self.__str__() - def __str__(self): - return f"{self.__class__.__name__}" + def __hash__(self): + return hash(self.__str__()) + + def __eq__(self, other): + if not isinstance(other, self.__class__): + return False + + return self.__dict__ == other.__dict__ class ConfigLoadWarning(CliWarning): @@ -23,8 +29,8 @@ def __init__(self, paths: Dict[str, bool]): self.paths = paths self.message = "Failed to load config files. Tried the following paths: \n" for path, exists in paths.items(): - self.message += f" - {path} - exists: {exists})\n" - self.message += "You may need to run `cloudsmith login` to authenticate and create a config file." + self.message += f" - {path} - exists: {exists}\n" + self.message += "You may need to run `cloudsmith login` to authenticate and create a config file. \n" def __str__(self): return f"{self.__class__.__name__} - {self.paths}" @@ -32,13 +38,15 @@ def __str__(self): class ProfileNotFoundWarning(CliWarning): """ - Warning for issues loading the configuration file. + Warning for issues finding the requested profile. """ - def __init__(self, path, profile): - self.path = path + def __init__(self, paths, profile): + self.path = paths self.profile = profile - self.message = f"Failed to load config file: {path} for profile: {profile}" + self.message = f"Failed to load profile {profile} from config. Tried the following paths: \n" + for path, exists in paths.items(): + self.message += f" - {path} - exists: {exists}\n" def __str__(self): return f"{self.__class__.__name__} - {self.path} - {self.profile}" @@ -51,11 +59,11 @@ class ApiAuthenticationWarning(CliWarning): def __init__(self, cloudsmith_host): self.cloudsmith_host = cloudsmith_host - self.message = "\n".join( + self.message = "".join( [ - "Failed to authenticate with Cloudsmith API", - "Please check your credentials and try again", - f"Host: {cloudsmith_host}", + "Failed to authenticate with Cloudsmith API ", + "Please check your credentials and try again.\n", + f"Host: {cloudsmith_host}\n", ] ) @@ -78,13 +86,14 @@ def append(self, warning: CliWarning): def __dedupe__(self) -> List[CliWarning]: return list(set(self.warnings)) - def report(self) -> List[CliWarning]: - return self.__dedupe__() - - def __str__(self) -> str: - return ",".join([str(x) for x in self.warnings]) + def display(self, display_fn): + for warning in self.__dedupe__(): + display_fn(warning.message) def __repr__(self) -> str: + return self.__str__() + + def __str__(self) -> str: return ",".join([str(x) for x in self.warnings]) def __len__(self) -> int: @@ -92,6 +101,6 @@ def __len__(self) -> int: def get_or_create_warnings(ctx): - """Get or create the options object.""" + """Get or create the warnings object.""" return ctx.ensure_object(CliWarnings) From 6e7e7d08697b023c477ced2b2d7a647df5c212c4 Mon Sep 17 00:00:00 2001 From: Kieran Manning Date: Tue, 27 May 2025 20:15:23 +0100 Subject: [PATCH 4/5] adding tests --- cloudsmith_cli/cli/commands/main.py | 1 + .../cli/tests/commands/test_main.py | 9 +++---- cloudsmith_cli/cli/tests/test_warnings.py | 26 +++++++++++++++++++ 3 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 cloudsmith_cli/cli/tests/test_warnings.py diff --git a/cloudsmith_cli/cli/commands/main.py b/cloudsmith_cli/cli/commands/main.py index 4f11fbba..47390931 100644 --- a/cloudsmith_cli/cli/commands/main.py +++ b/cloudsmith_cli/cli/commands/main.py @@ -73,6 +73,7 @@ def main(ctx, opts, version, no_warn): opts.no_warn = True if version: + opts.no_warn = True print_version() elif ctx.invoked_subcommand is None: click.echo(ctx.get_help()) diff --git a/cloudsmith_cli/cli/tests/commands/test_main.py b/cloudsmith_cli/cli/tests/commands/test_main.py index 43398c5d..4a7d21b4 100644 --- a/cloudsmith_cli/cli/tests/commands/test_main.py +++ b/cloudsmith_cli/cli/tests/commands/test_main.py @@ -6,17 +6,14 @@ class TestMainCommand: - @pytest.mark.usefixtures("set_no_warn_env_var") @pytest.mark.parametrize("option", ["-V", "--version"]) def test_main_version(self, runner, option): """Test the output of `cloudsmith --version`.""" result = runner.invoke(main, [option]) assert result.exit_code == 0 - assert ( - result.output == "Versions:\n" - "CLI Package Version: " + get_version() + "\n" - "API Package Version: " + get_api_version() + "\n" - ) + + assert "CLI Package Version: " + get_version() in result.output + assert "API Package Version: " + get_api_version() in result.output @pytest.mark.parametrize("option", ["-h", "--help"]) def test_main_help(self, runner, option): diff --git a/cloudsmith_cli/cli/tests/test_warnings.py b/cloudsmith_cli/cli/tests/test_warnings.py new file mode 100644 index 00000000..df57480a --- /dev/null +++ b/cloudsmith_cli/cli/tests/test_warnings.py @@ -0,0 +1,26 @@ +from cloudsmith_cli.cli.warnings import ( + ApiAuthenticationWarning, + CliWarnings, + ConfigLoadWarning, + ProfileNotFoundWarning, +) + + +class TestWarnings: + def test_warning_append(self): + """Test appending warnings to the CliWarnings.""" + + config_load_warning_1 = ConfigLoadWarning({"test_path_1": False}) + config_load_warning_2 = ConfigLoadWarning({"test_path_2": True}) + profile_load_warning = ProfileNotFoundWarning( + {"test_path_1": False}, "test_profile" + ) + api_authentication_warning = ApiAuthenticationWarning("test.cloudsmith.io") + cli_warnings = CliWarnings() + cli_warnings.append(config_load_warning_1) + cli_warnings.append(config_load_warning_2) + cli_warnings.append(profile_load_warning) + cli_warnings.append(profile_load_warning) + cli_warnings.append(api_authentication_warning) + assert len(cli_warnings) == 5 + assert len(cli_warnings.__dedupe__()) == 4 From 84e4286552c415a2f2b977a12185a7289d294555 Mon Sep 17 00:00:00 2001 From: Kieran Manning Date: Tue, 27 May 2025 20:17:35 +0100 Subject: [PATCH 5/5] revert old test after fixing version warn --- cloudsmith_cli/cli/tests/commands/test_main.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/cloudsmith_cli/cli/tests/commands/test_main.py b/cloudsmith_cli/cli/tests/commands/test_main.py index 4a7d21b4..f007832f 100644 --- a/cloudsmith_cli/cli/tests/commands/test_main.py +++ b/cloudsmith_cli/cli/tests/commands/test_main.py @@ -11,9 +11,11 @@ def test_main_version(self, runner, option): """Test the output of `cloudsmith --version`.""" result = runner.invoke(main, [option]) assert result.exit_code == 0 - - assert "CLI Package Version: " + get_version() in result.output - assert "API Package Version: " + get_api_version() in result.output + assert ( + result.output == "Versions:\n" + "CLI Package Version: " + get_version() + "\n" + "API Package Version: " + get_api_version() + "\n" + ) @pytest.mark.parametrize("option", ["-h", "--help"]) def test_main_help(self, runner, option):