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

Fix pkg.install when pkg already installed #48932

Merged
merged 8 commits into from Aug 9, 2018

Conversation

Projects
None yet
3 participants
@twangboy
Contributor

twangboy commented Aug 3, 2018

What does this PR do?

Fixes an issue with pkg.install on Windows where the current: <version> was being returned if you passed the same version that was installed. Correct behavior, when there are no changes, is to return an empty dictionary.

Adds tests to test valid output.

What issues does this PR fix or reference?

#48931

Tests written?

Yes

Commits signed with GPG?

Yes

@twangboy twangboy requested review from rallytime, damon-atkins and saltstack/team-windows Aug 3, 2018

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse Aug 3, 2018

@twangboy twangboy removed the request for review from saltstack/team-suse Aug 3, 2018

continue
# following can be version number or latest or Not Found
# Get the most recent version number available from winrepo.p

This comment has been minimized.

@damon-atkins

damon-atkins Aug 4, 2018

Member

Why is this change required it explains less about what is going on.

This comment has been minimized.

@twangboy

twangboy Aug 6, 2018

Contributor

I'll clarify this

'%s', version_num, pkg_name)
log.debug('"%s" version "%s" is already installed',
pkg_name, old[pkg_name][0])
ret[pkg_name] = {'current': old[pkg_name][0]}

This comment has been minimized.

@damon-atkins

damon-atkins Aug 4, 2018

Member

Not required. Also does not match the salt standard.

This comment has been minimized.

@twangboy

twangboy Aug 6, 2018

Contributor

This is required. It is the fix to the issue. If you don't pass a version number and the package is already installed it needs to return current: <version number>. Without this line it returns nothing.

This comment has been minimized.

@twangboy

twangboy Aug 6, 2018

Contributor

I don't know what you mean by the salt standard.

This comment has been minimized.

@damon-atkins

damon-atkins Aug 6, 2018

Member

Hi, It should return no results for no changes, as this is the standard across all the other platforms. Please look at how the other implementation work.

The following example shows the salt expected behaviour:

# salt-call -l info  pkg.version curl
[INFO    ] cmdmod._run Executing command [u'rpm', u'-qa', u'--queryformat', u'%{NAME}_|-%{EPOCH}_|-%{VERSION}_|-%{RELEASE}_|-%{ARCH}_|-(none)_|-%{INSTALLTIME}'] in directory '/root'
local:
    7.59.0-5.fc28

The above is the current version. So below will show no changes. i.e. no need output the current version.

# salt-call -l info  pkg.install curl
[INFO    ] cmdmod._run Executing command [u'rpm', u'-qa', u'--queryformat', u'%{NAME}_|-%{EPOCH}_|-%{VERSION}_|-%{RELEASE}_|-%{ARCH}_|-(none)_|-%{INSTALLTIME}'] in directory '/root'
[INFO    ] cmdmod._run Executing command [u'systemd-run', u'--scope', u'dnf', u'-y', u'--best', u'--allowerasing', u'install', u'curl'] in directory '/root'
[INFO    ] cmdmod._run Executing command [u'rpm', u'-qa', u'--queryformat', u'%{NAME}_|-%{EPOCH}_|-%{VERSION}_|-%{RELEASE}_|-%{ARCH}_|-(none)_|-%{INSTALLTIME}'] in directory '/root'
local:
    ----------

The following is a change example:

# salt-call -l info  pkg.version curl
[INFO    ] cmdmod._run Executing command [u'rpm', u'-qa', u'--queryformat', u'%{NAME}_|-%{EPOCH}_|-%{VERSION}_|-%{RELEASE}_|-%{ARCH}_|-(none)_|-%{INSTALLTIME}'] in directory '/root'
local:
    7.59.0-2.fc28

# salt-call -l info  pkg.install curl
[INFO    ] cmdmod._run Executing command [u'rpm', u'-qa', u'--queryformat', u'%{NAME}_|-%{EPOCH}_|-%{VERSION}_|-%{RELEASE}_|-%{ARCH}_|-(none)_|-%{INSTALLTIME}'] in directory '/root'
[INFO    ] cmdmod._run Executing command [u'systemd-run', u'--scope', u'dnf', u'-y', u'--best', u'--allowerasing', u'install', u'curl'] in directory '/root'
[INFO    ] cmdmod._run Executing command [u'rpm', u'-qa', u'--queryformat', u'%{NAME}_|-%{EPOCH}_|-%{VERSION}_|-%{RELEASE}_|-%{ARCH}_|-(none)_|-%{INSTALLTIME}'] in directory '/root'
local:
    ----------
    curl:
        ----------
        new:
            7.59.0-5.fc28
        old:
            7.59.0-2.fc28
    libcurl:
        ----------
        new:
            7.59.0-5.fc28
        old:
            7.59.0-2.fc28

