BL-143 | Version warning #299
Conversation
WalkthroughThe changes enhance the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as leverage
participant Conf as Config Loader
U->>CLI: Run leverage command
CLI->>Conf: conf.load()
Conf-->>CLI: Return configuration (including TERRAFORM_IMAGE_TAG)
CLI->>CLI: Compare current version with MINIMUM_VERSIONS
alt Version below minimum
CLI->>U: Display warning message
else Version meets requirement
CLI->>U: Proceed normally
end
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Pull Request Test Coverage Report for Build 13871569654Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
leverage/leverage.py (2)
29-36: Enhance version check implementation for better user experience.The version checking logic is well implemented, but consider the following improvements:
- Move this logic to a separate function for better modularity
- Provide more guidance to users on how to upgrade
- Consider adding an environment variable or config option to disable the warning
- Add error handling for invalid version formats
- # if there is a version restriction set, make sure we satisfy it - config = conf.load() - minimum_version = config.get("LEVERAGE_CLI_VERSION") - if minimum_version and Version(__version__) < Version(minimum_version): - click.echo( - f"\033[91mWARNING\033[0m\tYour current version ({__version__}) is lower than the required minimum ({minimum_version})." - ) + # Check if the current version meets the minimum required version + _check_version_compatibility() + +def _check_version_compatibility(): + """Check if the current version meets the minimum required version.""" + try: + config = conf.load() + minimum_version = config.get("LEVERAGE_CLI_VERSION") + + if not minimum_version: + return + + if Version(__version__) < Version(minimum_version): + click.echo( + f"\033[91mWARNING\033[0m\tYour current version ({__version__}) is lower than the required minimum ({minimum_version}).\n" + f"Please upgrade to the latest version using: pip install --upgrade leverage" + ) + except ValueError: + # Handle invalid version format + click.echo( + f"\033[91mWARNING\033[0m\tInvalid version format detected in configuration: {minimum_version}" + )
5-5:❓ Verification inconclusive
Consider using
importlib-metadatafor potentially better backward compatibility.The
packaging.versionmodule is a good choice for semantic version comparison, but it's worth noting that for Python versions < 3.8, you might need to use a backport version. Consider checking your Python compatibility requirements.
🏁 Script executed:
#!/bin/bash # Check the Python version requirement of the project # Look for a setup.py file to determine Python version requirements fd "setup.py" --exec grep -E "python_requires|classifiers.*Python" {} \; # Or check any requirements.txt for Python version specifications fd "requirements.txt" --exec grep -E "Python" {} \; # Check the current Python version used in the project if [ -f .python-version ]; then cat .python-version elif [ -f runtime.txt ]; then cat runtime.txt fiLength of output: 182
Python Version Compatibility Consideration in
leverage/leverage.pyThe current use of
packaging.versionis acceptable if your project targets Python 3.8 or later. However, since no explicit Python version requirements were detected (e.g., insetup.py,requirements.txt,pyproject.toml, orsetup.cfg), please manually verify whether backward compatibility for Python versions earlier than 3.8 is needed. If so, consider one of the following approaches:
- Explicitly document the minimum Python version: Add a Python version requirement (e.g., via
python_requiresor similar) so that it’s clear which versions are supported.- Use a conditional import for backward compatibility: For Python versions <3.8, you might leverage the
importlib_metadatabackport to ensure better compatibility.Please review your project’s Python version requirements and adjust accordingly if support for earlier versions is necessary.
tests/test_conf.py (4)
1-1: Use contextlib mock for better Python 3 compatibility.For Python 3, it's generally recommended to use
unittest.mockdirectly through theunittestpackage rather than importing justmock. For broader compatibility, especially with different Python versions, consider using:-from unittest import mock +import unittest.mock as mockAlternatively, the
mocklibrary is available as a standalone package for older Python versions.
60-69: Expand test coverage for version validation scenarios.The test is well-structured but only covers one scenario: when the current version is lower than the required minimum. Consider expanding test coverage to include additional scenarios:
- When no minimum version is specified
- When the current version matches the minimum
- When the current version is higher than the minimum
- When an invalid version format is provided in the configuration
@pytest.mark.parametrize( "config_version, expected_warning", [ ("99.9.9", True), # Version higher than current - should show warning (None, False), # No version specified - should not show warning ("0.0.1", False), # Version lower than current - should not show warning (__version__, False), # Same version - should not show warning ("invalid", True), # Invalid version format - should show warning ], ) def test_version_validation(config_version, expected_warning): """ Test version validation for different scenarios. """ runner = CliRunner() config = {} if config_version is not None: config["LEVERAGE_CLI_VERSION"] = config_version with mock.patch("leverage.conf.load", return_value=config): result = runner.invoke(leverage) warning_present = "lower than the required minimum" in result.output assert warning_present == expected_warning assert result.exit_code == 0 # Command should always execute successfullyAdditionally, consider mocking
__version__to make the test more deterministic and not dependent on the actual package version.
65-65: Consider using decorator-based mocking for cleaner test structure.While the context manager approach for mocking works well, consider using the decorator-based mocking pattern for improved readability and organization, especially for simple cases:
-def test_version_validation(): - """ - Test that we get a warning if we are working with a version lower than the required by the project. - """ - runner = CliRunner() - with mock.patch("leverage.conf.load", return_value={"LEVERAGE_CLI_VERSION": "99.9.9"}): - result = runner.invoke(leverage) +@mock.patch("leverage.conf.load", return_value={"LEVERAGE_CLI_VERSION": "99.9.9"}) +def test_version_validation(mock_load): + """ + Test that we get a warning if we are working with a version lower than the required by the project. + """ + runner = CliRunner() + result = runner.invoke(leverage)
68-69: Use a more specific assertion for the warning message.The current assertion checks if a specific string is in the output, which is good, but consider using a more precise assertion pattern to ensure the entire warning message is correctly formatted:
- assert "is lower than the required minimum (99.9.9)" in result.output + expected_warning = f"Your current version ({leverage.__version__}) is lower than the required minimum (99.9.9)" + assert expected_warning in result.output assert result.exit_code == 0This makes the test more specific about what it's looking for and will catch formatting changes to the warning message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
leverage/leverage.py(2 hunks)tests/test_conf.py(2 hunks)
🔇 Additional comments (1)
leverage/leverage.py (1)
9-9:❓ Verification inconclusive
Ensure dependency is properly defined in requirements.
The import of
conffrom theleveragepackage is appropriate for accessing configuration. Make sure this dependency is properly defined in your project's requirements or setup files.
🏁 Script executed:
#!/bin/bash # Verify if packaging dependency is properly defined in requirements # Check for packaging in requirements files fd "requirements.*.txt|setup.py" --exec grep -E "packaging" {} \;Length of output: 68
Manual Verification Required: Confirm Dependency Declaration
The import of
conffrom theleveragepackage appears correct for configuration access. However, the automated check (searching for dependency hints in setup files) was inconclusive. Please verify by manually ensuring that theleveragedependency (or the appropriate package name that providesconf) is explicitly listed in your project's setup or requirements files.
- Ensure that files like
setup.pyor anyrequirements*.txtinclude a declaration for the dependency.- Confirm that the package referenced in the import is the same as the one declared in your dependency management files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
leverage/leverage.py (2)
32-35: Ensure the fallback logic is intentional.If loading config fails due to
NotARepositoryError, the code returns immediately, skipping the version check. Confirm whether skipping the warning is the desired behavior in non-repository environments.Would you like me to open a new issue to clarify this behavior or provide additional logging here?
36-41: Consider providing feedback when the CLI version meets or exceeds the requirement.While the current code displays a warning upon version mismatch, you might also display a subtle message confirming that the version is sufficient. This can reassure users and further enhance developer experience.
if minimum_version and Version(__version__) < Version(minimum_version): click.echo( f"\033[91mWARNING\033[0m\tYour current version ({__version__}) is lower than the required minimum ({minimum_version})." ) +elif minimum_version: + click.echo(f"Your Leverage CLI version ({__version__}) meets the minimum requirement ({minimum_version}).")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
leverage/leverage.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration_tests_cli_refarch (3.9.15, 1.3.5-0.2.0)
🔇 Additional comments (4)
leverage/leverage.py (4)
5-5: Usingpackaging.versionfor version checks is a best practice.No concerns here; this approach provides reliable version comparisons.
9-9: Validate the usage ofconfacross the codebase.Make sure that the
confobject is only accessed after confirming that the environment is indeed a Leverage repository. Consider verifying if there's any scenario whereconfcould be used outside a repository context.
14-14: ImportingNotARepositoryErroris appropriate for error handling.This error type helps differentiate repository-related issues from other exceptions.
30-31: Documenting version restriction checks improves code readability.The inline comment clearly articulates the purpose of this logic. Good job.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_conf.py (2)
60-71: Test looks good but could be improved with a singlewithstatement.The test properly validates the version warning functionality by mocking the configuration and checking for appropriate warning messages in the output.
Consider combining the nested
withstatements for better readability:- with mock.patch("leverage.conf.load", return_value={"TERRAFORM_IMAGE_TAG": "1.1.1-2.2.2"}): - with mock.patch.dict("leverage.MINIMUM_VERSIONS", {"TERRAFORM": "3.3.3", "LEVERAGE_CLI": "4.4.4"}): - result = runner.invoke(leverage) + with mock.patch("leverage.conf.load", return_value={"TERRAFORM_IMAGE_TAG": "1.1.1-2.2.2"}), \ + mock.patch.dict("leverage.MINIMUM_VERSIONS", {"TERRAFORM": "3.3.3", "LEVERAGE_CLI": "4.4.4"}): + result = runner.invoke(leverage)🧰 Tools
🪛 Ruff (0.8.2)
65-66: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
69-70: Consider adding separate assertions for clearer test failures.The current assertions check for substring presence which is good, but when they fail, it might be less clear which part failed.
Consider splitting the assertions for the warning messages to make test failures more specific:
- assert "Your current TERRAFORM version (1.1.1) is lower than the required minimum (3.3.3)" in result.output.replace("\n", "") - assert "Your current LEVERAGE_CLI version (2.2.2) is lower than the required minimum (4.4.4)" in result.output.replace("\n", "") + output = result.output.replace("\n", "") + terraform_warning = "Your current TERRAFORM version (1.1.1) is lower than the required minimum (3.3.3)" + leverage_cli_warning = "Your current LEVERAGE_CLI version (2.2.2) is lower than the required minimum (4.4.4)" + + assert terraform_warning in output, "Missing Terraform version warning" + assert leverage_cli_warning in output, "Missing Leverage CLI version warning"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
leverage/__init__.py(1 hunks)leverage/leverage.py(2 hunks)tests/test_conf.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- leverage/leverage.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_conf.py
65-66: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
🔇 Additional comments (1)
leverage/__init__.py (1)
10-13: LGTM: Clean implementation of minimum version requirements.The
MINIMUM_VERSIONSdictionary is well-structured and follows proper Python naming conventions. This implementation aligns with the PR objective of supporting version warnings when the CLI version is below the specified minimum.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
leverage/leverage.py (1)
43-47: Consider making the version parsing more robust.The current approach assumes that
TERRAFORM_IMAGE_TAGfollows a specific format where splitting by-produces values that match the keys inMINIMUM_VERSIONS. This might be fragile if the format changes in the future.-# validate both CLI and TF versions -for key, current in zip(MINIMUM_VERSIONS, current_values.split("-")): - if Version(current) < Version(MINIMUM_VERSIONS[key]): - rich.print( - f"[red]WARNING[/red]\tYour current {key} version ({current}) is lower than the required minimum ({MINIMUM_VERSIONS[key]})." - ) +# validate both CLI and TF versions +parts = current_values.split("-") +# Ensure we have enough parts to compare +if len(parts) >= len(MINIMUM_VERSIONS): + for i, (key, min_version) in enumerate(MINIMUM_VERSIONS.items()): + current = parts[i] + try: + if Version(current) < Version(min_version): + rich.print( + f"[red]WARNING[/red]\tYour current {key} version ({current}) is lower than the required minimum ({min_version})." + ) + except ValueError: + # Handle case where version string isn't valid + rich.print(f"[yellow]WARNING[/yellow]\tUnable to parse version: {current}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
leverage/leverage.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration_tests_cli_refarch (3.9.15, 1.3.5-0.2.0)
🔇 Additional comments (6)
leverage/leverage.py (6)
5-6: LGTM - Good choice of libraries for version comparison and colored output.The imports of
richandVersionfrompackaging.versionare appropriate for implementing the version warning feature. Therichlibrary provides excellent formatting capabilities for console output, andVersionhandles semantic versioning comparisons correctly.
10-10: LGTM - Imports required for version checking functionality.The additional imports of
confandMINIMUM_VERSIONSfrom the leverage package are necessary for the new version checking feature. This follows good practice by importing only what's needed.
15-15: LGTM - Good exception handling preparation.Importing
NotARepositoryErrorproperly prepares for the exception handling in the version checking logic.
31-37: LGTM - Proper error handling for non-repository contexts.The version restriction check is correctly wrapped in a try-except block to gracefully handle cases when the command is run outside a leverage repository. This ensures the version check only runs in relevant contexts.
38-42: LGTM - Graceful handling of initialization state.The code correctly handles the case where the configuration file hasn't been created yet (e.g., during initialization), preventing unnecessary errors during early stages of project setup.
45-47: Great implementation of the colored warning message.The implementation using
rich.print()with color formatting for the warning message is excellent and consistent with the recommendation from the previous review. This provides clear visual feedback to users about version compatibility issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
leverage/leverage.py (3)
43-48: Consider additional input validation before splitting current_values.While the code correctly compares versions, it assumes that
current_valuescan be split with-and that the resulting parts match the keys inMINIMUM_VERSIONS. This could potentially lead to errors if the format changes.- for key, current in zip(MINIMUM_VERSIONS, current_values.split("-")): - if Version(current) < Version(MINIMUM_VERSIONS[key]): - rich.print( - f"[red]WARNING[/red]\tYour current {key} version ({current}) is lower than the required minimum ({MINIMUM_VERSIONS[key]})." - ) + # Split values and ensure we have the expected format + try: + parts = current_values.split("-") + if len(parts) != len(MINIMUM_VERSIONS): + return # Skip validation if format doesn't match + + for key, current in zip(MINIMUM_VERSIONS, parts): + if Version(current) < Version(MINIMUM_VERSIONS[key]): + rich.print( + f"[red]WARNING[/red]\tYour current {key} version ({current}) is lower than the required minimum ({MINIMUM_VERSIONS[key]})." + ) + except ValueError: + # Handle the case where version strings are not valid + pass
31-48: Consider adding error logging for unexpected issues.The code currently has no logging for potential errors that might occur during version checking, such as invalid version formats. Adding some debug logging would help with troubleshooting.
+ import logging + + logger = logging.getLogger(__name__) + ... try: config = conf.load() except NotARepositoryError: # restrictions are only verified within a leverage project + logger.debug("Not in a Leverage repository, skipping version check") return ... try: for key, current in zip(MINIMUM_VERSIONS, current_values.split("-")): if Version(current) < Version(MINIMUM_VERSIONS[key]): rich.print( f"[red]WARNING[/red]\tYour current {key} version ({current}) is lower than the required minimum ({MINIMUM_VERSIONS[key]})." ) except Exception as e: + logger.debug(f"Error during version check: {e}")
23-48: Consider adding a docstring for the version check functionality.It would be helpful to add documentation about the new version checking functionality, explaining what it does and how it works, either as a docstring within the
leveragefunction or as comments.def leverage(context, state, verbose): """Leverage Reference Architecture projects command-line tool.""" + """ + This function serves as the main entry point for the Leverage CLI tool. + + It performs the following operations: + 1. Sets up verbosity based on command line options + 2. If no subcommand is provided, displays help + 3. Checks if the current CLI version meets minimum requirements defined in the project's configuration + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
leverage/__init__.py(1 hunks)leverage/leverage.py(2 hunks)tests/test_conf.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- leverage/init.py
- tests/test_conf.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration_tests_cli_refarch (3.9.15, 1.3.5-0.2.0)
🔇 Additional comments (6)
leverage/leverage.py (6)
5-7: Good addition of required imports.The imports for
richandVersionfrompackaging.versionare appropriate for the version comparison functionality being implemented.
10-10: Properly updated imports from leverage module.Good job updating the imports to include the necessary components (
confandMINIMUM_VERSIONS) required for the version checking functionality.
15-15: Import for exception handling is correctly added.The import for
NotARepositoryErroris well placed and will be used appropriately in the version checking logic.
31-37: Appropriate error handling for non-repository contexts.The try-except block correctly handles the case when the code is executed outside of a Leverage repository context, preventing unnecessary warnings in irrelevant environments.
38-42: Good handling of edge cases in configuration.The code properly handles the case when the configuration exists but doesn't contain the required version information, which might occur during project initialization.
45-47: Colorized warning message looks good.The warning message using
rich.print()with colorized output is a nice touch and addresses the previous review comment by making the warning more visible and consistent with the project's styling.
What?
If you have specified a minimum version at your
build.envfile (LEVERAGE_CLI_VERSION), you will get a warning when the currenct CLI version is behind it.$ cat build.envWhy?
Be able to tell what CLI version was used in a given Leverage project.
References
closes #143
Summary by CodeRabbit
New Features
Tests