ipaclient: Increase minimum supported IPA version to 4.6.8#1419#1420
ipaclient: Increase minimum supported IPA version to 4.6.8#1419#1420t-woerner wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new global logger initialization at the bottom of ansible_ipa_client.py uses
logging.getLoggerbutloggingis no longer imported in this refactored path, so you should add a top-levelimport loggingto avoid a NameError at import time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new global logger initialization at the bottom of ansible_ipa_client.py uses `logging.getLogger` but `logging` is no longer imported in this refactored path, so you should add a top-level `import logging` to avoid a NameError at import time.
## Individual Comments
### Comment 1
<location path="roles/ipaclient/module_utils/ansible_ipa_client.py" line_range="259-265" />
<code_context>
+# pylint: enable=too-few-public-methods, useless-object-inheritance
+
+
+# Initialize installer and options
+installer = installer_obj()
+options = installer
+
+# Initialize logger
+logger = logging.getLogger("ipa-client-install")
+root_logger = logger
</code_context>
<issue_to_address>
**issue (bug_risk):** Logger initialization can fail when imports fail, potentially masking the intended import error handling.
With `logger = logging.getLogger("ipa-client-install")` now outside the `try ... except ImportError` block, it will execute even when an earlier import fails and `ANSIBLE_IPA_CLIENT_MODULE_IMPORT_ERROR` is set. If `logging` hasn’t been imported yet, this raises a `NameError` before `check_imports()` can surface the original `ImportError`. To keep the existing error-handling behavior, move logger initialization back inside the main import `try` (after `import logging`) or otherwise ensure it only runs after all imports succeed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # Initialize installer and options | ||
| installer = installer_obj() | ||
| options = installer | ||
|
|
||
| # Initialize logger | ||
| logger = logging.getLogger("ipa-client-install") | ||
| root_logger = logger |
There was a problem hiding this comment.
issue (bug_risk): Logger initialization can fail when imports fail, potentially masking the intended import error handling.
With logger = logging.getLogger("ipa-client-install") now outside the try ... except ImportError block, it will execute even when an earlier import fails and ANSIBLE_IPA_CLIENT_MODULE_IMPORT_ERROR is set. If logging hasn’t been imported yet, this raises a NameError before check_imports() can surface the original ImportError. To keep the existing error-handling behavior, move logger initialization back inside the main import try (after import logging) or otherwise ensure it only runs after all imports succeed.
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. Also to mark the code and special cases with the IPA versions that needed these.
19a5048 to
06b2c18
Compare
| # def __getattribute__(self, attr): | ||
| # value = super(installer_obj, self).__getattribute__(attr) | ||
| # if not attr.startswith("--") and not attr.endswith("--"): | ||
| # logger.debug( | ||
| # " <-- Accessing installer.%s (%s)" % (attr, repr(value))) | ||
| # return value | ||
|
|
||
| # def __getattr__(self, attr): | ||
| # # logger.info(" --> ADDING missing installer.%s" % attr) | ||
| # self.logger.warn(" --> ADDING missing installer.%s" % attr) | ||
| # setattr(self, attr, None) | ||
| # return getattr(self, attr) | ||
|
|
||
| # def __setattr__(self, attr, value): | ||
| # logger.debug(" --> Setting installer.%s to %s" % | ||
| # (attr, repr(value))) | ||
| # return super(installer_obj, self).__setattr__(attr, value) |
There was a problem hiding this comment.
I don't like to have commented out code, specially whithout a comment with the rationale.
rjeffman
left a comment
There was a problem hiding this comment.
In roles/ipaclient/library/ipaclient_api.py the code in lines 126-130 can be removed. The same code is found at lines 347-351 in roles/ipaclient/library/ipaclient_setup_nss.py.
README file should be also changed to reflect new minimum IPA version.
In roles/ipaclient/module_utils/ansible_ipa_client.py, lines 83-87 can be removed.
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. Also to mark the code and special cases with the IPA versions that needed these.
Summary by Sourcery
Raise the minimum supported FreeIPA server version for the ipaclient role and simplify compatibility handling accordingly.
Enhancements: