-
Notifications
You must be signed in to change notification settings - Fork 233
ipareplica: Don't rely on pkg_resources whenever possible #1350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Python's module "pkg_resources" API has been deprecated in Python 3.12 and will be removed in a future release, and recent FreeIPA versions provide a replacement for pkg_resources.parse_version. To remove ansible-freeipa dependency on pkg_resources and not add a dependency on the 'packaging' module, which is not available in the standard Python distribution, we'll try to import the funcion used in FreeIPA to parse versions, and fallback to pkg_resources when it fails. As an equivalent class is needed, a fallback function is not provided and execution will fail if neither the FreeIPA nor the pkg_resources parse_version function are available. Signed-off-by: Rafael Guterres Jeffman <rjeffman@redhat.com>
Reviewer's Guide by SourceryThis pull request modifies the Updated class diagram for version parsingclassDiagram
class ansible_ipa_replica {
+getargspec(func)
}
note for ansible_ipa_replica "Tries to import parse_version from ipapython.version, falls back to pkg_resources if unavailable."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rjeffman - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining why
pkg_resources
is still imported in theexcept
block.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR changes look good and do not affect the existing tests. All existing test suites have passed with these changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Python's module "pkg_resources" API has been deprecated in Python 3.12 and will be removed in a future release, and recent FreeIPA versions provide a replacement for pkg_resources.parse_version.
To remove ansible-freeipa dependency on pkg_resources and not add a dependency on the 'packaging' module, which is not available in the standard Python distribution, we'll try to import the funcion used in FreeIPA to parse versions, and fallback to pkg_resources when it fails.
As an equivalent class is needed, a fallback function is not provided and execution will fail if neither the FreeIPA nor the pkg_resources parse_version function are available.