plugins: Increase minimum supported IPA version to 4.6.8#1419
Conversation
This is preparation work for the SIX removal in Ansible that will drop support for RHEL-7 (Python 2) and together with this support for IPA versions prior to 4.8.4. The goal is to remove code that was needed for IPA versions prior to 4.6.8. Aso to mark the code and special cases with the IPA versions that needed these.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- You compute
IPA_PYTHON_VERSIONfor older FreeIPA releases but then perform the minimum-version check againstNUM_VERSION; ifNUM_VERSIONsemantics differ pre-3.2.1 as implied by the comment, the check should likely useIPA_PYTHON_VERSIONinstead to avoid mis-detecting supported/unsupported versions. - Consider making the
RuntimeErrormessage more explicit by including the minimum supported FreeIPA version (e.g.>= 4.6.8) so users immediately know what they need to upgrade to when the check fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You compute `IPA_PYTHON_VERSION` for older FreeIPA releases but then perform the minimum-version check against `NUM_VERSION`; if `NUM_VERSION` semantics differ pre-3.2.1 as implied by the comment, the check should likely use `IPA_PYTHON_VERSION` instead to avoid mis-detecting supported/unsupported versions.
- Consider making the `RuntimeError` message more explicit by including the minimum supported FreeIPA version (e.g. `>= 4.6.8`) so users immediately know what they need to upgrade to when the check fails.
## Individual Comments
### Comment 1
<location path="plugins/module_utils/ansible_freeipa_module.py" line_range="116-124" />
<code_context>
from ipalib.config import Env
from ipalib.constants import DEFAULT_CONFIG, LDAP_GENERALIZED_TIME_FORMAT
try:
+ # IPA >= 4.12.0
from ipalib.kinit import kinit_password, kinit_keytab
</code_context>
<issue_to_address>
**issue:** The new `is_ipa_configured` import path may break client‑only environments where `ipaserver` is not available.
The previous implementation had a final local fallback for `is_ipa_configured`, so the module still worked when `ipaserver` wasn’t installed (e.g., client‑only setups). The new approach assumes either `ipalib.facts` or `ipaserver.install.installutils` is present and will raise ImportError at import time if both are missing.
If client‑only usage is still required, please add a final fallback implementation or explicit error handling (e.g., degrade gracefully or raise a clear, intentional exception).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@t-woerner I used Rafael’s Code-reviewer Claude skill to review this PR. Here is the review summary. 1. Remove unused
**2. Remove duplicate
Optional: Enhance error message Notes:
|
rjeffman
left a comment
There was a problem hiding this comment.
Shouldn't the README files also note that the minimum supported version is now 4.6.8? Currently we have 4.4.0.
(Yes, this was assisted by Claude. Why not?)
| return True | ||
|
|
||
| return fstore.has_files() | ||
| # IPA >= 3.0.0 |
There was a problem hiding this comment.
It would be better to have # IPA < 4.8.9.
|
|
||
| return fstore.has_files() | ||
| # IPA >= 3.0.0 | ||
| from ipaserver.install.installutils import is_ipa_configured |
There was a problem hiding this comment.
Wouldn't this require ipaserver to be available, which is not the case in clients, for everything from 4.6.x to 4.8.9?
Here's the agent analysis (which I agree):
On client-only nodes running IPA 4.6.8-4.8.8 (no ipaserver package), the removal of the sysrestore fallback for is_ipa_configured causes the entire module import to fail.
| from ipapython.ipautil import run | ||
| from ipapython.ipautil import template_str | ||
| from ipapython.dn import DN | ||
| from ipapython.version import VERSION |
There was a problem hiding this comment.
Due to the inclusion of from ipapython.version import NUM_VERSION, VERSION (line 97), this becomes a duplicate import.
This is preparation work for the SIX removal in Ansible that will drop support for RHEL-7 (Python 2) and together with this support for IPA versions prior to 4.8.4.
The goal is to remove code that was needed for IPA versions prior to 4.6.8. Aso to mark the code and special cases with the IPA versions that needed these.
Summary by Sourcery
Raise the minimum supported FreeIPA version and simplify version-dependent IPA integration logic in the Ansible FreeIPA module utilities.
Bug Fixes:
Enhancements: