feat(7983): improving warnings on incorrect or absent configuration #193
Conversation
kieranmanning
force-pushed
the
ENG-7893/improve-first-cli-use-behaviour
branch
2 times, most recently
from
b7a3909 to
08876e1
Compare
kieranmanning
force-pushed
the
ENG-7893/improve-first-cli-use-behaviour
branch
from
26b75af to
e08e621
Compare
kieranmanning
changed the title
kieranmanning
force-pushed
the
ENG-7893/improve-first-cli-use-behaviour
branch
from
e08e621 to
84e4286
Compare
| 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 |
There was a problem hiding this comment.
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.
| return f"{self.__class__.__name__} - {self.cloudsmith_host}" | ||
|
|
||
|
|
||
| class CliWarnings(list): |
There was a problem hiding this comment.
Why inherit from list here? Can't we just use composition rather than inheritance?
| def __dedupe__(self) -> List[CliWarning]: | ||
| return list(set(self.warnings)) |
There was a problem hiding this comment.
Avoid user-defined dunders.
I don't see this used anywhere except in the test... what do we need it for?
| def __repr__(self) -> str: | ||
| return self.__str__() | ||
|
|
||
| def __str__(self) -> str: | ||
| return ",".join([str(x) for x in self.warnings]) |
There was a problem hiding this comment.
NAB: __str__ implicitly returns __repr__. You only need to define __repr__.
|
|
||
| def __eq__(self, other): | ||
| if not isinstance(other, self.__class__): | ||
| return False |
There was a problem hiding this comment.
| return False | |
| return NotImplemented |
| class CliWarning(ABC): | ||
| """ | ||
| Abstract base class for all Cloudsmith CLI warnings. | ||
| """ |
There was a problem hiding this comment.
What's the benefit of using an abstract base class for this purpose?
| if not any(list(existing_config_paths.values())): | ||
| config_load_warning = ConfigLoadWarning( | ||
| paths=existing_config_paths, | ||
| ) | ||
| warnings.append(config_load_warning) |
There was a problem hiding this comment.
Do we really want to warn if no config file was found?
The user might have provided the CLOUDSMITH_API_KEY environment variable for example, such as we do in the pytest suite.
| 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.
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?)
| click_warn_partial = partial(click.secho, fg="yellow") | |
| click_warn_partial = partial(click.secho, fg="yellow", err=True) |
| ) | ||
|
|
||
|
|
||
| class TestWarnings: |
There was a problem hiding this comment.
WLTS tests that replicate the scenarios mentioned in the PR description.
|
|
||
|
|
||
| @pytest.fixture() | ||
| def set_no_warn_env_var(): | ||
| """Set the CLOUDSMITH_API_KEY environment variable.""" | ||
| os.environ["CLOUDSMITH_CLI_NO_WARN"] = "True" |
There was a problem hiding this comment.
The need to add this fixture to so many pre-existing tests is a bit of a smell.
No, I'm not advocating for autouse=True :)
What?
Improve Cloudsmith CLI user experience when encountering configuration issues or uncertainties. Can be disabled with a
--no-warnflagWhy?
Currently, there are a number of potential pitfalls around config management for the Cloudsmith CLI. These include:
Acceptance Criteria (Is It Done?)
Screenshots