-
Notifications
You must be signed in to change notification settings - Fork 115
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
Stash output from container_manager #790
Conversation
Reviewer's Guide by SourceryThis pull request primarily introduces caching behavior to avoid repeated container engine selection in the 'container_manager' function, alongside various formatting and style improvements across multiple files. The changes are implemented by adding a global variable to stash the engine value and by tweaking code style details in configuration and device setup routines. No diagrams generated as the changes look simple and do not need a visual representation. 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 @rhatdan - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using
functools.cache
instead of manually implementing caching forcontainer_manager
.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 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.
ramalama/common.py
Outdated
global _engine | ||
if _engine != "": | ||
if _engine != "None": | ||
return None | ||
return _engine | ||
|
||
_engine = "None" |
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.
issue (bug_risk): Revisit the caching logic in container_manager.
The current logic checks if _engine is not an empty string and then, if it's not 'None', it returns None even when a valid engine value (e.g. 'podman' or 'docker') was stored. This inverted condition seems unintended. Consider revising the conditional so that a valid cached engine is returned rather than defaulting to None.
Fixes: containers#788 Signed-off-by: Daniel J Walsh <[email protected]>
global _engine | ||
if _engine != "": | ||
if _engine == "None": | ||
return None |
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.
Empty string, "None" and None... Python is frustrating with all the various kinds of null :'(
Fixes: #788
Summary by Sourcery
Bug Fixes: