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.

Sign up for GitHub

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

Jump to bottom

[master] Update pkg.group_installed state to support repo options #64349

Closed
wants to merge 3 commits into from
Closed

[master] Update pkg.group_installed state to support repo options #64349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a950d86
Update pkg.group_installed state to support repo options
terminalmage May 24, 2023
36b7dc3
Handle unsupported arguments gracefully if used in pkg.group_installe…
terminalmage May 25, 2023
14cbf33
Add changelog file
terminalmage May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/64348.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Update `pkg.group_installed` state to support repo options
69 changes: 64 additions & 5 deletions salt/modules/yumpkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ def _yum_pkginfo(output):
keys = itertools.cycle(("name", "version", "repoid"))
values = salt.utils.itertools.split(_strip_headers(output))
osarch = __grains__["osarch"]
for (key, value) in zip(keys, values):
for key, value in zip(keys, values):
if key == "name":
try:
cur["name"], cur["arch"] = value.rsplit(".", 1)
Expand Down Expand Up @@ -2570,7 +2570,7 @@ def group_list():
return ret


def group_info(name, expand=False, ignore_groups=None):
def group_info(name, expand=False, ignore_groups=None, **kwargs):
"""
.. versionadded:: 2014.1.0
.. versionchanged:: 3001,2016.3.0,2015.8.4,2015.5.10
Expand All @@ -2581,6 +2581,10 @@ def group_info(name, expand=False, ignore_groups=None):
to ``mandatory``, ``optional``, and ``default`` for accuracy, as
environment groups include other groups, and not packages. Finally,
this function now properly identifies conditional packages.
.. versionchanged:: 3006.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like its actually a feature, which resolves a bug so this would need to be 3007.0, same for the 3006.2 references below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for the pkgrepo.absent state says that you can pass anything you would pass through to the execution module, which is not true. IMO, that's a bug. Promised functionality doesn't exist. This modification is in service of fixing that bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, had my other PR in mind, I meant pkg.group_installed, not pkgrepo.absent.

Here's the relevant block of code: https://github.com/saltstack/salt/blob/c33b562/salt/states/pkg.py#L3402-L3406

Support for ``fromrepo``, ``enablerepo``, and ``disablerepo`` (as used
in :py:func:`pkg.install <salt.modules.yumpkg.install>`) has been
added.

Lists packages belonging to a certain group

Expand All @@ -2601,18 +2605,46 @@ def group_info(name, expand=False, ignore_groups=None):

.. versionadded:: 3001

fromrepo
Restrict ``yum groupinfo`` to the specified repo(s).
(e.g., ``yum --disablerepo='*' --enablerepo='somerepo'``)

.. versionadded:: 3006.2

enablerepo (ignored if ``fromrepo`` is specified)
Specify a disabled package repository (or repositories) to enable.
(e.g., ``yum --enablerepo='somerepo'``)

.. versionadded:: 3006.2

disablerepo (ignored if ``fromrepo`` is specified)
Specify an enabled package repository (or repositories) to disable.
(e.g., ``yum --disablerepo='somerepo'``)

.. versionadded:: 3006.2

CLI Example:

.. code-block:: bash

salt '*' pkg.group_info 'Perl Support'
salt '*' pkg.group_info 'Perl Support' fromrepo=base,updates
salt '*' pkg.group_info 'Perl Support' enablerepo=somerepo
"""
pkgtypes = ("mandatory", "optional", "default", "conditional")
ret = {}
for pkgtype in pkgtypes:
ret[pkgtype] = set()

cmd = [_yum(), "--quiet", "groupinfo", name]
options = _get_options(
**{
key: val
for key, val in kwargs.items()
if key in ("fromrepo", "enablerepo", "disablerepo")
}
)

cmd = [_yum(), "--quiet"] + options + ["groupinfo", name]
out = __salt__["cmd.run_stdout"](cmd, output_loglevel="trace", python_shell=False)

g_info = {}
Expand Down Expand Up @@ -2680,30 +2712,57 @@ def group_info(name, expand=False, ignore_groups=None):
return ret


def group_diff(name):
def group_diff(name, **kwargs):
"""
.. versionadded:: 2014.1.0
.. versionchanged:: 2016.3.0,2015.8.4,2015.5.10
Environment groups are now supported. The key names have been renamed,
similar to the changes made in :py:func:`pkg.group_info
<salt.modules.yumpkg.group_info>`.
.. versionchanged:: 3006.2
Support for ``fromrepo``, ``enablerepo``, and ``disablerepo`` (as used
in :py:func:`pkg.install <salt.modules.yumpkg.install>`) has been
added.

Lists which of a group's packages are installed and which are not
installed

name
The name of the group to check

fromrepo
Restrict ``yum groupinfo`` to the specified repo(s).
(e.g., ``yum --disablerepo='*' --enablerepo='somerepo'``)

.. versionadded:: 3006.2

enablerepo (ignored if ``fromrepo`` is specified)
Specify a disabled package repository (or repositories) to enable.
(e.g., ``yum --enablerepo='somerepo'``)

.. versionadded:: 3006.2

disablerepo (ignored if ``fromrepo`` is specified)
Specify an enabled package repository (or repositories) to disable.
(e.g., ``yum --disablerepo='somerepo'``)

.. versionadded:: 3006.2

CLI Example:

.. code-block:: bash

salt '*' pkg.group_diff 'Perl Support'
salt '*' pkg.group_diff 'Perl Support' fromrepo=base,updates
salt '*' pkg.group_diff 'Perl Support' enablerepo=somerepo
"""
pkgtypes = ("mandatory", "optional", "default", "conditional")
ret = {}
for pkgtype in pkgtypes:
ret[pkgtype] = {"installed": [], "not installed": []}

