From 44ee04bd1f7d683fce246c16e752ace04d244b4c Mon Sep 17 00:00:00 2001
From: Rick Elrod <rick@elrod.me>
Date: Thu, 7 Jan 2021 11:32:06 -0600
Subject: [PATCH] [dnf] Make "remove" filtering closer to dnf CLI (#73033)

Change:
- Internally, use dnf.subject.Subject#get_best_query for state: absent
- Add a bunch of tests for removing packages, given a bunch of different
  pkg specs (nv, nvr, nvra, wildcard, etc.)

Test Plan:
- New tests
- Local experiments with DNF API via PDB.

Tickets:
- Fixes #72809

Signed-off-by: Rick Elrod <rick@elrod.me>
---
 changelogs/fragments/72809-dnf-remove-NV.yml  |  2 +
 lib/ansible/modules/dnf.py                    | 20 ++------
 test/integration/targets/dnf/tasks/dnf.yml    | 50 +++++++++++++++++++
 .../targets/dnf/tasks/test_sos_removal.yml    | 19 +++++++
 4 files changed, 76 insertions(+), 15 deletions(-)
 create mode 100644 changelogs/fragments/72809-dnf-remove-NV.yml
 create mode 100644 test/integration/targets/dnf/tasks/test_sos_removal.yml

diff --git a/changelogs/fragments/72809-dnf-remove-NV.yml b/changelogs/fragments/72809-dnf-remove-NV.yml
new file mode 100644
index 00000000000..2df87042dac
--- /dev/null
+++ b/changelogs/fragments/72809-dnf-remove-NV.yml
@@ -0,0 +1,2 @@
+minor_changes:
+  - "dnf - When ``state: absent``, package names are now matched similarly to how the ``dnf`` CLI matches them (https://github.com/ansible/ansible/issues/72809)."
diff --git a/lib/ansible/modules/dnf.py b/lib/ansible/modules/dnf.py
index 38f80094811..7550b3371fb 100644
--- a/lib/ansible/modules/dnf.py
+++ b/lib/ansible/modules/dnf.py
@@ -1155,21 +1155,11 @@ class DnfModule(YumDnf):
                                 response['results'].append(handled_remove_error)
                         continue
 
-                    installed_pkg = list(map(str, installed.filter(name=pkg_spec).run()))
-                    if installed_pkg:
-                        candidate_pkg = self._packagename_dict(installed_pkg[0])
-                        installed_pkg = installed.filter(name=candidate_pkg['name']).run()
-                    else:
-                        candidate_pkg = self._packagename_dict(pkg_spec)
-                        installed_pkg = installed.filter(nevra=pkg_spec).run()
-                    if installed_pkg:
-                        installed_pkg = installed_pkg[0]
-                        evr_cmp = self._compare_evr(
-                            installed_pkg.epoch, installed_pkg.version, installed_pkg.release,
-                            candidate_pkg['epoch'], candidate_pkg['version'], candidate_pkg['release'],
-                        )
-                        if evr_cmp == 0:
-                            self.base.remove(pkg_spec)
+                    installed_pkg = dnf.subject.Subject(pkg_spec).get_best_query(
+                        sack=self.base.sack).installed().run()
+
+                    for pkg in installed_pkg:
+                        self.base.remove(str(pkg))
 
                 # Like the dnf CLI we want to allow recursive removal of dependent
                 # packages
diff --git a/test/integration/targets/dnf/tasks/dnf.yml b/test/integration/targets/dnf/tasks/dnf.yml
index bf0f96e673e..1734d1e46da 100644
--- a/test/integration/targets/dnf/tasks/dnf.yml
+++ b/test/integration/targets/dnf/tasks/dnf.yml
@@ -772,3 +772,53 @@
     that:
       - wildcard_absent is successful
       - wildcard_absent is not changed
+
+- name: Test removing with various package specs
+  block:
+    - name: Ensure sos is installed
+      dnf:
+        name: sos
+        state: present
+
+    - name: Determine version of sos
+      command: rpm -q --queryformat=%{version} sos
+      register: sos_version_command
+
+    - name: Determine release of sos
+      command: rpm -q --queryformat=%{release} sos
+      register: sos_release_command
+
+    - name: Determine arch of sos
+      command: rpm -q --queryformat=%{arch} sos
+      register: sos_arch_command
+
+    - set_fact:
+        sos_version: "{{ sos_version_command.stdout | trim }}"
+        sos_release: "{{ sos_release_command.stdout | trim }}"
+        sos_arch: "{{ sos_arch_command.stdout | trim }}"
+
+    # We are just trying to remove the package by specifying its spec in a
+    # bunch of different ways. Each "item" here is a test (a name passed, to make
+    # sure it matches, with how we call Hawkey in the dnf module).
+    - include_tasks: test_sos_removal.yml
+      with_items:
+        - sos
+        - sos-{{ sos_version }}
+        - sos-{{ sos_version }}-{{ sos_release }}
+        - sos-{{ sos_version }}-{{ sos_release }}.{{ sos_arch }}
+        - sos-*-{{ sos_release }}
+        - sos-{{ sos_version[0] }}*
+        - sos-{{ sos_version[0] }}*-{{ sos_release }}
+        - sos-{{ sos_version[0] }}*{{ sos_arch }}
+
+    - name: Ensure deleting a non-existing package fails correctly
+      dnf:
+        name: a-non-existent-package
+        state: absent
+      ignore_errors: true
+      register: nonexisting
+
+    - assert:
+        that:
+          - nonexisting is success
+          - nonexisting.msg == 'Nothing to do'
diff --git a/test/integration/targets/dnf/tasks/test_sos_removal.yml b/test/integration/targets/dnf/tasks/test_sos_removal.yml
new file mode 100644
index 00000000000..40ceb62bf4d
--- /dev/null
+++ b/test/integration/targets/dnf/tasks/test_sos_removal.yml
@@ -0,0 +1,19 @@
+# These are safe to just check in check_mode, because in the module, the
+# logic to match packages will happen anyway. check_mode will just prevent
+# the transaction from actually running once the matches are found.
+- name: Remove {{ item }}
+  dnf:
+    name: "{{ item }}"
+    state: absent
+  check_mode: true
+  register: sos_rm
+
+- debug:
+    var: sos_rm
+
+- assert:
+    that:
+      - sos_rm is successful
+      - sos_rm is changed
+      - "'Removed: sos-{{ sos_version }}-{{ sos_release }}' in sos_rm.results[0]"
+      - sos_rm.results|length == 1