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

FreeBSD pkg module enhancements #45217

Merged
merged 14 commits into from Jan 19, 2018
Merged

FreeBSD pkg module enhancements #45217

merged 14 commits into from Jan 19, 2018

Conversation

@amendlik
Copy link
Contributor

@amendlik amendlik commented Dec 29, 2017

What does this PR do?

Various enhancements for the FreeBSD pkgng execution module:

  1. Add functions to manage package locking
    • pkg.lock
    • pkg.unlock
    • pkg.locked
    • pkg.list_locked
  2. Add pkg.list_upgrades for compatibility with pkg.uptodate state
  3. Support pkgs keyword in pkg.upgrade for compatibility with pkg.uptodate state
  4. Add pkg.hold and pkg.unhold for compatibility with pkg.installed state

What issues does this PR fix or reference?

None

New Behavior

Better compatibility with pkg.uptodate and pkg.installed states

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.

@rallytime rallytime requested a review from terminalmage Jan 2, 2018
@rallytime
Copy link
Contributor

@rallytime rallytime commented Jan 2, 2018

amendlik added 10 commits Jan 2, 2018
This makes the pkgng execution module compatible with the
pkg.uptodate state.
The pkgs kwarg is passed by salt.states.pkg.uptodate. If it
is not recognized by the pkg module, all packages are upgraded.
These functions are called by salt.states.pkg.installed, if
present. They are implemented here for compatibility between
pkg.installed (state) and pkg.lock/unlock (module)
This change reimplements the pkgng.list_upgrades function
to use the `pkg upgrade --dry-run` command. An earlier version
of this function used `pkg version`, but it had several problems:

1) Long execution times when used with multiple repositories
   and the --no-repo-update argument.
2) It would show expected upgrades for a package when any repository
   had a newer version available. FreeBSD, by default, only
   applies upgrades from the repository the package was originally
   installed from.
3) It would show expected upgrades for locked packages.
@amendlik
Copy link
Contributor Author

@amendlik amendlik commented Jan 4, 2018

@rallytime Those lint errors are fixed now.

continue
locked_pkgs.append(pkgname)

log.debug("Locked packages: {0}".format(locked_pkgs))
Copy link
Collaborator

@terminalmage terminalmage Jan 4, 2018

Can you revise your logging calls, and remove str.format usage in favor of string replacement? Python logging already does string replacement using any positional args passed after the format string, so using str.format is redundant. Also, we have code in our custom logging handler which normalizes the format string and the args passed to it so that they are unicode, preventing log messages from triggering unicode encoding/decoding errors. This log message should be changed to:

    log.debug("Locked packages: %s", locked_pkgs)

I realize you have probably seen a lot of usage of str.format in logging, but it's something we're working to stamp out in the codebase to make logging work better with unicode.

Copy link
Collaborator

@terminalmage terminalmage Jan 4, 2018

Note, there are other logging calls where similar changes need to be made, this is just one example.

Copy link
Collaborator

@terminalmage terminalmage left a comment

One minor change left to make.

@@ -74,10 +75,10 @@ def __virtual__():
providers = {}
if 'providers' in __opts__:
providers = __opts__['providers']
log.debug('__opts__.providers: {0}'.format(providers))
log.debug('__opts__.providers: %s', str(providers))
Copy link
Collaborator

@terminalmage terminalmage Jan 8, 2018

There is no need to use str() here. The fact that you have used a %s placeholder in the log string means that Python's logging module will convert the dictionary to a string.

Copy link
Contributor Author

@amendlik amendlik Jan 9, 2018

Thank you for the code review, @terminalmage. The last item is fixed.

@amendlik
Copy link
Contributor Author

@amendlik amendlik commented Jan 15, 2018

@terminalmage Is there anything left to do on this change?

@rallytime rallytime requested a review from terminalmage Jan 16, 2018
@rallytime rallytime merged commit 428f247 into saltstack:develop Jan 19, 2018
4 of 10 checks passed
@amendlik amendlik deleted the pkgng-lock branch Mar 3, 2018
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

3 participants