The current version of win_pkg matches this behaviour (more work is needed still needed to improve matching the other platforms). The results then feed into changes under states/pkg.py

win_pkg needs to move closer to the behaviour of the other platforms. For example it needs to raise more CommandExecutionError where errors come out as comments under states/pkg.py

For example in states/pkg.py under def installed is:

        except CommandExecutionError as exc:
            ret = {'name': name, 'result': False}
            if exc.info:
                # Get information for state return from the exception.
                ret['changes'] = exc.info.get('changes', {})
                ret['comment'] = exc.strerror_without_changes
            else:
                ret['changes'] = {}
                ret['comment'] = ('An error was encountered while installing '
                                  'package(s): {0}'.format(exc))
            if warnings:
                ret.setdefault('warnings', []).extend(warnings)
            return ret

This comment has been minimized.

@damon-atkins

damon-atkins Aug 6, 2018

Member

Windows

# salt win10test state.high {'grepwin':{'pkg':['installed']}}
win10test:
    ----------
    pkg_|-grepwin_|-grepwin_|-installed:
        ----------
        __id__:
            grepwin
        __run_num__:
            0
        __sls__:
            None
        changes:
            ----------
            grepwin:
                ----------
                new:
                    1.6.682
                old:
        comment:
            The following packages were installed/updated: grepwin
        duration:
            28764.621
        name:
            grepwin
        result:
            True
        start_time:
            02:49:50.644832

# salt win10test state.high {'grepwin':{'pkg':['installed']}}
win10test:
    ----------
    pkg_|-grepwin_|-grepwin_|-installed:
        ----------
        __id__:
            grepwin
        __run_num__:
            0
        __sls__:
            None
        changes:
            ----------
        comment:
            All specified packages are already installed
        duration:
            28122.063
        name:
            grepwin
        result:
            True
        start_time:
            02:51:30.499313

Unix/Linux

# salt master state.high {'curl':{'pkg':['installed']}}
master:
    ----------
    pkg_|-curl_|-curl_|-installed:
        ----------
        __id__:
            curl
        __run_num__:
            0
        __sls__:
            None
        changes:
            ----------
        comment:
            All specified packages are already installed
        duration:
            5437.673
        name:
            curl
        result:
            True
        start_time:
            02:53:54.484794

If you place something in changes by returning data then comment will no longer say All specified packages are already installed

ret[pkg_name] = {'current': version_num}
continue
# If version number not installed, is the version available?
elif version_num != 'latest' and version_num not in pkginfo:
elif version_num.lower() != 'latest' and version_num not in pkginfo:

This comment has been minimized.

@damon-atkins

damon-atkins Aug 4, 2018

Member

Better to make sure latest is never upper case in winrepo. Otherwise this is a fix which is needed in multiple places on the off chance someone uses capitals. Have we documented capitals is ok somewhere. I would have thought all the doco is latest not Latest or LATEST Alternative is to just update the doco.

This comment has been minimized.

@twangboy

twangboy Aug 6, 2018

Contributor

version_num is populated with the contents of the version parameter passed either in a state file or on the CLI. Since it is user input, I added the .lower() just to make sure.
Later in the code, if version is not specified, version_num is then populated with the results of the _get_latest_pkg_version(pkginfo) command.
There aren't that many packages that use the latest option in the Software Definition file. But, it probably wouldn't hurt to run .lower() on all of them to account for inconsistencies.

This comment has been minimized.

@damon-atkins

damon-atkins Aug 6, 2018

Member

Note: states/pkg.py only supports latest There is no .lower() used in states/pkg.py.
Allowing it on the cli or in the sls package definitions might confusion the topic as people may try to use it with pkg.installed state. Hence I would suggest a reminder in the doco. And maybe an error in a package definition file reported by refresh_db. Educate the right way.

This comment has been minimized.

@twangboy

twangboy Aug 7, 2018

Contributor

So, you're suggesting to let the function fail if the user passes Latest instead of latest?

This comment has been minimized.

@twangboy

twangboy Aug 7, 2018

Contributor

If Latest is passed in the state system, wouldn't it be caught here?

@damon-atkins

Only place return results should be played with is a schedule job which is not waited for.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Aug 5, 2018

@twangboy Can we get a test written for this so it does not regress again?

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented Aug 6, 2018

Hi Guys this is not the correct fix. I have updated the issue #48931

@twangboy

This comment has been minimized.

Contributor

twangboy commented Aug 6, 2018

This is the fix to THIS issue

@twangboy

This comment has been minimized.

Contributor

twangboy commented Aug 6, 2018

@rallytime I'll write some tests

twangboy added some commits Aug 6, 2018

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented Aug 7, 2018

Hi @terminalmage

If pkg.install or pkg.remove end up not making any changes to the target system they return an empty dict indicating no changes. This is the salt standard for pkg.install and pkg.remove is to return differences in the packages and their version. Current release of 2016.11, 2017 and 2018 behaviour this way.

