[feature] Added support for non-python packages #564
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughDetects package type by probing repository files via get_package_type_from_setup(). load_config() sets config["package_type"] and dispatches to PACKAGE_VERSION_HANDLERS (python, npm, docker, ansible, openwrt) to populate version_path and config["CURRENT_VERSION"]. get_current_version() reads CURRENT_VERSION from config. bump_version() delegates updates to per-package handlers (tuple edit, JSON write, regex replace, or full-file replace). New test fixtures and unit tests cover detection, parsing, error cases, and bumping for all supported package types. Sequence Diagram(s)sequenceDiagram
participant User as User/CI
participant ConfigLoader as Config Loader
participant FileSystem as File System
participant VersionMgr as Version Manager
User->>ConfigLoader: load_config(project_path)
ConfigLoader->>FileSystem: probe files (setup.py, package.json, Makefile, docker-compose.yml, .ansible-lint, .luacheckrc, VERSION)
FileSystem-->>ConfigLoader: indicators found / package_type selected
alt python
ConfigLoader->>FileSystem: read package __init__.py VERSION tuple
FileSystem-->>ConfigLoader: VERSION tuple
else npm
ConfigLoader->>FileSystem: read package.json version
FileSystem-->>ConfigLoader: version field
else docker
ConfigLoader->>FileSystem: read Makefile OPENWISP_VERSION
FileSystem-->>ConfigLoader: OPENWISP_VERSION
else ansible
ConfigLoader->>FileSystem: read templates/openwisp2/version.py __openwisp_version__
FileSystem-->>ConfigLoader: version string
else openwrt-agents
ConfigLoader->>FileSystem: read VERSION file
FileSystem-->>ConfigLoader: version string
end
ConfigLoader->>ConfigLoader: normalize to CURRENT_VERSION tuple
ConfigLoader-->>User: return config with package_type, version_path, CURRENT_VERSION
User->>VersionMgr: bump_version(config, new_version)
alt python
VersionMgr->>FileSystem: update VERSION tuple in __init__.py
else npm
VersionMgr->>FileSystem: update "version" in package.json (JSON write)
else docker
VersionMgr->>FileSystem: replace OPENWISP_VERSION in Makefile (regex)
else ansible
VersionMgr->>FileSystem: replace __openwisp_version__ in templates/openwisp2/version.py (regex)
else openwrt-agents
VersionMgr->>FileSystem: overwrite VERSION file with new_version
end
FileSystem-->>User: confirm updated files
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_utils/releaser/version.py (1)
32-105: Handle missing/unknownpackage_typeto avoid UnboundLocalError.If
package_typeis absent or unsupported, none of the branches setnew_content, so the final write will crash. This is also a backwards-compatibility break for callers using legacy configs. Consider defaulting topythonand add an explicit unsupported-type error.🛠️ Proposed guardrail
- package_type = config.get("package_type") + package_type = config.get("package_type") or "python" if not version_path: # version bumping was not performed return False @@ elif package_type == "openwrt-agents": new_content = f"{new_version}\n" count = 1 if count == 0: raise RuntimeError(f"Failed to find and bump in {version_path}") + else: + raise ValueError(f"Unsupported package_type: {package_type}")
🤖 Fix all issues with AI agents
In `@openwisp_utils/releaser/config.py`:
- Around line 115-126: The current check in the version parsing logic accepts
versions with fewer than 3 parts but silently truncates ones with more than 3;
update the validation around version_tuple/version_str so it requires exactly 3
dot-separated parts (len(parts) == 3) and raises a ValueError with a clear
message if not, before populating config["CURRENT_VERSION"]; apply the same
exact-3-parts validation to the equivalent parsing logic for the
docker/ansible/openwrt branches as well.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_utils/releaser/config.pyopenwisp_utils/releaser/tests/conftest.pyopenwisp_utils/releaser/tests/test_config.pyopenwisp_utils/releaser/tests/test_version_bumping.pyopenwisp_utils/releaser/version.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_utils/releaser/tests/test_version_bumping.py (2)
openwisp_utils/releaser/tests/conftest.py (1)
mock_config(16-17)openwisp_utils/releaser/version.py (2)
get_current_version(7-26)bump_version(29-106)
🪛 Ruff (0.14.14)
openwisp_utils/releaser/config.py
117-119: Abstract raise to an inner function
(TRY301)
117-119: Avoid specifying long messages outside the exception class
(TRY003)
141-143: Avoid specifying long messages outside the exception class
(TRY003)
166-168: Abstract raise to an inner function
(TRY301)
166-168: Avoid specifying long messages outside the exception class
(TRY003)
185-187: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_utils/releaser/version.py
22-22: Consider moving this statement to an else block
(TRY300)
24-26: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
24-26: Avoid specifying long messages outside the exception class
(TRY003)
60-62: Avoid specifying long messages outside the exception class
(TRY003)
81-83: Avoid specifying long messages outside the exception class
(TRY003)
94-96: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (13)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
🔇 Additional comments (3)
openwisp_utils/releaser/tests/conftest.py (1)
65-150: Nice fixture scaffolding for multi-ecosystem tests.Clear, reusable helpers that keep tests focused and reduce duplication.
openwisp_utils/releaser/tests/test_version_bumping.py (1)
159-297: Good multi-package bumping coverage.These tests exercise the new package-type branches well and should catch regressions early.
openwisp_utils/releaser/tests/test_config.py (1)
180-429: Comprehensive package-type detection tests look solid.Covers detection, edge cases, and priority rules thoroughly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
openwisp_utils/releaser/config.py
Outdated
| parts = version_tuple.split(".") | ||
| if len(parts) < 3: | ||
| raise ValueError( | ||
| f"Version '{version_str}' does not have expected 3 parts (X.Y.Z)" | ||
| ) | ||
|
|
||
| config["CURRENT_VERSION"] = [ | ||
| int(parts[0]), | ||
| int(parts[1]), | ||
| int(parts[2]), | ||
| version_type, | ||
| ] |
There was a problem hiding this comment.
Reject versions with more than 3 parts (avoid silent truncation).
The current len(parts) < 3 check accepts 1.2.3.4 and silently drops the extra segment, which misrepresents the current version and can lead to incorrect bumps. Require exactly three parts here (and apply the same rule to the docker/ansible/openwrt branches).
🛠️ Suggested fix (apply similarly in other branches)
- if len(parts) < 3:
+ if len(parts) != 3:
raise ValueError(
f"Version '{version_str}' does not have expected 3 parts (X.Y.Z)"
)📝 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.
| parts = version_tuple.split(".") | |
| if len(parts) < 3: | |
| raise ValueError( | |
| f"Version '{version_str}' does not have expected 3 parts (X.Y.Z)" | |
| ) | |
| config["CURRENT_VERSION"] = [ | |
| int(parts[0]), | |
| int(parts[1]), | |
| int(parts[2]), | |
| version_type, | |
| ] | |
| parts = version_tuple.split(".") | |
| if len(parts) != 3: | |
| raise ValueError( | |
| f"Version '{version_str}' does not have expected 3 parts (X.Y.Z)" | |
| ) | |
| config["CURRENT_VERSION"] = [ | |
| int(parts[0]), | |
| int(parts[1]), | |
| int(parts[2]), | |
| version_type, | |
| ] |
🧰 Tools
🪛 Ruff (0.14.14)
117-119: Abstract raise to an inner function
(TRY301)
117-119: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@openwisp_utils/releaser/config.py` around lines 115 - 126, The current check
in the version parsing logic accepts versions with fewer than 3 parts but
silently truncates ones with more than 3; update the validation around
version_tuple/version_str so it requires exactly 3 dot-separated parts
(len(parts) == 3) and raises a ValueError with a clear message if not, before
populating config["CURRENT_VERSION"]; apply the same exact-3-parts validation to
the equivalent parsing logic for the docker/ansible/openwrt branches as well.
pushpitkamboj
force-pushed
the
feature/multi-package-support
branch
from
dc1d19f to
136275c
Compare
…penwisp#522 - Added package type detection for npm, docker, ansible, and openwrt-agents - Enhanced version parsing for all package types - Updated bump_version() to handle all package type version formats - Added 25 new tests for non-Python packages Fixes openwisp#522
pushpitkamboj
force-pushed
the
feature/multi-package-support
branch
from
136275c to
aec6725
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_utils/releaser/tests/test_version_bumping.py (1)
50-54: Test logic is outdated after refactoring.This test patches
os.path.existsbutget_current_versionno longer reads from the filesystem—it readsCURRENT_VERSIONdirectly from the config dict. The test passes coincidentally becausemock_config(from the fixture) doesn't have aCURRENT_VERSIONkey, causing(None, None)to be returned.Consider removing this test or updating it to test meaningful behavior. The
test_get_current_version_no_path_in_configandtest_get_current_version_no_tupletests already cover theNonereturn cases.🛠️ Suggested fix: remove the obsolete test
-@patch("os.path.exists", return_value=False) -def test_get_current_version_file_not_found(_, mock_config): - version, version_type = get_current_version(mock_config) - assert version is None - assert version_type is None
🤖 Fix all issues with AI agents
In `@openwisp_utils/releaser/config.py`:
- Around line 100-128: The npm branch swallows a ValueError raised during
version validation because the broad except (ValueError, SyntaxError) around
parsing sets config["CURRENT_VERSION"]=None instead of letting validation errors
propagate; update the code in the config["package_type"] == "npm" block (the
logic reading version_str, splitting into version_tuple/version_type, building
parts and setting config["CURRENT_VERSION"]) to perform the parts length check
and int conversion validation outside or before the try/except, or narrow the
except to only handle JSON/Syntax-related errors and re-raise any ValueError so
that the validation error for malformed version strings (the ValueError from the
parts length/int conversion) is not swallowed and will propagate as in the
docker/openwrt-agents branches.
In `@openwisp_utils/releaser/version.py`:
- Around line 49-102: The code lacks a fallback for an unrecognized
package_type, which can leave new_content undefined; update the control flow
after the existing elif blocks to add an else branch that raises a clear
RuntimeError (or ValueError) referencing package_type and version_path, ensuring
new_content and count are not used when package_type is unknown (i.e., in the
same function where package_type, new_content, count, and version_path are
referenced).
- Around line 98-102: The branch handling package_type == "openwrt-agents"
contains dead code: it sets count = 1 then immediately checks if count == 0,
which can never occur; remove the unnecessary count variable and the unreachable
if/raise block in the openwrt-agents branch (the code that assigns new_content =
f"{new_version}\n" should remain and any subsequent write logic should proceed
without the redundant count/error check), referencing the package_type,
new_content, count and version_path identifiers to locate and cleanly remove the
dead check.
🧹 Nitpick comments (1)
openwisp_utils/releaser/version.py (1)
21-21: Avoid shadowing the builtintype.Using
typeas a variable name shadows Python's built-intype()function, which can cause subtle issues iftype()is needed later in the same scope.♻️ Suggested rename
- type = current_version[3] if len(current_version) > 3 else "final" - return f"{major}.{minor}.{patch}", type + version_type = current_version[3] if len(current_version) > 3 else "final" + return f"{major}.{minor}.{patch}", version_type
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_utils/releaser/config.pyopenwisp_utils/releaser/tests/conftest.pyopenwisp_utils/releaser/tests/test_config.pyopenwisp_utils/releaser/tests/test_version_bumping.pyopenwisp_utils/releaser/version.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_utils/releaser/tests/test_version_bumping.py (2)
openwisp_utils/releaser/tests/conftest.py (1)
mock_config(16-17)openwisp_utils/releaser/version.py (2)
get_current_version(7-26)bump_version(29-106)
openwisp_utils/releaser/tests/test_config.py (2)
openwisp_utils/releaser/tests/conftest.py (12)
project_dir(10-12)create_package_json(78-79)create_changelog(49-50)init_git_repo(61-62)create_docker_compose(106-107)create_makefile(94-95)create_ansible_lint(116-117)create_ansible_version_file(128-129)create_luacheckrc(138-139)create_version_file(148-149)create_setup_py(27-28)create_package_dir_with_version(40-41)openwisp_utils/releaser/config.py (1)
load_config(57-220)
🪛 Ruff (0.14.14)
openwisp_utils/releaser/config.py
117-119: Abstract raise to an inner function
(TRY301)
117-119: Avoid specifying long messages outside the exception class
(TRY003)
141-143: Avoid specifying long messages outside the exception class
(TRY003)
166-168: Abstract raise to an inner function
(TRY301)
166-168: Avoid specifying long messages outside the exception class
(TRY003)
185-187: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_utils/releaser/version.py
22-22: Consider moving this statement to an else block
(TRY300)
24-26: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
24-26: Avoid specifying long messages outside the exception class
(TRY003)
60-62: Avoid specifying long messages outside the exception class
(TRY003)
81-83: Avoid specifying long messages outside the exception class
(TRY003)
94-96: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (14)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (5)
openwisp_utils/releaser/config.py (1)
21-38: LGTM! Clean package type detection logic.The priority-based detection using file existence checks is clear and maintainable. The order (python → npm → docker → ansible → openwrt-agents) ensures Python projects take precedence when multiple indicators exist.
openwisp_utils/releaser/tests/conftest.py (1)
65-149: LGTM! Well-structured test fixtures.The fixture factory pattern (returning helper functions) is appropriate for parameterized test setup. Each helper is focused and documented. The artifact creation logic correctly mirrors the expected file formats in the production code.
openwisp_utils/releaser/tests/test_config.py (2)
239-251: Test expectation may be inconsistent with other package types.This test expects
CURRENT_VERSION = Nonefor an invalid 2-part npm version, but the docker (line 296) and openwrt-agents (line 394) tests expect aValueErrorto be raised for the same scenario.If the intent is consistent validation across all package types, this test should use
pytest.raises(ValueError). However, if npm should gracefully handle invalid versions (as the current implementation does due to the try-except), the config.py behavior for npm differs from docker/openwrt-agents—which may be intentional or a bug.Please verify whether npm packages should raise
ValueErrorfor invalid versions (consistent with docker/openwrt-agents) or gracefully returnNone(current behavior). This relates to the exception handling issue noted in config.py.
180-429: Good coverage for new package types.The test suite comprehensively covers:
- Detection logic for each package type
- Valid version parsing with different formats (prerelease, underscore suffix)
- Graceful handling of missing version files
- Error cases for invalid version formats
- Priority ordering when multiple indicators exist
openwisp_utils/releaser/tests/test_version_bumping.py (1)
159-297: Good test coverage for new package types.The tests properly verify:
get_current_versionextracts version strings from theCURRENT_VERSIONlistbump_versioncorrectly modifies file content based onpackage_type- Different version types (final, beta, prerelease) are handled
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_utils/releaser/tests/test_version_bumping.py (1)
50-54: Test may be testing outdated behavior.
test_get_current_version_file_not_foundpatchesos.path.existsto returnFalse, butget_current_version()no longer performs file I/O—it reads fromconfig["CURRENT_VERSION"]. Sincemock_configdoesn't setCURRENT_VERSION, the test passes but for the wrong reason (missing key, not missing file).Consider updating the test name and removing the unnecessary patch, or removing this test if it's redundant with
test_get_current_version_no_tuple.♻️ Suggested fix: simplify the test
-@patch("os.path.exists", return_value=False) -def test_get_current_version_file_not_found(_, mock_config): +def test_get_current_version_missing_current_version_key(mock_config): + """Tests get_current_version when CURRENT_VERSION key is absent.""" version, version_type = get_current_version(mock_config) assert version is None assert version_type is None
🤖 Fix all issues with AI agents
In `@openwisp_utils/releaser/config.py`:
- Around line 129-149: Unify version-parsing error handling by wrapping the
Docker and openwrt-agents parsing blocks (the logic that reads Makefile, finds
version_match via re.search, splits version_str and builds
config["CURRENT_VERSION"]) in the same try/except pattern used by npm/ansible:
on any parsing error (missing match or not three integer parts) set
config["CURRENT_VERSION"] = None instead of raising ValueError; ensure you still
set config["version_path"] when a valid version is found and only populate the
list [major, minor, patch, "final"] on successful int conversion.
🧹 Nitpick comments (2)
openwisp_utils/releaser/version.py (2)
21-21: Avoid shadowing the built-intype.Using
typeas a variable name shadows Python's built-intype()function. Consider renaming toversion_typefor clarity and to avoid potential confusion.♻️ Suggested fix
- type = current_version[3] if len(current_version) > 3 else "final" - return f"{major}.{minor}.{patch}", type + version_type = current_version[3] if len(current_version) > 3 else "final" + return f"{major}.{minor}.{patch}", version_type
64-69: Consider movingimport jsonto the top of the file.The
jsonimport inside the function works but is unconventional. Moving it to the module level improves readability and follows Python conventions.♻️ Suggested fix
At the top of the file, add:
import jsonThen remove line 65:
elif package_type == "npm": - import json - data = json.loads(content)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_utils/releaser/config.pyopenwisp_utils/releaser/tests/conftest.pyopenwisp_utils/releaser/tests/test_config.pyopenwisp_utils/releaser/tests/test_version_bumping.pyopenwisp_utils/releaser/version.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_utils/releaser/tests/test_config.py (2)
openwisp_utils/releaser/tests/conftest.py (12)
project_dir(10-12)create_package_json(78-79)create_changelog(49-50)init_git_repo(61-62)create_docker_compose(106-107)create_makefile(94-95)create_ansible_lint(116-117)create_ansible_version_file(128-129)create_luacheckrc(138-139)create_version_file(148-149)create_setup_py(27-28)create_package_dir_with_version(40-41)openwisp_utils/releaser/config.py (1)
load_config(57-220)
openwisp_utils/releaser/tests/test_version_bumping.py (2)
openwisp_utils/releaser/tests/conftest.py (1)
mock_config(16-17)openwisp_utils/releaser/version.py (2)
get_current_version(7-26)bump_version(29-106)
🪛 Ruff (0.14.14)
openwisp_utils/releaser/config.py
117-119: Abstract raise to an inner function
(TRY301)
117-119: Avoid specifying long messages outside the exception class
(TRY003)
141-143: Avoid specifying long messages outside the exception class
(TRY003)
166-168: Abstract raise to an inner function
(TRY301)
166-168: Avoid specifying long messages outside the exception class
(TRY003)
185-187: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_utils/releaser/version.py
22-22: Consider moving this statement to an else block
(TRY300)
24-26: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
24-26: Avoid specifying long messages outside the exception class
(TRY003)
60-62: Avoid specifying long messages outside the exception class
(TRY003)
81-83: Avoid specifying long messages outside the exception class
(TRY003)
94-96: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (1)
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (11)
openwisp_utils/releaser/config.py (2)
21-38: LGTM!The
get_package_type_from_setup()function provides a clean, prioritized detection mechanism for package types based on file existence. The priority order (python > npm > docker > ansible > openwrt-agents) is well-structured.
82-99: LGTM!The Python package version extraction maintains the existing behavior with proper error handling for malformed VERSION tuples.
openwisp_utils/releaser/tests/test_config.py (5)
180-251: LGTM!The npm package tests provide good coverage for version detection, prerelease suffix handling (both
-and_separators), missing version handling, and invalid version format handling. The test expectations correctly align with the implementation behavior.
253-298: LGTM!Docker package tests verify detection via
docker-compose.yml, version extraction from Makefile, and proper error handling for missing/invalid versions. The test correctly expectsValueErrorfor invalid versions, matching the current implementation.
300-351: LGTM!Ansible package tests verify detection via
.ansible-lint, version extraction fromtemplates/openwisp2/version.py, and graceful handling of missing/malformed versions. The expectations align with the implementation.
353-396: LGTM!OpenWRT agents tests verify detection via
.luacheckrc, version extraction fromVERSIONfile, and proper error handling. The test correctly expectsValueErrorfor invalid versions, matching the current implementation.
398-429: LGTM!Priority and fallback tests ensure correct behavior when multiple package indicators exist and when no package type is detected. Good edge case coverage.
openwisp_utils/releaser/tests/test_version_bumping.py (3)
159-201: LGTM!NPM package tests provide good coverage for
get_current_version(including prerelease handling) andbump_versionwith JSON content manipulation.
203-266: LGTM!Docker and Ansible package tests verify version retrieval from config and version bumping via regex substitution in respective files.
268-297: LGTM!OpenWRT agents tests verify version retrieval and the file content replacement behavior of
bump_version.openwisp_utils/releaser/tests/conftest.py (1)
65-149: LGTM!The test helper functions and fixtures follow a consistent pattern and provide good support for testing multiple package ecosystems. The parameterization with default versions enables flexible test scenarios.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
openwisp_utils/releaser/config.py
Outdated
| elif config["package_type"] == "docker": | ||
| if os.path.exists("Makefile"): | ||
| with open("Makefile", "r") as f: | ||
| content = f.read() | ||
| version_match = re.search( | ||
| r"^OPENWISP_VERSION\s*=\s*([^\s]+)", content, re.MULTILINE | ||
| ) | ||
| if version_match: | ||
| config["version_path"] = "Makefile" | ||
| version_str = version_match.group(1) | ||
| parts = version_str.split(".") | ||
| if len(parts) < 3: | ||
| raise ValueError( | ||
| f"Version '{version_str}' does not have expected 3 parts (X.Y.Z)" | ||
| ) | ||
| config["CURRENT_VERSION"] = [ | ||
| int(parts[0]), | ||
| int(parts[1]), | ||
| int(parts[2]), | ||
| "final", | ||
| ] |
There was a problem hiding this comment.
Inconsistent error handling for invalid versions across package types.
The docker (lines 140-143) and openwrt-agents (lines 184-187) branches raise ValueError directly when the version has fewer than 3 parts, while npm (line 127) and ansible (line 175) catch the error and set CURRENT_VERSION = None. This inconsistency means some invalid versions crash load_config() while others are handled gracefully.
Consider unifying the behavior: either all package types should raise errors for invalid versions, or all should handle them gracefully.
♻️ Suggested approach: wrap docker and openwrt-agents parsing in try-except
elif config["package_type"] == "docker":
if os.path.exists("Makefile"):
with open("Makefile", "r") as f:
content = f.read()
version_match = re.search(
r"^OPENWISP_VERSION\s*=\s*([^\s]+)", content, re.MULTILINE
)
if version_match:
config["version_path"] = "Makefile"
- version_str = version_match.group(1)
- parts = version_str.split(".")
- if len(parts) < 3:
- raise ValueError(
- f"Version '{version_str}' does not have expected 3 parts (X.Y.Z)"
- )
- config["CURRENT_VERSION"] = [
- int(parts[0]),
- int(parts[1]),
- int(parts[2]),
- "final",
- ]
+ try:
+ version_str = version_match.group(1)
+ parts = version_str.split(".")
+ if len(parts) != 3:
+ raise ValueError(
+ f"Version '{version_str}' does not have expected 3 parts (X.Y.Z)"
+ )
+ config["CURRENT_VERSION"] = [
+ int(parts[0]),
+ int(parts[1]),
+ int(parts[2]),
+ "final",
+ ]
+ except (ValueError, SyntaxError):
+ config["CURRENT_VERSION"] = NoneAlso applies to: 177-193
🧰 Tools
🪛 Ruff (0.14.14)
141-143: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@openwisp_utils/releaser/config.py` around lines 129 - 149, Unify
version-parsing error handling by wrapping the Docker and openwrt-agents parsing
blocks (the logic that reads Makefile, finds version_match via re.search, splits
version_str and builds config["CURRENT_VERSION"]) in the same try/except pattern
used by npm/ansible: on any parsing error (missing match or not three integer
parts) set config["CURRENT_VERSION"] = None instead of raising ValueError;
ensure you still set config["version_path"] when a valid version is found and
only populate the list [major, minor, patch, "final"] on successful int
conversion.
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)
openwisp_utils/releaser/tests/test_version_bumping.py (1)
50-54: Test is now testing incorrect behavior.
get_current_versionno longer reads from a file - it reads fromconfig["CURRENT_VERSION"]. Theos.path.existspatch has no effect on this function. Additionally,mock_configdoesn't includeCURRENT_VERSION, so this test passes for the wrong reason (returnsNone, NonebecauseCURRENT_VERSIONis missing, not because the file doesn't exist).This test should either be removed or updated to test a meaningful scenario.
Suggested fix: remove or update the test
-@patch("os.path.exists", return_value=False) -def test_get_current_version_file_not_found(_, mock_config): - version, version_type = get_current_version(mock_config) - assert version is None - assert version_type is NoneOr update to test meaningful behavior:
def test_get_current_version_missing_current_version(): """Tests that get_current_version returns None when CURRENT_VERSION is not in config.""" config = {"version_path": "path/__init__.py"} version, version_type = get_current_version(config) assert version is None assert version_type is None
🤖 Fix all issues with AI agents
In `@openwisp_utils/releaser/tests/test_config.py`:
- Around line 336-350: The test test_ansible_package_malformed_version is
inconsistent with other malformed-version tests: update it to expect a
ValueError instead of CURRENT_VERSION == None by invoking load_config() inside a
pytest.raises(ValueError) context (referencing the test function name and the
load_config call) so it matches the docker and openwrt-agents tests and the
intended exception behavior in config.py.
- Around line 239-250: Update the test_npm_package_invalid_version to expect a
ValueError instead of asserting CURRENT_VERSION is None: call load_config inside
a with pytest.raises(ValueError) context (targeting the test function
test_npm_package_invalid_version and the load_config invocation) so the test
aligns with other invalid-version tests (e.g.,
test_docker_package_invalid_version and test_openwrt_agents_invalid_version) and
the unified exception behavior in config.py.
🧹 Nitpick comments (3)
openwisp_utils/releaser/version.py (2)
21-22: Avoid shadowing built-intype.Using
typeas a variable name shadows the Python built-in. Consider renaming toversion_typefor consistency with how it's used elsewhere.Suggested fix
- type = current_version[3] if len(current_version) > 3 else "final" - return f"{major}.{minor}.{patch}", type + version_type = current_version[3] if len(current_version) > 3 else "final" + return f"{major}.{minor}.{patch}", version_type
64-70: npm bump: consider preserving original JSON formatting.The current implementation uses
json.dumps(data, indent=2)which may reformat the entire file (e.g., changing key ordering, spacing). If the original file used different indentation or had trailing commas, this could cause unnecessary diff noise.openwisp_utils/releaser/tests/test_version_bumping.py (1)
23-25: Fixturemock_configis missingCURRENT_VERSIONkey.The
mock_configfixture only providesversion_pathbut many tests now requireCURRENT_VERSIONandpackage_type. Tests using this fixture withget_current_versionwill returnNone, NonebecauseCURRENT_VERSIONis missing.Consider updating the fixture or using inline config dicts (as done in the new tests).
Suggested update
`@pytest.fixture` def mock_config(): - return {"version_path": "path/__init__.py"} + return { + "version_path": "path/__init__.py", + "package_type": "python", + "CURRENT_VERSION": [1, 2, 0, "alpha"], + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_utils/releaser/config.pyopenwisp_utils/releaser/tests/conftest.pyopenwisp_utils/releaser/tests/test_config.pyopenwisp_utils/releaser/tests/test_version_bumping.pyopenwisp_utils/releaser/version.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_utils/releaser/tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (1)
openwisp_utils/releaser/tests/test_version_bumping.py (1)
openwisp_utils/releaser/version.py (2)
get_current_version(7-26)bump_version(29-106)
🪛 Ruff (0.14.14)
openwisp_utils/releaser/config.py
117-119: Abstract raise to an inner function
(TRY301)
117-119: Avoid specifying long messages outside the exception class
(TRY003)
141-143: Avoid specifying long messages outside the exception class
(TRY003)
166-168: Abstract raise to an inner function
(TRY301)
166-168: Avoid specifying long messages outside the exception class
(TRY003)
185-187: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_utils/releaser/version.py
22-22: Consider moving this statement to an else block
(TRY300)
24-26: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
24-26: Avoid specifying long messages outside the exception class
(TRY003)
60-62: Avoid specifying long messages outside the exception class
(TRY003)
81-83: Avoid specifying long messages outside the exception class
(TRY003)
94-96: Avoid specifying long messages outside the exception class
(TRY003)
102-102: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
openwisp_utils/releaser/config.py (1)
21-38: LGTM on package type detection.The detection logic with clear file-based heuristics is clean. The priority order (python → npm → docker → ansible → openwrt-agents) is sensible given python is most common.
openwisp_utils/releaser/tests/test_config.py (1)
180-429: Good comprehensive test coverage for new package types.The new tests thoroughly cover:
- Detection of each package type
- Version extraction with various formats (prerelease suffixes, underscores)
- Missing version files
- Invalid version formats
- Package type priority
openwisp_utils/releaser/tests/test_version_bumping.py (1)
159-297: Good test coverage for per-package-type version operations.The new tests comprehensively cover version retrieval and bumping for all supported package types (npm, docker, ansible, openwrt-agents). The tests properly mock file I/O and verify the written content.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
pushpitkamboj
changed the title
pushpitkamboj
changed the title
|
hi, please resolve code rabbit's comments by either:
Or,
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Good progress @pushpitkamboj, it looks promising!
I haven't tested it manually first as it's gonna be quite time consuming but will do asap. Let's review the code in the meantime.
Please stop adding so many blank lines, if you do a quick search you'll find out why adding many blank lines is a bad habit.
openwisp_utils/releaser/config.py
Outdated
| int(parts[1]), | ||
| int(parts[2]), | ||
| "final", | ||
| ] |
There was a problem hiding this comment.
lots of if statements here, would be good to write functions to handle each case and do something like:
package_type_map = {
"openwrt": _handle_openwrt
}
package_type_map[config["package_type"]](args..)
openwisp_utils/releaser/version.py
Outdated
| raise RuntimeError( | ||
| f"Failed to find and bump VERSION tuple in {version_path}." | ||
| ) | ||
|
|
There was a problem hiding this comment.
please remove useless blank lines like this, this PR is full of it, use blank lines sparingly, please don't have me have to point out all the blank lines one by one.
openwisp_utils/releaser/version.py
Outdated
|
|
||
| if count == 0: | ||
| raise RuntimeError(f"Failed to find and bump VERSION tuple in {version_path}.") | ||
| if package_type == "python": |
There was a problem hiding this comment.
same regardin if statements here
openwisp_utils/releaser/config.py
Outdated
| return "ansible" | ||
|
|
||
| if os.path.exists(".luacheckrc"): | ||
| return "openwrt-agents" |
There was a problem hiding this comment.
| return "openwrt-agents" | |
| return "openwrt" |
|
Thank you and I am really sorry for the blank lines thing, it's my mistake. It won't bother you again on my PRs :) |
…atch Replaced if/elif chains with handler functions and dictionary mapping for better maintainability.
|
I have made the required changes, please review them @nemesifier |
openwisp_utils/releaser/version.py
Outdated
|
|
||
| with open(version_path, "w") as f: | ||
| f.write(new_content) | ||
|
|
There was a problem hiding this comment.
avoid unnecessary blank lines
openwisp_utils/releaser/config.py
Outdated
| raise ValueError( | ||
| f"Version '{version_str}' does not have expected 3 parts (X.Y.Z)" | ||
| ) | ||
|
|
There was a problem hiding this comment.
avoid unnecessary blank lines
openwisp_utils/releaser/config.py
Outdated
| raise ValueError( | ||
| f"Version '{version_str}' does not have expected 3 parts (X.Y.Z)" | ||
| ) | ||
|
|
There was a problem hiding this comment.
avoid unnecessary blank lines, try to slash the amount of blank lines without needing me to point out all of them. Use blank lines sparingly. Adding blank lines does not improve the code or make it more readable.
pushpitkamboj
force-pushed
the
feature/multi-package-support
branch
from
324976e to
35b896f
Compare
|
removed all the blank lines. I think this can be tested manually now! |
nemesifier
changed the title
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I am going to do a round manual testing, in the meantime I want to share a few other comments.
Make sure to follow up with @coderabbitai's comments please.
| init_git_repo(project_dir) | ||
|
|
||
| config = load_config() | ||
|
|
There was a problem hiding this comment.
please remove blank lines in test bodies, use them sparingly, there's too many
openwisp_utils/releaser/version.py
Outdated
| "npm": _bump_npm_version, | ||
| "docker": _bump_docker_version, | ||
| "ansible": _bump_ansible_version, | ||
| "openwrt-agents": _bump_openwrt_agents_version, |
There was a problem hiding this comment.
I asked to change this for consistency:
| "openwrt-agents": _bump_openwrt_agents_version, | |
| "openwrt": _bump_openwrt_agents_version, |
There was a problem hiding this comment.
yes, sorry I did changed the name in the code but forgot to change them in test cases.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I did a round of manual testing on docker-openwisp which needed a new release and it went well, the tool automatically figured out the version number 👏 .
Please follow up with the previous comments when you can and as soon as everything is addressed I'll book some time to test it on npm, openwrt and ansible packages.
pushpitkamboj
force-pushed
the
feature/multi-package-support
branch
from
c6948b5 to
9a6bc49
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_utils/releaser/version.py`:
- Around line 56-60: The _bump_npm_version function should catch JSON decoding
errors when calling json.loads(content) and raise a clearer error that includes
the affected file path (version_path) and the original exception message; wrap
json.loads in a try-except for json.JSONDecodeError, and in the except block
raise or re-raise a new ValueError or RuntimeError with a message like
"Malformed JSON in {version_path}: {original_error}" so callers get informative
context while preserving the original exception details.
🧹 Nitpick comments (1)
openwisp_utils/releaser/version.py (1)
23-26: Chain the exception for better debugging context.Per Python best practices (B904), exceptions raised within an
exceptblock should be chained usingraise ... from errto preserve the original traceback.♻️ Suggested improvement
- except IndexError: - raise RuntimeError( + except IndexError as err: + raise RuntimeError( f"The VERSION tuple {current_version} does not appear to have at least three elements." - ) + ) from err
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_utils/releaser/config.pyopenwisp_utils/releaser/tests/conftest.pyopenwisp_utils/releaser/tests/test_config.pyopenwisp_utils/releaser/tests/test_version_bumping.pyopenwisp_utils/releaser/version.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_utils/releaser/tests/test_config.py (2)
openwisp_utils/releaser/tests/conftest.py (12)
project_dir(11-13)create_package_json(79-80)create_changelog(50-51)init_git_repo(62-63)create_docker_compose(107-108)create_makefile(95-96)create_ansible_lint(117-118)create_ansible_version_file(129-130)create_luacheckrc(139-140)create_version_file(149-150)create_setup_py(28-29)create_package_dir_with_version(41-42)openwisp_utils/releaser/config.py (1)
load_config(189-241)
openwisp_utils/releaser/tests/test_version_bumping.py (1)
openwisp_utils/releaser/version.py (2)
get_current_version(8-26)bump_version(100-122)
🪛 Ruff (0.14.14)
openwisp_utils/releaser/version.py
[warning] 22-22: Consider moving this statement to an else block
(TRY300)
[warning] 24-26: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 24-26: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 39-39: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 56-56: Unused function argument: version_path
(ARG001)
[warning] 85-85: Unused function argument: content
(ARG001)
[warning] 85-85: Unused function argument: version_path
(ARG001)
[warning] 110-110: Abstract raise to an inner function
(TRY301)
[warning] 110-110: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 118-118: Avoid specifying long messages outside the exception class
(TRY003)
openwisp_utils/releaser/config.py
[warning] 91-93: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 117-119: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 146-148: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 168-170: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (14)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
🔇 Additional comments (15)
openwisp_utils/releaser/tests/conftest.py (1)
66-150: LGTM! Well-structured test fixtures.The new helper functions and fixtures follow a consistent pattern and provide good coverage for all supported package types. The implementation is clean and enables comprehensive testing of the multi-package support feature.
One minor note: the
jsonimport at line 68 is inside the function rather than at module level. This is acceptable for test fixtures to avoid unnecessary imports when the fixture isn't used, though moving it to the top would be more conventional.openwisp_utils/releaser/version.py (2)
85-97: LGTM! Clean handler-based architecture.The dispatcher pattern with
VERSION_BUMP_HANDLERSprovides a clean, extensible design for supporting multiple package types. The unused parameters in_bump_openwrt_version(and_bump_npm_version) maintain API consistency across all handlers, which is appropriate.
116-122: LGTM! Good error handling for unknown package types.The check for unknown
package_typeat lines 117-118 addresses the previously flagged issue and ensures a clear error message when an unrecognized package type is encountered.openwisp_utils/releaser/config.py (3)
52-70: LGTM! Appropriate error handling for Python version parsing.The Python handler correctly uses try-except around
ast.literal_evalto handle edge cases with malformed VERSION tuples, settingCURRENT_VERSION = Noneinstead of crashing. This is appropriate sinceliteral_evalcan fail on syntactically valid but unevaluable expressions.
179-186: LGTM! Clean dispatcher pattern.The
PACKAGE_VERSION_HANDLERSmapping provides a clean and extensible way to handle different package types, aligning well with the similar pattern inversion.py.
210-214: LGTM! Dispatcher integration in load_config.The integration of the handler-based approach into
load_config()is clean and straightforward, replacing the previous inline version extraction logic.openwisp_utils/releaser/tests/test_version_bumping.py (4)
33-41: LGTM! Clean test refactoring.The test now uses an inline
mock_configwithCURRENT_VERSIONdirectly, which is cleaner and aligns with the new architecture whereget_current_versionreads from config rather than parsing files.
80-95: LGTM! Good edge case coverage.The tests for
CURRENT_VERSION = Noneand short tuples properly validate the error handling inget_current_version.
159-198: LGTM! Comprehensive npm package tests.Good coverage of npm version handling including prerelease versions and the bump operation.
200-286: LGTM! Complete per-package-type test coverage.The tests for docker, ansible, and openwrt package types follow a consistent pattern and provide good coverage of both version reading and bumping operations.
openwisp_utils/releaser/tests/test_config.py (5)
169-228: LGTM! Comprehensive npm package test coverage.Good coverage including prerelease suffixes (
-beta), underscore suffixes (_rc1), missing version handling, and invalid version format detection. The error handling test now correctly expectsValueErrorfor invalid versions.
231-270: LGTM! Docker package tests.Proper coverage of docker package detection, handling of missing Makefile, and invalid version format.
273-316: LGTM! Ansible package tests with consistent error handling.The tests properly cover ansible package detection and the malformed version test now expects
ValueError, aligning with the other package types.
319-356: LGTM! OpenWRT package tests.Good coverage of openwrt package detection and version validation.
359-385: LGTM! Package type priority and edge case tests.The priority test confirms Python takes precedence over npm when both indicators exist, and the no-package-type test validates graceful handling when no recognizable package type is found.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| def _bump_npm_version(content, new_version, version_path): | ||
| """Handles version bumping for NPM packages.""" | ||
| data = json.loads(content) | ||
| data["version"] = new_version | ||
| return json.dumps(data, indent=2) + "\n" |
There was a problem hiding this comment.
Add error handling for malformed JSON in npm version bumping.
If package.json contains malformed JSON, json.loads(content) will raise a JSONDecodeError that propagates without a clear message. Consider wrapping this in a try-except for a more informative error.
🛠️ Suggested fix
def _bump_npm_version(content, new_version, version_path):
"""Handles version bumping for NPM packages."""
- data = json.loads(content)
- data["version"] = new_version
- return json.dumps(data, indent=2) + "\n"
+ try:
+ data = json.loads(content)
+ except json.JSONDecodeError as e:
+ raise RuntimeError(f"Failed to parse {version_path}: {e}") from e
+ data["version"] = new_version
+ return json.dumps(data, indent=2) + "\n"🧰 Tools
🪛 Ruff (0.14.14)
[warning] 56-56: Unused function argument: version_path
(ARG001)
🤖 Prompt for AI Agents
In `@openwisp_utils/releaser/version.py` around lines 56 - 60, The
_bump_npm_version function should catch JSON decoding errors when calling
json.loads(content) and raise a clearer error that includes the affected file
path (version_path) and the original exception message; wrap json.loads in a
try-except for json.JSONDecodeError, and in the except block raise or re-raise a
new ValueError or RuntimeError with a message like "Malformed JSON in
{version_path}: {original_error}" so callers get informative context while
preserving the original exception details.
- Updated _handle_npm_version and _handle_ansible_version to raise ValueError instead of silently setting CURRENT_VERSION to None - Renamed 'openwrt-agents' to 'openwrt' for consistency across config.py, version.py, and tests - Updated test_npm_package_invalid_version and test_ansible_package_malformed_version to expect ValueError
pushpitkamboj
force-pushed
the
feature/multi-package-support
branch
from
6eacd80 to
a26d6e5
Compare
|
I have addressed all the issues and resolved them, You can try testing with other packages too now! |
4 participants
Checklist
Description of Changes
Closes [feature] Allow releaser tool to bump version of non python modules #522