From 1e3b927a73a33951ae116d49aa727a77a7fb00ea Mon Sep 17 00:00:00 2001
From: Adam Miller <admiller@redhat.com>
Date: Tue, 28 Aug 2018 16:44:46 -0500
Subject: [PATCH] Improve error condition handling for dnf module (#44770)

- Fix comma separated list handling for package names
- Fix error message for unavailable/unknown package install attempt
- Fix pkg install result output generation

Signed-off-by: Adam Miller <admiller@redhat.com>
---
 lib/ansible/module_utils/yumdnf.py         | 30 +++++++++++++++++-----
 lib/ansible/modules/packaging/os/dnf.py    | 25 +++++++++++++++---
 test/integration/targets/dnf/tasks/dnf.yml |  2 +-
 3 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/lib/ansible/module_utils/yumdnf.py b/lib/ansible/module_utils/yumdnf.py
index 0d53914462..aa3e0138a6 100644
--- a/lib/ansible/module_utils/yumdnf.py
+++ b/lib/ansible/module_utils/yumdnf.py
@@ -87,12 +87,30 @@ class YumDnf(with_metaclass(ABCMeta, object)):
 
         # It's possible someone passed a comma separated string since it used
         # to be a string type, so we should handle that
-        if self.enablerepo and len(self.enablerepo) == 1 and ',' in self.enablerepo:
-            self.enablerepo = self.module.params['enablerepo'].split(',')
-        if self.disablerepo and len(self.disablerepo) == 1 and ',' in self.disablerepo:
-            self.disablerepo = self.module.params['disablerepo'].split(',')
-        if self.exclude and len(self.exclude) == 1 and ',' in self.exclude:
-            self.exclude = self.module.params['exclude'].split(',')
+        self.names = self.listify_comma_sep_strings_in_list(self.names)
+        self.disablerepo = self.listify_comma_sep_strings_in_list(self.disablerepo)
+        self.enablerepo = self.listify_comma_sep_strings_in_list(self.enablerepo)
+        self.exclude = self.listify_comma_sep_strings_in_list(self.exclude)
+
+    def listify_comma_sep_strings_in_list(self, some_list):
+        """
+        method to accept a list of strings as the parameter, find any strings
+        in that list that are comma separated, remove them from the list and add
+        their comma separated elements to the original list
+        """
+        new_list = []
+        remove_from_original_list = []
+        for element in some_list:
+            if ',' in element:
+                remove_from_original_list.append(element)
+                new_list.extend([e.strip() for e in element.split(',')])
+
+        for element in remove_from_original_list:
+            some_list.remove(element)
+
+        some_list.extend(new_list)
+
+        return some_list
 
     @abstractmethod
     def run(self):
diff --git a/lib/ansible/modules/packaging/os/dnf.py b/lib/ansible/modules/packaging/os/dnf.py
index 7b7ecf33bd..395ea824b8 100644
--- a/lib/ansible/modules/packaging/os/dnf.py
+++ b/lib/ansible/modules/packaging/os/dnf.py
@@ -283,6 +283,16 @@ class DnfModule(YumDnf):
 
         self._ensure_dnf()
 
+    def _sanitize_dnf_error_msg(self, spec, error):
+        """
+        For unhandled dnf.exceptions.Error scenarios, there are certain error
+        messages we want to filter. Do that here.
+        """
+        if to_text("no package matched") in to_text(error):
+            return "No package {0} available.".format(spec)
+
+        return error
+
     def _package_dict(self, package):
         """Return a dictionary of information for the package."""
         # NOTE: This no longer contains the 'dnfstate' field because it is
@@ -856,7 +866,9 @@ class DnfModule(YumDnf):
                         install_result = self._mark_package_install(pkg_spec)
                         if install_result['failed']:
                             failure_response['msg'] += install_result['msg']
-                            failure_response['failures'].append(install_result['failure'])
+                            failure_response['failures'].append(self._sanitize_dnf_error_msg(pkg_spec, install_result['failure']))
+                        else:
+                            response['results'].append(install_result['msg'])
 
             elif self.state == 'latest':
                 # "latest" is same as "installed" for filenames.
@@ -901,7 +913,9 @@ class DnfModule(YumDnf):
                         install_result = self._mark_package_install(pkg_spec, upgrade=True)
                         if install_result['failed']:
                             failure_response['msg'] += install_result['msg']
-                            failure_response['failures'].append(install_result['failure'])
+                            failure_response['failures'].append(self._sanitize_dnf_error_msg(pkg_spec, install_result['failure']))
+                        else:
+                            response['results'].append(install_result['msg'])
 
             else:
                 # state == absent
@@ -1075,7 +1089,12 @@ def main():
     try:
         module_implementation.run()
     except dnf.exceptions.RepoError as de:
-        module.exit_json(msg="Failed to synchronize repodata: {0}".format(to_native(de)))
+        module.fail_json(
+            msg="Failed to synchronize repodata: {0}".format(to_native(de)),
+            rc=1,
+            results=[],
+            changed=False
+        )
 
 
 if __name__ == '__main__':
diff --git a/test/integration/targets/dnf/tasks/dnf.yml b/test/integration/targets/dnf/tasks/dnf.yml
index 850701a852..0a373ca3b0 100644
--- a/test/integration/targets/dnf/tasks/dnf.yml
+++ b/test/integration/targets/dnf/tasks/dnf.yml
@@ -461,7 +461,7 @@
     that:
         - "dnf_result is failed"
         - "'non-existent-rpm' in dnf_result['failures'][0]"
-        - "'no package matched' in dnf_result['failures'][0]"
+        - "'No package non-existent-rpm available' in dnf_result['failures'][0]"
         - "'Failed to install some of the specified packages' in dnf_result['msg']"
 
 - name: use latest to install httpd