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

[2017.7] fixes to states/pkg.py #48426

Merged
merged 5 commits into from Jul 31, 2018

Conversation

Projects
None yet
4 participants
@garethgreenaway
Member

garethgreenaway commented Jul 3, 2018

What does this PR do?

Fixing a situation when a package is already installed via salt or manually and a state attempts to set that package to be held.

What issues does this PR fix or reference?

#46689

Previous Behavior

Previously the holding/unholding logic was only being run against packages that were being installed.

New Behavior

This change moves the holding logic outside and runs it against all desired packages. Adding a new test to test holding logic.

Tests written?

Yes

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

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

@garethgreenaway garethgreenaway requested review from terminalmage and removed request for saltstack/team-suse Jul 3, 2018

else:
hold_ret = __salt__['pkg.unhold'](
name=name, pkgs=desired, sources=sources
)

This comment has been minimized.

@isbm

isbm Jul 5, 2018

Contributor

Or shorter, so easier comprehend half-year later while debuggin and no same code repetitions:

hold_ret = __salt__['pkg.{}hold'.format(not kwargs['hold'] and 'un' or '')](
        name=name, pkgs=desired, sources=sources
)

This comment has been minimized.

@terminalmage

terminalmage Jul 5, 2018

Member

This doesn't look that much better. It's certainly a lot less readable. It would be more readable like this:

action = 'pkg.hold' if kwargs['hold'] else 'pkg.unhold'
hold_ret = __salt__[action](
    name=name, pkgs=desired, sources=sources
)

This comment has been minimized.

@isbm

isbm Jul 5, 2018

Contributor

That was exactly my first version, including the variable name... Probably I should use less compression in a future. 😏

except (CommandExecutionError, SaltInvocationError) as exc:
comment.append(str(exc))
if 'pkg.hold' in __salt__:
if 'hold' in kwargs:

This comment has been minimized.

@isbm

isbm Jul 5, 2018

Contributor

No need in nested ifs and unnecessary large indent because of it:

if 'pkg.hold' in __salt__ and 'hold' in kwargs:

This comment has been minimized.

@terminalmage

terminalmage Jul 5, 2018

Member

While @garethgreenaway didn't originally write this (he just dedented existing code), this would probably be a good idea, especially since there is no action taken when the hold argument is not passed.

else:
hold_ret = __salt__['pkg.unhold'](
name=name, pkgs=desired, sources=sources
)

This comment has been minimized.

@terminalmage

terminalmage Jul 5, 2018

Member

This doesn't look that much better. It's certainly a lot less readable. It would be more readable like this:

action = 'pkg.hold' if kwargs['hold'] else 'pkg.unhold'
hold_ret = __salt__[action](
    name=name, pkgs=desired, sources=sources
)
except (CommandExecutionError, SaltInvocationError) as exc:
comment.append(str(exc))
if 'pkg.hold' in __salt__:
if 'hold' in kwargs:

This comment has been minimized.

@terminalmage

terminalmage Jul 5, 2018

Member

While @garethgreenaway didn't originally write this (he just dedented existing code), this would probably be a good idea, especially since there is no action taken when the hold argument is not passed.

@garethgreenaway garethgreenaway force-pushed the garethgreenaway:46689_fixing_pkg_held_when_package_is_installed branch from 3c3c476 to 1208f59 Jul 11, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 31, 2018

@garethgreenaway garethgreenaway force-pushed the garethgreenaway:46689_fixing_pkg_held_when_package_is_installed branch from 6909037 to e521f6b Jul 31, 2018

garethgreenaway added some commits Jul 3, 2018

Fixing a situation when a package is already installed via salt or ma…
…nually and a state attempts to set that package to be held. Previously the holding/unholding logic was only being run against packages that were being installed. This change moves the holding logic outside and runs it against all desired packages. Adding a new test to test holding logic.
General cleanup in pkg state, reducing duplicate code. Fixing the req…
…uires_salt_modules decorator, sys.doc was returning too much information for the event to handle. This change specifically calls sys.doc with the module name.

@garethgreenaway garethgreenaway force-pushed the garethgreenaway:46689_fixing_pkg_held_when_package_is_installed branch from e521f6b to 9b0f5dd Jul 31, 2018

@terminalmage

LGTM

@rallytime rallytime merged commit 8a12852 into saltstack:2017.7 Jul 31, 2018

8 checks passed

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

garethgreenaway added a commit to garethgreenaway/salt that referenced this pull request Oct 31, 2018

pkg.installed currently fails when sources is used along with hold: T…
…rue. This was due to a change in saltstack#48426 that swapped out sending the pkgs variable for the desired variable instead.  This caused problems with pkg.hold because desired and sources are always populated, and pkg.hold can only include one or the other.  This change just includes desired in the call to pkg.hold since desired has the same value for sources.

garethgreenaway added a commit to garethgreenaway/salt that referenced this pull request Nov 20, 2018

pkg.installed currently fails when sources is used along with hold: T…
…rue. This was due to a change in saltstack#48426 that swapped out sending the pkgs variable for the desired variable instead.  This caused problems with pkg.hold because desired and sources are always populated, and pkg.hold can only include one or the other.  This change just includes desired in the call to pkg.hold since desired has the same value for sources.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment