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

Bugfixes and unit tests for pkgin module #47814

Merged
merged 3 commits into from May 25, 2018

Conversation

Projects
None yet
6 participants
@travispaul
Contributor

travispaul commented May 24, 2018

Corrects pkg.latest_version and pkg.file_dict behavior

What does this PR do?

Fixes 2 bugs in the pkgin module:

  • pkg.latest_version doesn't return a version for an uninstalled package.
  • pkg.file_dict crashes.

What issues does this PR fix or reference?

None that I have found.

Previous Behavior

pkg.latest_version

pkgin reports that nginx version 1.14.0 is available but not yet installed:

salt# pkgin se nginx
nginx-1.14.0         Lightweight HTTP server and mail proxy server

=: package is installed and up-to-date
<: package is installed but newer version is available
: installed package has a greater version than available package

However, pkg.latest_version returns nothing:

salt# salt \* pkg.latest_version nginx
salt.n7test.local:

pkg.file_dict

After installing a package, pkg.file_dict throws an exception:

# salt \* pkg.install nginx
salt.n7-test.local:
   ----------
   nginx:
       ----------
       new:
           1.14.0
       old:
   pcre:
       ----------
       new:
           8.42
       old:

salt# pkg_info -qL nginx
/opt/pkg/current/man/man8/nginx.8
/opt/pkg/current/sbin/nginx
/opt/pkg/current/share/examples/nginx/conf/fastcgi.conf
/opt/pkg/current/share/examples/nginx/conf/fastcgi_params
/opt/pkg/current/share/examples/nginx/conf/koi-utf
/opt/pkg/current/share/examples/nginx/conf/koi-win
/opt/pkg/current/share/examples/nginx/conf/mime.types
/opt/pkg/current/share/examples/nginx/conf/nginx.conf
/opt/pkg/current/share/examples/nginx/conf/win-utf
/opt/pkg/current/share/examples/nginx/html/50x.html
/opt/pkg/current/share/examples/nginx/html/inde

salt# salt \* pkg.file_dict nginx

salt.n7-test.local:
   The minion function caused an exception: Traceback (most recent call last):
     File "/opt/pkg/current/lib/python2.7/site-packages/salt/minion.py", line 1600, in _thread_return
       return_data = minion_instance.executors[fname](opts, data, func, args, kwargs)
     File "/opt/pkg/current/lib/python2.7/site-packages/salt/executors/direct_call.py", line 12, in execute
       return func(*args, **kwargs)
     File "/opt/pkg/current/lib/python2.7/site-packages/salt/modules/pkgin.py", line 684, in file_dict
       for field in ret:
   RuntimeError: dictionary changed size during iteration

New Behavior

pkg.latest_version

salt# salt \* pkg.latest_version nginx
salt.n7-test.local:
   1.14.0

pkg.file_dict

salt# salt \* pkg.file_dict nginx
salt.n7-test.local:
   ----------
   files:
       ----------
       nginx:
           - /opt/pkg/current/man/man8/nginx.8
           - /opt/pkg/current/sbin/nginx
           - /opt/pkg/current/share/examples/nginx/conf/fastcgi.conf
           - /opt/pkg/current/share/examples/nginx/conf/fastcgi_params
           - /opt/pkg/current/share/examples/nginx/conf/koi-utf
           - /opt/pkg/current/share/examples/nginx/conf/koi-win
           - /opt/pkg/current/share/examples/nginx/conf/mime.types
           - /opt/pkg/current/share/examples/nginx/conf/nginx.conf
           - /opt/pkg/current/share/examples/nginx/conf/win-utf
           - /opt/pkg/current/share/examples/nginx/html/50x.html
           - /opt/pkg/current/share/examples/nginx/html/index.html
           - /opt/pkg/current/share/examples/rc.d/nginx

Tests written?

Yes

Commits signed with GPG?

Yes


I'd just like to say that Salt has the best documentation I've ever seen for writing tests. As someone who has not written much Python, I especially found the page on using iPython invaluable.
I ran my unit test on a machine with pkgin installed then removed pkgin and the pkg_* tools and the tests still ran as expected but I may have missed something as writing Python unit tests is very new to me. Please let me know if they can be approved.

