Fix snap module, and module helper behavior on rc != 0 in output (#2912)

* Try some snap fixes.

* Fix logic.

* Try to run tests privileged.

* Prevent failure on rc != 0.

* Fix formatting.

* Revert "Try to run tests privileged."

This reverts commit 77ca91f502.

* Try to run tests on RHEL instead.

* Make sure that snapd is running.

* Add changelog fragment.

* str -> to_native.

* Make sure that installed binary is actually found.

* Add check mode tests.

* Mention #2835 in changelog fragment.

changelogs/fragments/2912-snap-module-helper.yml

@@ -0,0 +1,3 @@
+bugfixes:
+  - module_helper module utils - avoid failing when non-zero ``rc`` is present on regular exit (https://github.com/ansible-collections/community.general/pull/2912).
+  - snap - fix various bugs which prevented the module from working at all, and which resulted in ``state=absent`` fail on absent snaps (https://github.com/ansible-collections/community.general/issues/2835, https://github.com/ansible-collections/community.general/issues/2906, https://github.com/ansible-collections/community.general/pull/2912).

plugins/module_utils/mh/base.py

@@ -59,4 +59,7 @@ class ModuleHelperBase(object):
         self.__init_module__()
         self.__run__()
         self.__quit_module__()
-        self.module.exit_json(changed=self.has_changed(), **self.output)
+        output = self.output
+        if 'failed' not in output:
+            output['failed'] = False
+        self.module.exit_json(changed=self.has_changed(), **output)

plugins/modules/packaging/os/snap.py

@@ -107,6 +107,8 @@ snaps_removed:
 
 import re
 
+from ansible.module_utils.common.text.converters import to_native
+
 from ansible_collections.community.general.plugins.module_utils.module_helper import (
     CmdStateModuleHelper, ArgFormat, ModuleHelperException
 )
@@ -123,7 +125,7 @@ __state_map = dict(
 
 
 def _state_map(value):
-    return __state_map[value]
+    return [__state_map[value]]
 
 
 class Snap(CmdStateModuleHelper):
@@ -163,20 +165,20 @@ class Snap(CmdStateModuleHelper):
                 results[i].append(output[i])
 
         return [
-            '; '.join(results[0]),
+            '; '.join([to_native(x) for x in results[0]]),
             self._first_non_zero(results[1]),
             '\n'.join(results[2]),
             '\n'.join(results[3]),
         ]
 
     def snap_exists(self, snap_name):
-        return 0 == self.run_command(params=[{'state': 'info'}, {'name': [snap_name]}])[0]
+        return 0 == self.run_command(params=[{'state': 'info'}, {'name': snap_name}])[0]
 
     def is_snap_installed(self, snap_name):
-        return 0 == self.run_command(params=[{'state': 'list'}, {'name': [snap_name]}])[0]
+        return 0 == self.run_command(params=[{'state': 'list'}, {'name': snap_name}])[0]
 
     def is_snap_enabled(self, snap_name):
-        rc, out, err = self.run_command(params=[{'state': 'list'}, {'name': [snap_name]}])
+        rc, out, err = self.run_command(params=[{'state': 'list'}, {'name': snap_name}])
         if rc != 0:
             return None
         result = out.splitlines()[1]
@@ -196,7 +198,7 @@ class Snap(CmdStateModuleHelper):
         self.validate_input_snaps()  # if snap doesnt exist, it will explode when trying to install
         self.vars.meta('classic').set(output=True)
         self.vars.meta('channel').set(output=True)
-        actionable_snaps = [s for s in self.vars.name if self.is_snap_installed(s)]
+        actionable_snaps = [s for s in self.vars.name if not self.is_snap_installed(s)]
         if not actionable_snaps:
             return
         self.changed = True
@@ -207,9 +209,9 @@ class Snap(CmdStateModuleHelper):
         has_one_pkg_params = bool(self.vars.classic) or self.vars.channel != 'stable'
         has_multiple_snaps = len(actionable_snaps) > 1
         if has_one_pkg_params and has_multiple_snaps:
-            commands = [params + [s] for s in actionable_snaps]
+            commands = [params + [{'actionable_snaps': [s]}] for s in actionable_snaps]
         else:
-            commands = [params + actionable_snaps]
+            commands = [params + [{'actionable_snaps': actionable_snaps}]]
         self.vars.cmd, rc, out, err = self._run_multiple_commands(commands)
         if rc == 0:
             return
@@ -227,7 +229,7 @@ class Snap(CmdStateModuleHelper):
 
     def state_absent(self):
         self.validate_input_snaps()  # if snap doesnt exist, it will be absent by definition
-        actionable_snaps = [s for s in self.vars.name if not self.is_snap_installed(s)]
+        actionable_snaps = [s for s in self.vars.name if self.is_snap_installed(s)]
         if not actionable_snaps:
             return
         self.changed = True
@@ -235,7 +237,7 @@ class Snap(CmdStateModuleHelper):
         if self.module.check_mode:
             return
         params = ['classic', 'channel', 'state']  # get base cmd parts
-        commands = [params + actionable_snaps]
+        commands = [params + [{'actionable_snaps': actionable_snaps}]]
         self.vars.cmd, rc, out, err = self._run_multiple_commands(commands)
         if rc == 0:
             return
@@ -253,7 +255,7 @@ class Snap(CmdStateModuleHelper):
         if self.module.check_mode:
             return
         params = ['classic', 'channel', 'state']  # get base cmd parts
-        commands = [params + actionable_snaps]
+        commands = [params + [{'actionable_snaps': actionable_snaps}]]
         self.vars.cmd, rc, out, err = self._run_multiple_commands(commands)
         if rc == 0:
             return
@@ -271,7 +273,7 @@ class Snap(CmdStateModuleHelper):
         if self.module.check_mode:
             return
         params = ['classic', 'channel', 'state']  # get base cmd parts
-        commands = [params + actionable_snaps]
+        commands = [params + [{'actionable_snaps': actionable_snaps}]]
         self.vars.cmd, rc, out, err = self._run_multiple_commands(commands)
         if rc == 0:
             return

tests/integration/targets/snap/aliases

@@ -3,4 +3,4 @@ skip/aix
 skip/freebsd
 skip/osx
 skip/macos
-disabled #FIXME 2609
+skip/docker

tests/integration/targets/snap/defaults/main.yml

@@ -0,0 +1,4 @@
+has_snap: false
+
+snap_packages:
+  - snapd

tests/integration/targets/snap/handlers/main.yml

@@ -0,0 +1,5 @@
+---
+- name: Remove snapd
+  package:
+    name: "{{ snap_packages }}"
+    state: absent

tests/integration/targets/snap/meta/main.yml

@@ -0,0 +1,3 @@
+dependencies:
+  - setup_pkg_mgr
+  - setup_epel

tests/integration/targets/snap/tasks/Debian.yml

@@ -0,0 +1 @@
+default.yml

tests/integration/targets/snap/tasks/Fedora.yml

@@ -0,0 +1 @@
+default.yml

tests/integration/targets/snap/tasks/RedHat.yml

@@ -0,0 +1 @@
+default.yml

tests/integration/targets/snap/tasks/default.yml

@@ -0,0 +1,15 @@
+---
+- name: Install snapd
+  package:
+    name: "{{ snap_packages }}"
+    state: present
+  notify: Remove snapd
+
+- name: Make sure that snapd is running
+  service:
+    name: snapd
+    state: started
+
+- name: Inform that snap is installed
+  set_fact:
+    has_snap: true

tests/integration/targets/snap/tasks/main.yml

@@ -4,28 +4,46 @@
 # and should not be used as examples of how to write Ansible roles #
 ####################################################################
 
-- name: install snapd
+- name: Include distribution specific tasks
-  apt:
+  include_tasks: "{{ lookup('first_found', params) }}"
-    name: snapd
+  vars:
-    state: present
+    params:
-  register: snapd_install_ubuntu
+      files:
-  when: ansible_distribution == 'Ubuntu'
+        - "{{ ansible_facts.distribution }}-{{ ansible_facts.distribution_major_version }}.yml"
-
+        - "{{ ansible_facts.os_family }}-{{ ansible_facts.distribution_major_version }}.yml"
-- name: install snapd
+        - "{{ ansible_facts.distribution }}.yml"
-  dnf:
+        - "{{ ansible_facts.os_family }}.yml"
-    name: snapd
+        - "nothing.yml"
-    state: present
+      paths:
-  register: snapd_install_fedora
+        - "{{ role_path }}/tasks"
-  when: ansible_distribution == 'Fedora'
 
 - block:
-    - name: install package
+    - name: Make sure package is not installed
+      community.general.snap:
+        name: hello-world
+        state: absent
+
+    - name: Install package (check mode)
+      community.general.snap:
+        name: hello-world
+        state: present
+      register: install_check
+      check_mode: true
+
+    - name: Install package
       community.general.snap:
         name: hello-world
         state: present
       register: install
 
-    - name: install package again
+    - name: Install package again (check mode)
+      community.general.snap:
+        name: hello-world
+        state: present
+      register: install_again_check
+      check_mode: true
+
+    - name: Install package again
       community.general.snap:
         name: hello-world
         state: present
@@ -35,18 +53,36 @@
       assert:
         that:
           - install is changed
+          - install_check is changed
           - install_again is not changed
+          - install_again_check is not changed
 
-    - name: check package has been installed correctly
+    - name: Check package has been installed correctly
       command: hello-world
+      environment:
+        PATH: /var/lib/snapd/snap/bin/
 
-    - name: remove package
+    - name: Remove package (check mode)
+      community.general.snap:
+        name: hello-world
+        state: absent
+      register: remove_check
+      check_mode: true
+
+    - name: Remove package
       community.general.snap:
         name: hello-world
         state: absent
       register: remove
 
-    - name: remove package again
+    - name: Remove package again (check mode)
+      community.general.snap:
+        name: hello-world
+        state: absent
+      register: remove_again_check
+      check_mode: true
+
+    - name: Remove package again
       community.general.snap:
         name: hello-world
         state: absent
@@ -56,17 +92,7 @@
       assert:
         that:
           - remove is changed
+          - remove_check is changed
           - remove_again is not changed
-  when: ansible_distribution in ['Ubuntu','Fedora']
+          - remove_again_check is not changed
-
+  when: has_snap
-- name: Remove snapd in case it was not installed
-  apt:
-    name: snapd
-    state: absent
-  when: snapd_install_ubuntu is changed and ansible_distribution == 'Ubuntu'
-
-- name: Remove snapd in case it was not installed
-  dnf:
-    name: snapd
-    state: absent
-  when: snapd_install_fedora is changed and ansible_distribution == 'Fedora'

tests/integration/targets/snap/tasks/nothing.yml

@@ -0,0 +1,2 @@
+---
+# Do nothing