pkgs = list_pkgs()
group_pkgs = group_info(name, expand=True)
group_pkgs = group_info(name, expand=True, **kwargs)
for pkgtype in pkgtypes:
for member in group_pkgs.get(pkgtype, []):
if member in pkgs:
Expand Down
95 changes: 72 additions & 23 deletions salt/states/pkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
import os
import re

import salt.utils.args
import salt.utils.pkg
import salt.utils.platform
import salt.utils.versions
Expand Down Expand Up @@ -257,7 +258,7 @@ def _find_download_targets(
normalize=True,
skip_suggestions=False,
ignore_epoch=None,
**kwargs
**kwargs,
):
"""
Inspect the arguments to pkg.downloaded and discover what packages need to
Expand Down Expand Up @@ -512,7 +513,7 @@ def _find_install_targets(
ignore_epoch=None,
reinstall=False,
refresh=False,
**kwargs
**kwargs,
):
"""
Inspect the arguments to pkg.installed and discover what packages need to
Expand Down Expand Up @@ -799,7 +800,7 @@ def _find_install_targets(
package_name,
ignore_types=ignore_types,
verify_options=verify_options,
**kwargs
**kwargs,
)
except (CommandExecutionError, SaltInvocationError) as exc:
failed_verify = exc.strerror
Expand Down Expand Up @@ -833,7 +834,7 @@ def _find_install_targets(
package_name,
ignore_types=ignore_types,
verify_options=verify_options,
**kwargs
**kwargs,
)
except (CommandExecutionError, SaltInvocationError) as exc:
failed_verify = exc.strerror
Expand Down Expand Up @@ -1061,7 +1062,7 @@ def installed(
ignore_epoch=None,
reinstall=False,
update_holds=False,
**kwargs
**kwargs,
):
"""
.. versionchanged:: 3007.0
Expand Down Expand Up @@ -1769,7 +1770,7 @@ def installed(
ignore_epoch=ignore_epoch,
reinstall=reinstall,
refresh=refresh,
**kwargs
**kwargs,
)

try:
Expand Down Expand Up @@ -1938,7 +1939,7 @@ def installed(
update_holds=update_holds,
ignore_epoch=ignore_epoch,
split_arch=False,
**kwargs
**kwargs,
)
except CommandExecutionError as exc:
ret = {"name": name, "result": False}
Expand Down Expand Up @@ -2155,7 +2156,7 @@ def installed(
reinstall_pkg,
ignore_types=ignore_types,
verify_options=verify_options,
**kwargs
**kwargs,
)
if verify_result:
failed.append(reinstall_pkg)
Expand Down Expand Up @@ -2344,7 +2345,7 @@ def downloaded(
downloadonly=True,
fromrepo=fromrepo,
ignore_epoch=ignore_epoch,
**kwargs
**kwargs,
)
ret["result"] = True
ret["changes"].update(pkg_ret)
Expand Down Expand Up @@ -2518,7 +2519,7 @@ def latest(
skip_verify=False,
pkgs=None,
watch_flags=True,
**kwargs
**kwargs,
):
"""
.. versionchanged:: 3007.0
Expand Down Expand Up @@ -2829,7 +2830,7 @@ def latest(
fromrepo=fromrepo,
skip_verify=skip_verify,
pkgs=targeted_pkgs,
**kwargs
**kwargs,
)
except CommandExecutionError as exc:
return {
Expand Down Expand Up @@ -2937,7 +2938,7 @@ def _uninstall(
pkgs=None,
normalize=True,
ignore_epoch=None,
**kwargs
**kwargs,
):
"""
Common function for package removal
Expand Down Expand Up @@ -3001,7 +3002,6 @@ def _uninstall(
}

if __opts__["test"]:

_changes = {}
_changes.update({x: {"new": "{}d".format(action), "old": ""} for x in targets})

Expand Down Expand Up @@ -3144,7 +3144,7 @@ def removed(name, version=None, pkgs=None, normalize=True, ignore_epoch=None, **
pkgs=pkgs,
normalize=normalize,
ignore_epoch=ignore_epoch,
**kwargs
**kwargs,
)
except CommandExecutionError as exc:
ret = {"name": name, "result": False}
Expand Down Expand Up @@ -3236,7 +3236,7 @@ def purged(name, version=None, pkgs=None, normalize=True, ignore_epoch=None, **k
pkgs=pkgs,
normalize=normalize,
ignore_epoch=ignore_epoch,
**kwargs
**kwargs,
)
except CommandExecutionError as exc:
ret = {"name": name, "result": False}
Expand Down Expand Up @@ -3367,9 +3367,16 @@ def group_installed(name, skip=None, include=None, **kwargs):
.. versionchanged:: 2016.11.0
Added support in :mod:`pacman <salt.modules.pacman>`

.. versionchanged:: 3006.2
For RPM-based systems, support for ``fromrepo``, ``enablerepo``, and
``disablerepo`` (as used in :py:func:`pkg.install
<salt.modules.yumpkg.install>`) has been added. This allows one to, for
example, use ``enablerepo`` to perform a group install from a repo that
is otherwise disabled.

Ensure that an entire package group is installed. This state is currently
only supported for the :mod:`yum <salt.modules.yumpkg>` and :mod:`pacman <salt.modules.pacman>`
package managers.
only supported for the :mod:`yum <salt.modules.yumpkg>` and :mod:`pacman
<salt.modules.pacman>` package managers.

skip
Packages that would normally be installed by the package group
Expand Down Expand Up @@ -3399,6 +3406,45 @@ def group_installed(name, skip=None, include=None, **kwargs):
This option can no longer be passed as a comma-separated list, it
must now be passed as a list (as shown in the above example).

.. note::
The below options are only supported on RPM-based systems

fromrepo
Restrict ``yum groupinfo`` to the specified repo(s).
(e.g., ``yum --disablerepo='*' --enablerepo='somerepo'``)

.. code-block:: yaml

MyGroup:
pkg.group_installed:
- fromrepo: base,updates

.. versionadded:: 3006.2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, if on https://github.com/saltstack/salt/pull/64349/files#r1210742029 we can reasonable say it's a bug, because of your argument, why is it versionadded here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because support for these arguments needed to be added to make the code agree with the documentation: https://github.com/saltstack/salt/blob/c33b562/salt/states/pkg.py#L3402-L3406

Is it no longer considered a bug when the code doesn't do what the documentation says? I'm confused.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it no longer considered a bug when the code doesn't do what the documentation says? I'm confused.

It is, of course.

But can I argue that the function changed to accommodate the missing(and promised support) for the extra arguments?

Copy link
Contributor Author

@terminalmage terminalmage May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But can I argue that the function changed to accommodate the missing(and promised support) for the extra arguments?

I don't think that has ever been in dispute.

What seems to be in dispute though (unless I misunderstood your question), is whether adding arguments to a function to fix a bug makes the PR no longer a bugfix.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was suggesting from versionadded to versionchanged not that this is not code added to fix a bug.

Ignore my rant, I agree with the versionadded usage here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, yes, I definitely misunderstood your earlier comment (sorry about that).

My thinking behind using versionadded is that prior releases did not support those arguments. I can see how the documentation could be confusing though. At the top of the docstring there is a versionchanged which describes the repo options, but the options themselves have versionadded markers.

I'm happy with whichever solution makes the docs make the most sense.


enablerepo (ignored if ``fromrepo`` is specified)
Specify a disabled package repository (or repositories) to enable.
(e.g., ``yum --enablerepo='somerepo'``)

.. code-block:: yaml

MyGroup:
pkg.group_installed:
- enablerepo: myrepo

.. versionadded:: 3006.2

disablerepo (ignored if ``fromrepo`` is specified)
Specify an enabled package repository (or repositories) to disable.
(e.g., ``yum --disablerepo='somerepo'``)

.. code-block:: yaml

MyGroup:
pkg.group_installed:
- disablerepo: epel

.. versionadded:: 3006.2

.. note::
Because this is essentially a wrapper around :py:func:`pkg.install
<salt.modules.yumpkg.install>`, any argument which can be passed to
Expand Down Expand Up @@ -3432,13 +3478,16 @@ def group_installed(name, skip=None, include=None, **kwargs):
include[idx] = str(item)

try:
diff = __salt__["pkg.group_diff"](name)
except CommandExecutionError as err:
ret[
"comment"
] = "An error was encountered while installing/updating group '{}': {}.".format(
name, err
diff = __salt__["pkg.group_diff"](
name, **salt.utils.args.clean_kwargs(**kwargs)
)
except (CommandExecutionError, TypeError) as err:
if "unexpected keyword argument" in str(err):
ret["comment"] = "Repo options are not supported on this platform"
else:
ret[
"comment"
] = f"An error was encountered while installing/updating group '{name}': {err}."
return ret

mandatory = diff["mandatory"]["installed"] + diff["mandatory"]["not installed"]
Expand Down
Loading
Loading