Bugfixes and unit tests for pkgin module
Corrects pkg.lastest_version and pkg.file_dict behavior

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse May 24, 2018

@travispaul

Added comments describing changes.

p = line.split(',' if _supports_parsing() else None)
if line.startswith('No results found for'):
return pkglist
p = line.split(';' if _supports_parsing() else None)

This comment has been minimized.

@travispaul

travispaul May 24, 2018

Contributor

This appears to have been inadvertently changed from ';' to ',' in 9df3dab

@@ -190,7 +192,7 @@ def latest_version(*names, **kwargs):
s = _splitpkg(p[0])
if s:
if not s[0] in pkglist:
if len(p) > 1 and p[1] == '<':
if len(p) > 1 and p[1] == '<' or p[1] == '':

This comment has been minimized.

@travispaul

travispaul May 24, 2018

Contributor

p[1] will be an empty string here if the package has not yet been installed.

@@ -681,7 +682,7 @@ def file_dict(*packages):
continue # unexpected string
ret = {'errors': errors, 'files': files}
for field in ret:
for field in list(ret):

This comment has been minimized.

@travispaul

travispaul May 24, 2018

Contributor

Using list() here resolves the RuntimeError: dictionary changed size during iteration exception.

@@ -181,7 +181,9 @@ def latest_version(*names, **kwargs):
out = __salt__['cmd.run'](cmd, output_loglevel='trace')
for line in out.splitlines():
p = line.split(',' if _supports_parsing() else None)
if line.startswith('No results found for'):

This comment has been minimized.

@travispaul

travispaul May 24, 2018

Contributor

Handles the case where the package wasn't found.

pkgin latest_version bugfix
Handle case where the currently installed package is the latest
@@ -192,7 +192,7 @@ def latest_version(*names, **kwargs):
s = _splitpkg(p[0])
if s:
if not s[0] in pkglist:
if len(p) > 1 and p[1] == '<' or p[1] == '':
if len(p) > 1 and p[1] in ('<', '', '='):

This comment has been minimized.

@travispaul

travispaul May 24, 2018

Contributor

p[1] is an empty string if the package is not installed and '=' if it is already installed and the latest version.

@isbm

isbm approved these changes May 24, 2018

@sjorge

This comment has been minimized.

Contributor

sjorge commented May 24, 2018

LGTM, will test this on SmartOS when I get home in about 1 hours.

@sjorge

sjorge approved these changes May 24, 2018

LGTM, works fine on SmartOS too.

[cronos :: sjorge][/salt/states/.extmods/_modules][master ✔]
[!]$ salt ares pkg.latest_version salt
ares:
    2018.3.0
[cronos :: sjorge][/salt/states/.extmods/_modules][master ✔]
[.]$ salt ares pkg.file_dict salt
ares:
    ----------
    files:
        ----------
        salt:
            - /opt/local/bin/salt
            - /opt/local/bin/salt-api
            - /opt/local/bin/salt-call
            - /opt/local/bin/salt-cloud
            - ...
self.assertEqual(pkgin.latest_version('somepkg'), '1.0')
'''
Test getting the latest version of an ininstalled package

This comment has been minimized.

@cachedout

cachedout May 24, 2018

Contributor

installed or uninstalled? ;)

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 24, 2018

1 similar comment
@rallytime

This comment has been minimized.

Contributor

rallytime commented May 24, 2018

@travispaul

This comment has been minimized.

Contributor

travispaul commented May 24, 2018

Thanks for the feedback. Hopefully I got those lint issues fixed. pylint wasn't as forgiving as jenkins and had some additional output (I did not run it until now).

@travispaul

This comment has been minimized.

Contributor

travispaul commented May 25, 2018

Looks like those lint issues are now fixed: https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/22156/
Please let me know if any other changes are needed. Thanks.

@rallytime rallytime merged commit 13cd450 into saltstack:develop May 25, 2018

4 of 10 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5248 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23182 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10219 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19304 — ABORTED
Details
codeclimate 1 issue to fix
Details
default Build finished.
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25438 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17526 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22156 — SUCCESS
Details

rallytime added a commit that referenced this pull request May 29, 2018

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