yum: fix changed in group remove

lib/ansible/modules/packaging/os/yum.py

@@ -342,6 +342,18 @@ def po_to_nevra(po):
     else:
         return '%s-%s-%s.%s' % (po.name, po.version, po.release, po.arch)
 
+
+def is_group_installed(name):
+    my = yum_base()
+    groups_list = my.doGroupLists()
+    for group in groups_list[0]:  # list of the installed groups on the first index
+        name_lower = name.lower()
+        if name_lower.endswith(group.name.lower()) or name_lower.endswith(group.groupid.lower()):
+            return True
+
+    return False
+
+
 def is_installed(module, repoq, pkgspec, conf_file, qf=def_qf, en_repos=None, dis_repos=None, is_pkg=False, installroot='/'):
     if en_repos is None:
         en_repos = []
@@ -746,16 +758,7 @@ def install(module, items, repoq, yum_basecmd, conf_file, en_repos, dis_repos, i
 
         # groups
         elif spec.startswith('@'):
-            found = False
+            if is_group_installed(spec):
-            my = yum_base()
-            groups_list = my.doGroupLists()
-            for group in groups_list[0]:  # list of the installed groups on the first index
-                spec_lower = spec.lower()
-                if spec_lower.endswith(group.name.lower()) or spec_lower.endswith(group.groupid.lower()):
-                    found = True
-                    break
-
-            if found:
                 continue
 
             pkg = spec
@@ -860,47 +863,48 @@ def remove(module, items, repoq, yum_basecmd, conf_file, en_repos, dis_repos, in
     res['rc'] = 0
 
     for pkg in items:
-        is_group = False
-        # group remove - this is doom on a stick
         if pkg.startswith('@'):
-            is_group = True # nopep8 this will be fixed in next MR this module needs major rewrite anyway.
+            installed = is_group_installed(pkg)
         else:
-            if not is_installed(module, repoq, pkg, conf_file, en_repos=en_repos, dis_repos=dis_repos, installroot=installroot):
+            installed = is_installed(module, repoq, pkg, conf_file, en_repos=en_repos, dis_repos=dis_repos, installroot=installroot)
-                res['results'].append('%s is not installed' % pkg)
-                continue
 
+        if installed:
             pkgs.append(pkg)
+        else:
+            res['results'].append('%s is not installed' % pkg)
 
     if pkgs:
-        # run an actual yum transaction
-        cmd = yum_basecmd + ["remove"] + pkgs
-
         if module.check_mode:
             module.exit_json(changed=True, results=res['results'], changes=dict(removed=pkgs))
 
+        # run an actual yum transaction
+        cmd = yum_basecmd + ["remove"] + pkgs
+
         rc, out, err = module.run_command(cmd)
 
         res['rc'] = rc
         res['results'].append(out)
         res['msg'] = err
 
+        if rc != 0:
+            module.fail_json(**res)
+
         # compile the results into one batch. If anything is changed
         # then mark changed
         # at the end - if we've end up failed then fail out of the rest
         # of the process
 
-        # at this point we should check to see if the pkg is no longer present
+        # at this point we check to see if the pkg is no longer present
-
         for pkg in pkgs:
-            if not pkg.startswith('@'): # we can't sensibly check for a group being uninstalled reliably
+            if pkg.startswith('@'):
-                # look to see if the pkg shows up from is_installed. If it doesn't
+                installed = is_group_installed(pkg)
-                if not is_installed(module, repoq, pkg, conf_file, en_repos=en_repos, dis_repos=dis_repos, installroot=installroot):
-                    res['changed'] = True
             else:
+                installed = is_installed(module, repoq, pkg, conf_file, en_repos=en_repos, dis_repos=dis_repos, installroot=installroot)
+
+            if installed:
                 module.fail_json(**res)
 
-        if rc != 0:
+        res['changed'] = True
-            module.fail_json(**res)
 
     return res
 

test/integration/targets/yum/tasks/main.yml

@@ -19,14 +19,23 @@
 # Note: We install the yum package onto Fedora so that this will work on dnf systems
 # We want to test that for people who don't want to upgrade their systems.
 - include: 'yum.yml'
-  when: ansible_distribution in ['RedHat', 'CentOS', 'ScientificLinux', 'Fedora'] and
+  when:
-        ansible_python.version.major == 2
+    - ansible_distribution in ['RedHat', 'CentOS', 'ScientificLinux', 'Fedora']
+    - ansible_python.version.major == 2
 
 # We can't run yum --installroot tests on dnf systems.  Dnf systems revert to
 # yum-deprecated, and yum-deprecated refuses to run if yum.conf exists
 # so we cannot configure yum-deprecated correctly in an empty /tmp/fake.root/
 # It will always run with $releasever unset
 - include: 'yuminstallroot.yml'
-  when: (ansible_distribution in ['RedHat', 'CentOS', 'ScientificLinux'] or
+  when:
-        (ansible_distribution in ['Fedora'] and ansible_distribution_major_version|int < 23)) and
+    - (ansible_distribution in ['RedHat', 'CentOS', 'ScientificLinux'] or (ansible_distribution in ['Fedora'] and ansible_distribution_major_version|int < 23))
-        ansible_python.version.major == 2
+    - ansible_python.version.major == 2
+
+# el6 has a broken yum group implementation, when you try to remove a group it goes through
+# deps and ends up with trying to remove yum itself and the whole process fails
+# so don't run the yum group remove tests there
+- include: 'yum_group_remove.yml'
+  when:
+    - (ansible_distribution in ['RedHat', 'CentOS', 'ScientificLinux'] and ansible_distribution_major_version|int > 6) or ansible_distribution in ['Fedora']
+    - ansible_python.version.major == 2

test/integration/targets/yum/tasks/yum_group_remove.yml

@@ -0,0 +1,142 @@
+- name: install a group to test and yum-utils
+  yum:
+    name: "{{ item }}"
+    state: present
+  with_items:
+    - "@Development Tools"
+    - yum-utils
+
+- name: check mode remove the group
+  yum:
+    name: "@Development Tools"
+    state: absent
+  check_mode: yes
+  register: yum_result
+
+- name: verify changed
+  assert:
+    that:
+        - "yum_result.changed"
+
+- name: verify yum module outputs
+  assert:
+    that:
+        - "'changed' in yum_result"
+        - "'results' in yum_result"
+
+- name: remove the group
+  yum:
+    name: "@Development Tools"
+    state: absent
+  register: yum_result
+
+- name: verify changed
+  assert:
+    that:
+        - "yum_result.rc == 0"
+        - "yum_result.changed"
+
+- name: verify yum module outputs
+  assert:
+    that:
+        - "'changed' in yum_result"
+        - "'msg' in yum_result"
+        - "'results' in yum_result"
+
+- name: remove the group again
+  yum:
+    name: "@Development Tools"
+    state: absent
+  register: yum_result
+
+- name: verify changed
+  assert:
+    that:
+        - "not yum_result.changed"
+
+- name: verify yum module outputs
+  assert:
+    that:
+        - "'changed' in yum_result"
+        - "'msg' in yum_result"
+        - "'results' in yum_result"
+
+- name: check mode remove the group again
+  yum:
+    name: "@Development Tools"
+    state: absent
+  check_mode: yes
+  register: yum_result
+
+- name: verify changed
+  assert:
+    that:
+        - "not yum_result.changed"
+
+- name: verify yum module outputs
+  assert:
+    that:
+        - "'changed' in yum_result"
+        - "'results' in yum_result"
+
+- name: install a group and a package to test
+  yum:
+    name: "@Development Tools,sos"
+    state: present
+  register: yum_output
+
+- debug: var=yum_output
+
+- name: check mode remove the group along with the package
+  yum:
+    name: "@Development Tools,sos"
+    state: absent
+  register: yum_result
+  check_mode: yes
+
+- name: verify changed
+  assert:
+    that:
+        - "yum_result.changed"
+
+- name: verify yum module outputs
+  assert:
+    that:
+        - "'changed' in yum_result"
+        - "'results' in yum_result"
+
+- name: remove the group along with the package
+  yum:
+    name: "@Development Tools,sos"
+    state: absent
+  register: yum_result
+
+- name: verify changed
+  assert:
+    that:
+        - "yum_result.changed"
+
+- name: verify yum module outputs
+  assert:
+    that:
+        - "'changed' in yum_result"
+        - "'msg' in yum_result"
+        - "'results' in yum_result"
+
+- name: check mode remove the group along with the package
+  yum:
+    name: "@Development Tools,sos"
+    state: absent
+  register: yum_result
+  check_mode: yes
+
+- name: verify not changed
+  assert:
+    that:
+        - "not yum_result.changed"
+
+- name: verify yum module outputs
+  assert:
+    that:
+        - "'changed' in yum_result"
+        - "'results' in yum_result"