Skip to content
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

pkg.installed version parameter support for multiple conditions #48511

Merged
merged 14 commits into from Jul 16, 2018

Conversation

@0w3w
Copy link
Contributor

@0w3w 0w3w commented Jul 10, 2018

What does this PR do?

salt.states.pkg.installed supporting multiple version conditions.

Previous Behavior

Support for specifying a package condition in the version was recently added, this to enable more control over what versions will be installed.
E.G.

mypkgs:
  pkg.installed:
    - pkgs:
      - foo
      - bar: '>=1.2.3-4'

see https://docs.saltstack.com/en/latest/ref/states/all/salt.states.pkg.html#salt.states.pkg.installed

New Behavior

This feature enables support for multiple conditions, this to allow version ranges and exclusions, which will enable more complex scenarios.
E.G.

mypkgs:
  pkg.installed:
    - pkgs:
      - foo
      - bar: ' > 12.0.0, < 15.0.0, != 14.0.1'

Tests written?

Yes

…s to allow version ranges and exclusions, for more control over what versions will be installed.
@ghost ghost self-requested a review Jul 10, 2018
meaksh
meaksh approved these changes Jul 11, 2018
Copy link
Member

@meaksh meaksh left a comment

Looks good to me 👍. Thanks @0w3w for contributing!

@rallytime rallytime requested a review from terminalmage Jul 11, 2018
@rallytime
Copy link
Contributor

@rallytime rallytime commented Jul 11, 2018

@0w3w
Copy link
Contributor Author

@0w3w 0w3w commented Jul 12, 2018

@rallytime pylint errors fixed!


def test_split_comparison(self):
'''
Test passing a jid on the command line
Copy link
Collaborator

@terminalmage terminalmage Jul 13, 2018

I don't think that this test has anything to do with JIDs. Bad copy-and-paste?

'''
Test passing a jid on the command line
'''
for test_parameter in PkgUtilsTestCase.test_parameters:
Copy link
Collaborator

@terminalmage terminalmage Jul 13, 2018

There is no need to refer to class attributes here are they are inherited by the instance. You can use self.test_parameters here.

@0w3w
Copy link
Contributor Author

@0w3w 0w3w commented Jul 13, 2018

@terminalmage Comments addressed, good catch, thanks for the review!

Copy link
Collaborator

@terminalmage terminalmage left a comment

blind-review

@damon-atkins
Copy link
Collaborator

@damon-atkins damon-atkins commented Jul 16, 2018

Hi 0w3w
Just a few questions.

This change assumes comma and spaces are not used in a version string. Before it assumed prefix of a version string would not be >= etc. What impact will this have? I suspect none. However might be worth a note in the doco. Have you added the doco to pkg.installed() for this change?

What happens if 14.0.1 or 17.0 is already installed, is it removed or upgraded or down-graded?
bar: ' > 12.0.0, < 15.0.0, != 14.0.1'
If down or up graded which version does it move too.

I am assume that compare version() is coming from the execution module still.

allow_updates should be removed. i.e. time to add a warning the setting is going.

The following seems to be in only one place.

            if allow_updates and len(version_conditions) == 1 and operator == '==':
                operator = '>='

I would have thought that allow_updates bool check could be removed from other parts of the code if version string is prefixed with >= when allow_updates is true at the start of the functions.

@0w3w
Copy link
Contributor Author

@0w3w 0w3w commented Jul 16, 2018

Hey @damon-atkins

You are right, the change assumes version numbers following the debian standard (https://www.debian.org/doc/debian-policy/#version) as well as the classic standard version format (major.minor.update.build), which covers, I would say, all of the versioning schemas. The REGEX part to detect the version string was not change though, so whatever version string was previously supported, will still be supported. The list of operators was expanded to support libsolv-based package managers and debian operators.
I don't expect any impact on this change, as it is, in essence, backwards compatible.

I have not updated the DOCO as I expect the first pkg provider to claim support for this, to update it. (opkg can be a great provider for this, I'll talk to Alejandro del Castillo, the maintainer of opkg, to see if he can implement this)

"What happens if 14.0.1 or 17.0 is already installed, is it removed or upgraded or down-graded? bar: ' > 12.0.0, < 15.0.0, != 14.0.1'"
It will be downgraded to the highest available version that meets the condition.

Didn't make changes to compare_version, should come from the same module.

Regarding the allow_updates behavior, I purposefully maintained the existing logic to warranty backwards compatibility and didn't change existing behavior.

Thanks for the review!

@rallytime
Copy link
Contributor

@rallytime rallytime commented Jul 16, 2018

Thanks for this addition @0w3w and congrats on your first salt PR! :)

@rallytime rallytime merged commit 943e21f into saltstack:develop Jul 16, 2018
8 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants