Add version_cmp for FreeBSD pkg. #36550
Conversation
| @@ -165,6 +165,10 @@ def _fulfills_version_spec(versions, oper, desired_version, | |||
| otherwise returns False | |||
| ''' | |||
| cmp_func = __salt__.get('pkg.version_cmp') | |||
| # stripping "with_origin" dict wrapper | |||
| if salt.utils.is_freebsd(): | |||
| if type(versions) is dict and 'version' in versions: | |||
There was a problem hiding this comment.
@terminalmage @rickyninja and I stared at this for a while to see if we should be doing something inside pkgng.py to disable with_origin but neither of us could determine if that was the right intent. This seems good to me in all cases but I'm interested if you have any feedback.
There was a problem hiding this comment.
Type checking should be using isinstance, since it more gracefully handles subclasses. Not saying we'll ever subclass dict here, but it's a good way of future-proofing against potential future breakage.
With regard to disabling with_origin, I'm not familiar with this option, I did not add support for it. It seems to default to False though, looking at the source code.
There was a problem hiding this comment.
ok, it's handled in any case with the type inspection.
There was a problem hiding this comment.
@amontalban ok with these changes?
There was a problem hiding this comment.
Like I said, we should not be using simple type matching here. Can we get this changed to:
if isinstance(versions, dict) and 'version' in versions:There was a problem hiding this comment.
I amended the commit to switch to isinstance last night. You can see in "Files changed" tab, but not here in discussion.
There was a problem hiding this comment.
@terminalmage this appears to be an artifact of the new GitHub reviews changes
There was a problem hiding this comment.
Yeah it does, I'm use to seeing a note that the comments are on an outdated diff when the PR is updated, and it doesn't appear to do that now.
| @@ -1241,10 +1245,14 @@ def installed( | |||
| failed_hold = None | |||
| if targets or to_reinstall: | |||
| reinstall = bool(to_reinstall) | |||
| force = False | |||
| if salt.utils.is_freebsd(): | |||
| force = True # Downgrades need to be forced. | |||
There was a problem hiding this comment.
This doesn't appear to be part of adding a version_cmp function. Is this related to another issue?
There was a problem hiding this comment.
It's necessary for version comparison to apply, pkg wont honor a downgrade without it.
There was a problem hiding this comment.
I was testing with pkg.installed. You can not install a package when the version is less than the currently installed version without doing a force.
There was a problem hiding this comment.
OK, this wasn't mentioned at all in the pull request message, so I wasn't sure. Thanks for the clarification.
There was a problem hiding this comment.
@rickyninja Thanks for the additional explanation. I did merge this, but it might need to be tweaked a little now that I think about it. The way we decide whether or not a downgrade needs to be done is typically handled within the execution module itself (see for instance yumpkg.py).
There was a problem hiding this comment.
@terminalmage You're welcome. This is my first time looking at the salt code, so I wasn't aware of the yumpkg downgrade stuff. If you need any more changes from me in a follow-up pull request please let me know.
There was a problem hiding this comment.
@rickyninja sure thing! It'll work as-is for now, but the code should be moved into pkgng.py probably. We try to keep platform-specific stuff out of the state module when possible, because it results in different behavior when executed via the state and via just calling pkg.install on the salt CLI.
What does this PR do?
Enable pkg compare_version for FreeBSD.
What issues does this PR fix or reference?
None I'm aware of.
Previous Behavior
Seems like it's falling back to a generic salt version_cmp, but doesn't work right for FreeBSD.
New Behavior
Version comparison works for FreeBSD packages.
Tests written?
No
Please review Salt's Contributing Guide for best practices.