The PR will move win_pkg away from that standard, cause states/pkg.py to get data it would not normal expect. i.e. current: version number to show what version is currently installed.

This PR will change the output to

salt-call pkg.version
local:
    2.2.6
salt-call pkg.install vlc version=2.2.6
local:
    ----------
    vlc:
        ----------
        current:
            2.2.6

Instead of standard empty dict for no changes in software or version

salt-call pkg.install vlc version=2.2.6
local:
    ----------

Which will result in output like

win10test:
----------
          ID: vlc
    Function: pkg.installed
      Result: True
     Comment: Package vlc=2.2.6 was reinstalled.
     Started: 12:58:06.916154
    Duration: 19869.135 ms
     Changes:
              ----------
              vlc:
                  ----------
                  current:
                      2.2.6

vs standard

    ID: curl
    Function: pkg.installed
      Result: True
     Comment: Package curl=7.59.0-5.fc28 was reinstalled.
     Started: 13:06:09.163772
    Duration: 71738.376 ms
     Changes:
              ----------
              curl:
                  ----------
                  new:
                      7.59.0-5.fc28
                  old:
                      7.59.0-5.fc28

The return results from pkg.install and pkg.remove and other functions used by states/pkg.py need to match the other pkg modules. (ignoring package name differences between platforms)

@@ -1217,31 +1217,37 @@ def install(name=None, refresh=False, pkgs=None, **kwargs):
ret[pkg_name] = 'Unable to locate package {0}'.format(pkg_name)
continue
version_num = options.get('version', '')
version_num = options.get('version')

This comment has been minimized.

@damon-atkins

damon-atkins Aug 7, 2018

Member

Are you saying that version will always exist under options and hence there is no need for a default value to prevent an error.

This comment has been minimized.

@twangboy

twangboy Aug 7, 2018

Contributor

No, just using the default None

twangboy added some commits Aug 7, 2018

@twangboy

This comment has been minimized.

Contributor

twangboy commented Aug 7, 2018

@damon-atkins I have modified the PR to return an empty dict when there are no changes. I have also added tests to ensure correct returns in those scenarios. Thank you for educating me on proper salt behavior in regards to the pkg modules. Elsewhere in salt, execution module outputs are not standardized, by design apparently (I asked around). But in the case of the pkg system, there is some standardization. Thanks for keeping me straight.

@damon-atkins

Some more suggestions

continue
# following can be version number or latest or Not Found
# Get the most recent version number available from winrepo.p

This comment has been minimized.

@damon-atkins

damon-atkins Aug 8, 2018

Member

Pls leave the detail in about following can be version number or latest or Not Found
Most of the comments of have added over the years about making sure myself and others understand how the code works and its logic. It helps people new to the code, and reminds me when looking at the logic, and we result in less bugs being introduced.

version_num = _get_latest_pkg_version(pkginfo)
if version_num == 'latest' and 'latest' not in pkginfo:
if version_num.lower() == 'latest' and 'latest' not in pkginfo:

This comment has been minimized.

@damon-atkins

damon-atkins Aug 8, 2018

Member

Salt is like a language. Python does not support IF or iF it supports only if Unless you wish to scan the rest of the salt source code tree for latest and and support Latest or laTest etc. Users need to get some things right.

Has anyone complained about this?

continue
# If version number not installed, is the version available?
elif version_num != 'latest' and version_num not in pkginfo:
elif version_num.lower() != 'latest' and version_num not in pkginfo:

This comment has been minimized.

@damon-atkins
if not version_num:
if pkg_name in old:
log.debug('A version (%s) already installed for package '
'%s', version_num, pkg_name)
log.debug('"%s" version "%s" is already installed',

This comment has been minimized.

@damon-atkins

damon-atkins Aug 8, 2018

Member

I have had ErikJ turn my " into \' in the past. Given salts debug messages do not indicate the module running, it would be best to keep the word package.

version_num = _get_latest_pkg_version(pkginfo)
# Check if the version is already installed
if version_num in old.get(pkg_name, []):
# Desired version number already installed
ret[pkg_name] = {'current': version_num}
log.debug('"%s" version "%s" is already installed',

This comment has been minimized.

@damon-atkins
continue
# following can be version number or latest or Not Found
# Get the most recent version number available from winrepo.p
# May also return `latest` or an empty string

This comment has been minimized.

@damon-atkins

damon-atkins Aug 9, 2018

Member

or 'Not Found' a package definition can contain Not Found to match when the version string in the app can not be found.

@damon-atkins

Subject to comment update. Thanks

@rallytime rallytime merged commit 9b6a9ff into saltstack:2018.3 Aug 9, 2018

4 of 8 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details

@twangboy twangboy deleted the twangboy:fix_win_repo branch Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment