Make Salt to return an error on "pkg" modules and states when targeting duplicated package names

[meaksh]

What does this PR do?

This PR makes Salt to raise a CommandExecutionError when executing pkg modules or states and you are targeting several packages via pkgs argument but containing duplicated packages names in this list.

Currently, the logic around pkg_resource and repack_pkgs override possible duplicates, which leads into unexpected and inconsistent situations. This PR is not changing the logic to allow duplicated but instead making Salt to raise an error to avoid getting inconsistent results.

Previous Behavior

Given I have the following duplicated packages installed on my instance:

[root@e4d3d9edd1e5 /]# rpm -qa | grep kernel-devel
kernel-devel-4.18.0-348.20.1.el8_5.x86_64
kernel-devel-4.18.0-348.23.1.el8_5.x86_64
kernel-devel-4.18.0-348.12.1.el8_5.x86_64

[root@e4d3d9edd1e5 /]# salt-call --version
salt-call 3004.1

And given I have also this state SLS:

[root@e4d3d9edd1e5 /]# cat /srv/salt/mytest.sls
my_test_state:
  pkg.removed:
    - pkgs:
       - kernel-devel: 4.18.0-348.23.1.el8_5
       - kernel-devel: 4.18.0-348.20.1.el8_5
       - kernel-devel: 4.18.0-348.12.2.el8_5

When I execute my test state:

[root@e4d3d9edd1e5 /]# salt-call --local state.apply mytest
local:
----------
          ID: my_test_state
    Function: pkg.removed
      Result: True
     Comment: All targeted packages were removed.
     Started: 11:23:57.451158
    Duration: 3402.302 ms
     Changes:   
              ----------
              kernel-devel:
                  ----------
                  new:
                      4.18.0-348.20.1.el8_5,4.18.0-348.23.1.el8_5
                  old:
                      4.18.0-348.20.1.el8_5,4.18.0-348.23.1.el8_5,4.18.0-348.12.2.el8_5

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time:   3.402 s

As you can see, this result is a bit unexpected, as the state execution succeeded but it didn't really removed all the targeted packages (as it claims), but only the latest one specified in the pkgs list.

Now if I retrigger my state execution (given the last specified package in the list was already removed by previous run), then I got a completely wrong and inconsistent result:

[root@e4d3d9edd1e5 /]# salt-call --local state.apply mytest
local:
----------
          ID: my_test_state
    Function: pkg.removed
      Result: True
     Comment: All specified packages (matching specified versions) are already absent
     Started: 11:29:34.364189
    Duration: 442.461 ms
     Changes:   

Summary for local
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time: 442.461 ms

And this result is of course, not correct:

[root@e4d3d9edd1e5 /]# rpm -qa | grep kernel-devel
kernel-devel-4.18.0-348.20.1.el8_5.x86_64
kernel-devel-4.18.0-348.23.1.el8_5.x86_64

New Behavior

With the new behavior implemented in this PR, we are making Salt to raise an error when the user is passing duplicated names in the pkgs list, to make it clear this cannot be processed and the user needs to manage each duplicated package individually:

[root@e4d3d9edd1e5 /]# salt-call --local state.apply mytest
local:
----------
          ID: my_test_state
    Function: pkg.removed
      Result: False
     Comment: An error was encountered while removing package(s): You are passing a list of packages that contains duplicated packages names: [OrderedDict([('kernel-devel', '4.18.0-348.23.1.el8_5')]), OrderedDict([('kernel-devel', '4.18.0-348.20.1.el8_5')]), OrderedDict([('kernel-devel', '4.18.0-348.12.2.el8_5')])]. This cannot be processed. In case you are targeting different versions for the same package, please target them individually
     Started: 11:38:49.756961
    Duration: 3.987 ms
     Changes:   

Summary for local
------------
Succeeded: 0
Failed:    1
------------
Total states run:     1
Total run time:   3.987 ms

Adding support to pkg_resources and pkgs modules to allow targeting duplicated package names is a different story which is not the purpose of this PR.

What do you think about this? Thanks!

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes

[github-actions]

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)

Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- duration: 1.27s
- exit code: 1
/home/runner/.cache/pre-commit/repogpwp3115/py_env-python3/lib/python3.9/site-packages/_distutils_hack/init.py:30: UserWarning: Setuptools is replacing distutils.

warnings.warn("Setuptools is replacing distutils.")

The function 'format_pkg_list' on 'salt/modules/pkg_resource.py' does not have a 'CLI Example:' in it's docstring

The function 'format_version' on 'salt/modules/pkg_resource.py' does not have a 'CLI Example:' in it's docstring

Found 2 errors

---

Thanks again!

[twangboy]

This needs to be rebased and conflicts resolved

[meaksh]

@twangboy thanks for the review! this should be now ready as I fixed your suggestion 👍

[Ch3LL]

The bug seems to be that we should fix the ability to use different versions with the same package name. Is there a reason for not taking this approach?

[Ch3LL]

bump @meaksh ^

[meaksh]

Hey @Ch3LL, thanks for reviewing here and sorry for the delay!

I see no particular reason against fixing the ability properly on pkg_resources and pkgs modules, but for now, this PR was simply preventing the current inconsistency to happen until this feature is fixed.

As I mentioned on the description, this PR is not changing the logic to allow duplicated but instead is making Salt to raise an error to avoid getting inconsistent results. Adding support to pkg_resources and pkgs modules to allow targeting duplicated package names is a different story which is not the purpose of this PR.

Hth!