Enable pkg.installed to detect packages by their origin name on FreeBSD #67127
Conversation
| cver = cur_pkgs.get(package_name, []) | ||
|
|
||
| # FreeBSD pkg supports `openjdk` and `java/openjdk7` package names | ||
| origin = bool(re.search('/', package_name)) |
There was a problem hiding this comment.
You don't need a regex to check if a substring is in a string; it can just be has_origin = '/' in package_name. However we might want to use a regex in order to not match cases like /package_name and package_name/
| origin = bool(re.search('/', package_name)) | ||
|
|
||
| if __grains__['os'] == 'FreeBSD' and origin: | ||
| cver = [k for k, v in cur_pkgs.items() if v['origin'] == package_name] |
There was a problem hiding this comment.
There are often hundreds or thousands of packages installed on a system. If someone is attempting to install many of these packages that have a / in the name then iterating over all the packages every time (O(n*m) might not be as performant as one would desire; although it could be an acceptable since there are probably not millions or hundreds of thousands of packages.
There was a problem hiding this comment.
It appears there is a orgins dict stored in the context (
Line 171 in 9233e1c
|
@bdrx312 I appreciate the review and agree with your recommendations, but my approach was just to copy the pattern used elsewhere in the file to the area where it was omitted. Is that sufficient for this fix, or do we need to go back and address the original implementation? |
I am just a contributor that uses the tool, so I try to review pull requests when I have time and try to make improvements to the code so I don't have any authority to say whether a change is sufficient or not. With that said, since your changes seem to be the existing pattern used in other places in this file I think your change should be sufficient for a fix. Ideally someone can improve this in the future though. I would think ideally any places where there are conditional checks for the os should be contained in the os specific module like the pkgng.py for freebsd. |
What does this PR do?
Enable pkg.installed to detect packages by their origin name on FreeBSD
What issues does this PR fix or reference?
Fixes #67126
Previous Behavior
pkg.installedwould not detect packages installed by their origin name when run in test modeNew Behavior
pkg.installedcorrectly detects packages installed by their origin name when run in test mode and reports no changes.Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes