-
Notifications
You must be signed in to change notification settings - Fork 27
feat(7983): improving warnings on incorrect or absent configuration #193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
996fc1c
fbc418e
f63529b
6e7e7d0
84e4286
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,7 +1,12 @@ | ||||||
"""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 | ||||||
from ...core.utils import get_github_website, get_help_website | ||||||
from ...core.version import get_version as get_cli_version | ||||||
|
@@ -51,13 +56,37 @@ 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: | ||||||
opts.no_warn = True | ||||||
print_version() | ||||||
elif ctx.invoked_subcommand is None: | ||||||
click.echo(ctx.get_help()) | ||||||
|
||||||
|
||||||
@main.result_callback() | ||||||
@click.pass_context | ||||||
def result_callback(ctx, _, **kwargs): | ||||||
"""Callback for main function. Required for saving warnings til the end.""" | ||||||
|
||||||
warnings = get_or_create_warnings(ctx) | ||||||
opts = config.get_or_create_options(ctx) | ||||||
|
||||||
if warnings and not opts.no_warn: | ||||||
click_warn_partial = partial(click.secho, fg="yellow") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't write warnings to stdout. (Question: Is writing to stdout why a bunch of pre-existing tests now need the pytest fixture to suppress these warnings? If we write warnings to stderr instead, do those tests pass without that fixture?)
Suggested change
|
||||||
warnings.display(click_warn_partial) |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
|
||||||
|
@@ -148,8 +150,9 @@ def has_default_file(cls): | |||||
return False | ||||||
|
||||||
@classmethod | ||||||
def load_config(cls, opts, path=None, profile=None): | ||||||
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) | ||||||
|
@@ -159,10 +162,25 @@ def load_config(cls, opts, path=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": | ||||||
values = config.get("profile:%s" % profile, {}) | ||||||
cls._load_values_into_opts(opts, values) | ||||||
try: | ||||||
values = config["profile:%s" % profile] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
cls._load_values_into_opts(opts, values) | ||||||
except KeyError: | ||||||
warning = ProfileNotFoundWarning( | ||||||
paths=existing_config_paths, profile=profile | ||||||
) | ||||||
warnings.append(warning) | ||||||
|
||||||
if not any(list(existing_config_paths.values())): | ||||||
config_load_warning = ConfigLoadWarning( | ||||||
paths=existing_config_paths, | ||||||
) | ||||||
warnings.append(config_load_warning) | ||||||
Comment on lines
+179
to
+183
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want to warn if no config file was found? The user might have provided the |
||||||
|
||||||
return values | ||||||
|
||||||
|
@@ -206,7 +224,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, warnings=None, profile=None): | ||||||
""" | ||||||
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 +269,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, warnings=None, profile=None): | ||||||
"""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, warnings=warnings, profile=profile) | ||||||
|
||||||
def load_creds_file(self, path, profile=None): | ||||||
def load_creds_file(self, path, warnings=None, profile=None): | ||||||
"""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, warnings=warnings, profile=profile) | ||||||
|
||||||
@property | ||||||
def api_config(self): | ||||||
|
@@ -268,6 +310,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.""" | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
Comment on lines
+76
to
+81
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The need to add this fixture to so many pre-existing tests is a bit of a smell. No, I'm not advocating for |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
from cloudsmith_cli.cli.warnings import ( | ||
ApiAuthenticationWarning, | ||
CliWarnings, | ||
ConfigLoadWarning, | ||
ProfileNotFoundWarning, | ||
) | ||
|
||
|
||
class TestWarnings: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WLTS tests that replicate the scenarios mentioned in the PR description. |
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't see this mentioned in the PR description.
There is probably something to be done here. You're right to question the awkward combination of tooling, but I think it's better to omit this from the PR and raise it separately.