fix(tasks): version bump for helm charts #7419
Conversation
WalkthroughThe pull request modifies Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
When bumping X.Y.9 to X.Y.10, the Helm chart version would not get bumped since the comparison was done on plain strings (it would not detect a new version). Make use of the Python packaging Version type to compare versions instead and solve this issue. Signed-off-by: Fatih Acar <[email protected]>
fatih-acar
force-pushed
the
fac-fix-helm-version-bumper
branch
from
d4e9d5f to
9a5559d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tasks/release.py (1)
99-101: Enhance type hints and docstring per coding guidelines.The function is missing a return type annotation and the docstring doesn't follow the required Google-style format with Args/Returns sections.
As per coding guidelines.
Apply this diff to add the return type hint:
-def update_helm_chart(context: Context, chart_repo: str | None = "helm/") -> None: # noqa: ARG001 +def update_helm_chart(context: Context, chart_repo: str | None = "helm/") -> None: # noqa: ARG001Actually, the return type
-> Noneis already present. Apply this diff to enhance the docstring:- """Update helm/Chart.yaml with the current version from pyproject.toml.""" + """Update helm/Chart.yaml with the current version from pyproject.toml. + + Compares the current app version from pyproject.toml with the version in Chart.yaml + and updates both the appVersion and chart version fields using semantic versioning. + + Args: + context: Invoke context (unused but required by task decorator) + chart_repo: Path to the helm chart repository directory + + Raises: + ValueError: If appVersion or version fields are missing from Chart.yaml + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tasks/release.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/instructions/python-docstring.instructions.md)
**/*.py: Use triple double quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include a brief one-line description at the top of each docstring
Add a detailed description section when additional context is needed
Document function/method parameters under an Args/Parameters section without typing information
Include a Returns section describing the return value
Include a Raises section listing possible exceptions
Provide an Examples section demonstrating usage when helpfulUse ruff and mypy to validate and lint Python files
**/*.py: Use type hints for all Python function parameters and return values
Prefer asynchronous code in Python when feasible
Define asynchronous Python functions withasync def
Useawaitfor asynchronous calls in Python
Use Pydantic models instead of standard dataclasses
Use ruff and mypy for linting and type checking
**/*.py: Use type hints for all Python function parameters and return values
Use async/await whenever possible in Python code
Define asynchronous functions withasync def
Await asynchronous calls withawait
Use Pydantic models instead of standard dataclasses for data modeling
Use triple quotes (""") for all Python docstrings
Write docstrings in Google-style format
Include docstring sections when applicable: one-line summary, optional details, Args (without types), Returns, Raises, Examples
Validate and lint Python with ruff and mypy
Files:
tasks/release.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: E2E-testing-invoke-demo-start
- GitHub Check: backend-docker-integration
- GitHub Check: backend-tests-functional
- GitHub Check: backend-tests-unit
- GitHub Check: backend-tests-integration
🔇 Additional comments (3)
tasks/release.py (3)
103-104: LGTM! Scoped import is appropriate.The local import with clear rationale avoids forcing the packaging dependency on all invoke users. This is a good pattern for optional tooling dependencies.
134-145: Narrow the exception handling and verify downgrade behavior.The version comparison logic correctly addresses the PR objective of using semantic versioning instead of string comparison. However:
The bare
except Exception:on line 143 is too broad and could mask unexpected errors. Consider catching specific exceptions likeInvalidVersionorAttributeError.The logic doesn't explicitly handle version downgrades (when
app_version < old_app_version). Verify this is intentional behavior.Apply this diff to narrow exception handling:
- except Exception: + except (ValueError, AttributeError) as e: # Fallback in case app_version has non-standard format for Helm comparison - print(f"Warning: Unable to strictly compare versions, using default Helm chart version: {new_helm_version}") + print(f"Warning: Unable to strictly compare versions ({e}), using default Helm chart version: {new_helm_version}")
148-149: LGTM! String conversion timing is correct.Converting Version objects to strings immediately before YAML serialization ensures proper formatting in the output files while maintaining semantic version comparisons in the logic.
|
|
||
| # Get the app version directly from pyproject.toml | ||
| app_version = get_version_from_pyproject() # Returns a string like '1.1.0a1' | ||
| app_version = Version(get_version_from_pyproject()) # Returns a string like '1.1.0a1' |
There was a problem hiding this comment.
Add exception handling for malformed version strings.
If get_version_from_pyproject() returns a non-PEP 440 compliant version string, Version() will raise InvalidVersion, crashing the task. Consider wrapping in a try-except block to provide a clearer error message.
Apply this diff to add error handling:
- app_version = Version(get_version_from_pyproject()) # Returns a string like '1.1.0a1'
+ try:
+ app_version = Version(get_version_from_pyproject()) # Returns a string like '1.1.0a1'
+ except Exception as e:
+ raise ValueError(f"Invalid version in pyproject.toml: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app_version = Version(get_version_from_pyproject()) # Returns a string like '1.1.0a1' | |
| try: | |
| app_version = Version(get_version_from_pyproject()) # Returns a string like '1.1.0a1' | |
| except Exception as e: | |
| raise ValueError(f"Invalid version in pyproject.toml: {e}") from e |
🤖 Prompt for AI Agents
In tasks/release.py around line 107, the call creating
Version(get_version_from_pyproject()) can raise packaging.version.InvalidVersion
for malformed/non-PEP440 strings; wrap that call in a try-except that catches
InvalidVersion, include the offending version string in a clear error message,
and exit or re-raise a more descriptive exception so the task fails with
actionable output; also ensure packaging.version.InvalidVersion is imported or
referenced correctly.
| old_app_version = Version(chart_yaml.get("appVersion", "")) | ||
| if old_app_version == app_version: | ||
| print( | ||
| f"{str(chart_path)} updates not required, `appVersion` of {old_app_version} matches current from `pyproject.toml`" | ||
| ) | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
Verify early return doesn't skip remaining charts.
Line 123 uses return within a loop over multiple charts. If the first chart doesn't need updating, the function exits without processing remaining charts. Verify this is intentional, or consider using continue instead.
Run the following script to check how charts are processed:
🏁 Script executed:
#!/bin/bash
# Description: Verify behavior when first chart doesn't need updates
# Check the loop structure and return/continue usage
ast-grep --pattern $'for chart in $$$:
$$$
if $COND:
$$$
return
$$$'Length of output: 0
🏁 Script executed:
#!/bin/bash
sed -n '90,140p' tasks/release.pyLength of output: 2231
Replace return with continue to process all charts. The return inside the loop at tasks/release.py (around line 123) exits the function after the first matching chart, skipping the rest; use continue instead.
🤖 Prompt for AI Agents
In tasks/release.py around lines 118 to 123, the loop currently uses return when
a chart's appVersion already matches the pyproject version, which exits the
entire function and skips remaining charts; change that return to continue so
the loop proceeds to the next chart and all charts are processed. Ensure no
other control flow relies on the function exiting at that point and preserve the
existing print message before continuing.
CodSpeed Performance ReportMerging #7419 will not alter performanceComparing Summary
Footnotes |
When bumping X.Y.9 to X.Y.10, the Helm chart version would not get bumped since the comparison was done on plain strings (it would not detect a new version).
Make use of the Python packaging Version type to compare versions instead and solve this issue.
Summary by CodeRabbit
Bug Fixes
Refactor