pacman: improve docs, make sure that packages is always returned, deprecate update_cache behavior (#4330)

* Improve docs, make sure that packages is always returned, deprecate update_cache behavior.

* Add cache_updated return value.

* Revert "Add cache_updated return value."

This reverts commit 367297bb5c.

* Update tests/unit/plugins/modules/packaging/os/test_pacman.py

Co-authored-by: Jean Raby <jean@raby.sh>

Co-authored-by: Jean Raby <jean@raby.sh>

changelogs/fragments/4330-pacman-packages-update_cache.yml

@@ -0,0 +1,7 @@
+bugfixes:
+  - "pacman - make sure that ``packages`` is always returned when ``name`` or ``upgrade`` is specified, also if nothing is done
+     (https://github.com/ansible-collections/community.general/pull/4329)."
+deprecated_features:
+  - "pacman - from community.general 5.0.0 on, the ``changed`` status of ``update_cache`` will no longer be ignored if ``name`` or ``upgrade`` is specified.
+     To keep the old behavior, add something like ``register: result`` and ``changed_when: result.packages | length > 0`` to your task
+     (https://github.com/ansible-collections/community.general/pull/4329)."

plugins/modules/packaging/os/pacman.py

@@ -81,6 +81,8 @@ options:
             - This can be run as part of a package installation or as a separate step.
             - Alias C(update-cache) has been deprecated and will be removed in community.general 5.0.0.
             - If not specified, it defaults to C(false).
+            - Please note that this option will only have an influence on the module's C(changed) state
+              if I(name) and I(upgrade) are not specified. This will change in community.general 5.0.0.
         type: bool
         aliases: [ update-cache ]
 
@@ -112,20 +114,28 @@ notes:
 
 RETURN = """
 packages:
-    description: a list of packages that have been changed
+    description:
-    returned: when upgrade is set to yes
+        - A list of packages that have been changed.
+        - Before community.general 4.5.0 this was only returned when I(upgrade=true).
+          In community.general 4.5.0, it was sometimes omitted when the package list is empty,
+          but since community.general 4.6.0 it is always returned when I(name) is specified or
+          I(upgrade=true).
+    returned: success and I(name) is specified or I(upgrade=true)
     type: list
+    elements: str
     sample: [ package, other-package ]
 
 stdout:
-    description: Output from pacman.
+    description:
+        - Output from pacman.
     returned: success, when needed
     type: str
     sample: ":: Synchronizing package databases...  core is up to date :: Starting full system upgrade..."
     version_added: 4.1.0
 
 stderr:
-    description: Error output from pacman.
+    description:
+        - Error output from pacman.
     returned: success, when needed
     type: str
     sample: "warning: libtool: local (2.4.6+44+gb9b44533-14) is newer than core (2.4.6+42+gb88cebd5-15)\nwarning ..."
@@ -158,6 +168,9 @@ EXAMPLES = """
     extra_args: --builddir /var/cache/yay
 
 - name: Upgrade package foo
+  # The 'changed' state of this call will only indicate whether foo was
+  # installed/upgraded, but not on whether the cache was updated. This
+  # will change in community.general 5.0.0!
   community.general.pacman:
     name: foo
     state: latest
@@ -185,6 +198,9 @@ EXAMPLES = """
     upgrade: yes
 
 - name: Run the equivalent of "pacman -Syu" as a separate step
+  # The 'changed' state of this call will only indicate whether
+  # something was upgraded, but not on whether the cache was
+  # updated. This will change in community.general 5.0.0!
   community.general.pacman:
     update_cache: yes
     upgrade: yes
@@ -280,9 +296,8 @@ class Pacman(object):
                 self.success()
 
         # Avoid shadowing lack of changes in the following stages
-        # so that update_cache: yes doesn't always return changed
+        # so that `update_cache: true` doesn't always return changed
-        # TODO: remove this when update_cache is tweaked to report its real
+        # TODO: remove this in community.general 5.0.0
-        # changed status (i.e. no changed if package lists were up to date)
         self.changed = False
 
         self.inventory = self._build_inventory()
@@ -320,6 +335,7 @@ class Pacman(object):
                 pkgs_to_install.append(p)
 
         if len(pkgs_to_install) == 0 and len(pkgs_to_install_from_url) == 0:
+            self.exit_params["packages"] = []
             self.add_exit_infos("package(s) already installed")
             return
 
@@ -374,6 +390,7 @@ class Pacman(object):
 
         if len(installed_pkgs) == 0:
             # This can happen with URL packages if pacman decides there's nothing to do
+            self.exit_params["packages"] = []
             self.add_exit_infos("package(s) already installed")
             return
 
@@ -410,6 +427,7 @@ class Pacman(object):
         pkg_names_to_remove = [p.name for p in pkgs if p.name in self.inventory["installed_pkgs"]]
 
         if len(pkg_names_to_remove) == 0:
+            self.exit_params["packages"] = []
             self.add_exit_infos("package(s) already absent")
             return
 

tests/unit/plugins/modules/packaging/os/test_pacman.py

@@ -325,6 +325,7 @@ class TestPacman:
             P.run()
         self.mock_run_command.call_count == 0
         out = e.value.args[0]
+        assert "packages" not in out
         assert out["changed"]
 
     @pytest.mark.parametrize(
@@ -582,8 +583,8 @@ class TestPacman:
         with pytest.raises(AnsibleExitJson) as e:
             P.run()
         out = e.value.args[0]
-        assert "packages" not in out
         assert not out["changed"]
+        assert "packages" in out
         assert "diff" not in out
         self.mock_run_command.call_count == 0
 
@@ -1010,6 +1011,7 @@ class TestPacman:
         if raises == AnsibleExitJson:
             assert out["packages"] == expected_packages
             assert out["changed"]
+            assert "packages" in out
             assert "diff" in out
         else:
             assert out["stdout"] == "stdout"