fix(plugins): replace ansible.utils.ipaddr with stdlib-based test plugin#24
fix(plugins): replace ansible.utils.ipaddr with stdlib-based test plugin#24
Conversation
The role used ansible.utils.ipaddr to validate master node IPs, which requires the netaddr Python package. netaddr is not installed by ansible-galaxy collection install and was not documented as a prerequisite, causing runtime failures for users who followed the README. Replace the single ipaddr usage with a new test plugin that uses the Python standard library ipaddress module. The plugin has no external dependencies and supports both IPv4 and IPv6. - Add plugins/test/ip_address.py with is_ip_address test - Use cozystack.installer.is_ip_address in compute-master-nodes.yml - Drop ansible.utils collection dependency (galaxy.yml, requirements.yml) - Drop netaddr pip install and ansible.utils install from CI - Add IPv6 inventory fixture and CI step to verify IPv6 acceptance Closes #18 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
…Pv6 fix - Replace fe80::1 with 2001:db8::12 in IPv6 test inventory: link-local addresses without scope ID are not realistic cluster IPs and make the test misleading. - Add unit tests for plugins/test/ip_address.py with ansible-test units coverage for IPv4, IPv6, hostnames, malformed strings, and None input. - Add CI step to run ansible-test units. - Add version_added to plugin DOCUMENTATION. - Document the dependency drop and new test in CHANGELOG.rst. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- is_ip_address now rejects non-string inputs. ipaddress.ip_address() silently accepts int/bool (treating them as 32-bit address values), violating the documented type: str contract. - Bump collection version to 1.2.3 to match CHANGELOG entry and plugin version_added so ansible-test sanity passes. - Unit tests cover int, bool, bytes, and list rejection. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Update examples/*/requirements.yml from 1.2.2 to v1.2.3. Also adopts the v prefix — git tags are v-prefixed and installs via type: git fail without it. - Fill in missing CHANGELOG entries for v1.1.3, v1.2.1 and v1.2.2 so the v1.2.3 entry does not appear to jump from v1.1.2. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
- Add empty __init__.py to tests/unit/, tests/unit/plugins/ and tests/unit/plugins/test/. ansible-test units (ansible-core >= 2.15) requires these for pytest to resolve the FQCN import of the plugin module, otherwise the new Run unit tests CI step would fail with a collection import error. - Fix stale cozystack_chart_version default in README (1.1.2 to 1.2.2). The actual default in roles/cozystack/defaults/main.yml is 1.2.2. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
ipaddress.ip_address() rejects CIDR notation since CIDR represents a network rather than a single address. Add explicit test cases to document this for both IPv4 and IPv6 CIDR inputs. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 8 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
roles/cozystack/tasks/compute-master-nodes.yml (1)
24-29:⚠️ Potential issue | 🟡 MinorTrim split values before validation to avoid false negatives.
Comma-separated values with spaces (for example,
"10.0.0.1, 10.0.0.2") will currently fail for the second item even when the IP itself is valid.Suggested fix
- loop: "{{ _cozystack_master_nodes.split(',') }}" + loop: "{{ _cozystack_master_nodes.split(',') | map('trim') | list }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roles/cozystack/tasks/compute-master-nodes.yml` around lines 24 - 29, The loop is splitting _cozystack_master_nodes by commas but not trimming whitespace, causing valid IPs like " 10.0.0.2" to fail the is_ip_address test; update the loop to trim each split value before validation (e.g., apply a trim/map to the split results or trim each item before running the is_ip_address check) so the task that references _cozystack_master_nodes and the is_ip_address test validates the cleaned values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@roles/cozystack/tasks/compute-master-nodes.yml`:
- Around line 24-29: The loop is splitting _cozystack_master_nodes by commas but
not trimming whitespace, causing valid IPs like " 10.0.0.2" to fail the
is_ip_address test; update the loop to trim each split value before validation
(e.g., apply a trim/map to the split results or trim each item before running
the is_ip_address check) so the task that references _cozystack_master_nodes and
the is_ip_address test validates the cleaned values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 37fd95f7-73a4-4d99-8af4-18e737411e99
📒 Files selected for processing (15)
.github/workflows/test.ymlCHANGELOG.rstREADME.mdexamples/rhel/requirements.ymlexamples/suse/requirements.ymlexamples/ubuntu/requirements.ymlgalaxy.ymlplugins/test/ip_address.pyrequirements.ymlroles/cozystack/tasks/compute-master-nodes.ymltests/test-ipv6-inventory.ymltests/unit/__init__.pytests/unit/plugins/__init__.pytests/unit/plugins/test/__init__.pytests/unit/plugins/test/test_ip_address.py
💤 Files with no reviewable changes (1)
- requirements.yml
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the cozystack.installer to version 1.2.3, removing dependencies on ansible.utils and netaddr by introducing a bundled is_ip_address Jinja2 test. The changes include IPv6 support for inventory host keys, updated example requirements, and new unit tests for the IP validation plugin. A formatting correction is required in the CHANGELOG.rst to ensure the title underline length matches the text length for valid reStructuredText.
| @@ -2,6 +2,39 @@ | |||
| cozystack.installer Release Notes | |||
| ============================ | |||
There was a problem hiding this comment.
The underline for the main title is too short. In reStructuredText (RST), the underline (and overline, if present) must be at least as long as the title text to be valid. The title 'cozystack.installer Release Notes' is 33 characters long, but the underline is only 28 characters.
| ============================ | |
| ================================= |
There was a problem hiding this comment.
Fixed. The title is 33 chars; both the overline and underline are now 33 = characters to satisfy the RST requirement.
The title 'cozystack.installer Release Notes' is 33 characters, but the overline and underline were only 28. RST requires them to be at least as long as the title, so tooling that parses the file strictly would reject it. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
ansible-test units runs pytest against the controller interpreter and expects pytest (plus the -n auto helpers pytest-xdist/pytest-forked and the commonly-used pytest-mock) to be importable there. Without these the job fails with 'No module named pytest' before any test runs. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
kitsunoff
left a comment
kitsunoff
left a comment
There was a problem hiding this comment.
LGTM. Clean dependency removal — stdlib ipaddress is the right call for a single validation use case. Plugin enforces the type: str contract explicitly, unit test coverage is thorough, and CI no longer needs netaddr. Backfilled CHANGELOG entries are a nice touch.
2 participants
Problem
The role validated master node IPs with
ansible.utils.ipaddr, which requires thenetaddrPython package.ansible-galaxy collection installdoes not pullnetaddrfor users, and the README never documented the requirement — so users following the docs hit a cryptic runtime failure at the validation task.Solution
Replace the single
ipaddrusage with a bundled Jinja2 test plugin backed by Python's standard libraryipaddressmodule. The new plugin has no external dependencies, supports both IPv4 and IPv6, and enforces the documentedtype: strcontract (rejecting int/bool/bytes).Changes
Plugin
plugins/test/ip_address.py—is_ip_addresstest using stdlibcompute-master-nodes.ymltocozystack.installer.is_ip_addressDependencies
ansible.utilsfromgalaxy.ymlandrequirements.ymlnetaddrpip install andansible.utilsinstall from CITests
tests/unit/plugins/test/test_ip_address.pycovering IPv4, IPv6, hostnames, malformed strings, None, int/bool/bytes rejection, and CIDR rejectionansible-test unitsinto the sanity CI jobtests/test-ipv6-inventory.ymlwith documentation-range IPv6 addresses; new CI step verifies IPv6 host keys are acceptedVersioning & docs
galaxy.ymlto1.2.3, addversion_addedto plugin DOCUMENTATIONexamples/*/requirements.ymltov1.2.3(with v prefix matching git tags)cozystack_chart_versiondefault in README (1.1.2 → 1.2.2)Closes #18
Summary by CodeRabbit
New Features
Chores
ansible.utilscollection dependency.Tests