puppet: refactored to use CmdRunner (#5612)

* puppet: refactored to use CmdRunner

* add changelog fragment

* add more tests

.github/BOTMETA.yml

@@ -309,6 +309,9 @@ files:
   $module_utils/pipx.py:
     labels: pipx
     maintainers: russoz
+  $module_utils/puppet.py:
+    labels: puppet
+    maintainers: russoz
   $module_utils/pure.py:
     labels: pure pure_storage
     maintainers: $team_purestorage

changelogs/fragments/5612-puppet-cmd-runner.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - puppet - refactored module to use ``CmdRunner`` for executing ``puppet`` (https://github.com/ansible-collections/community.general/pull/5612).

plugins/module_utils/puppet.py

@@ -0,0 +1,114 @@
+# -*- coding: utf-8 -*-
+# Copyright (c) 2022, Alexei Znamensky <russoz@gmail.com>
+# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt)
+# SPDX-License-Identifier: GPL-3.0-or-later
+
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+
+import os
+
+from ansible_collections.community.general.plugins.module_utils.cmd_runner import CmdRunner, cmd_runner_fmt
+
+
+_PUPPET_PATH_PREFIX = ["/opt/puppetlabs/bin"]
+
+
+def get_facter_dir():
+    if os.getuid() == 0:
+        return '/etc/facter/facts.d'
+    else:
+        return os.path.expanduser('~/.facter/facts.d')
+
+
+def _puppet_cmd(module):
+    return module.get_bin_path("puppet", False, _PUPPET_PATH_PREFIX)
+
+
+# If the `timeout` CLI command feature is removed,
+# Then we could add this as a fixed param to `puppet_runner`
+def ensure_agent_enabled(module):
+    runner = CmdRunner(
+        module,
+        command="puppet",
+        path_prefix=_PUPPET_PATH_PREFIX,
+        arg_formats=dict(
+            _agent_disabled=cmd_runner_fmt.as_fixed(['config', 'print', 'agent_disabled_lockfile']),
+        ),
+        check_rc=False,
+    )
+
+    rc, stdout, stderr = runner("_agent_disabled").run()
+    if os.path.exists(stdout.strip()):
+        module.fail_json(
+            msg="Puppet agent is administratively disabled.",
+            disabled=True)
+    elif rc != 0:
+        module.fail_json(
+            msg="Puppet agent state could not be determined.")
+
+
+def puppet_runner(module):
+
+    # Keeping backward compatibility, allow for running with the `timeout` CLI command.
+    # If this can be replaced with ansible `timeout` parameter in playbook,
+    # then this function could be removed.
+    def _prepare_base_cmd():
+        _tout_cmd = module.get_bin_path("timeout", False)
+        if _tout_cmd:
+            cmd = ["timeout", "-s", "9", module.params["timeout"], _puppet_cmd(module)]
+        else:
+            cmd = ["puppet"]
+        return cmd
+
+    def noop_func(v):
+        _noop = cmd_runner_fmt.as_map({
+            True: "--noop",
+            False: "--no-noop",
+        })
+        return _noop(module.check_mode or v)
+
+    _logdest_map = {
+        "syslog": ["--logdest", "syslog"],
+        "all": ["--logdest", "syslog", "--logdest", "console"],
+    }
+
+    @cmd_runner_fmt.unpack_args
+    def execute_func(execute, manifest):
+        if execute:
+            return ["--execute", execute]
+        else:
+            return [manifest]
+
+    runner = CmdRunner(
+        module,
+        command=_prepare_base_cmd(),
+        path_prefix=_PUPPET_PATH_PREFIX,
+        arg_formats=dict(
+            _agent_fixed=cmd_runner_fmt.as_fixed([
+                "agent", "--onetime", "--no-daemonize", "--no-usecacheonfailure",
+                "--no-splay", "--detailed-exitcodes", "--verbose", "--color", "0",
+            ]),
+            _apply_fixed=cmd_runner_fmt.as_fixed(["apply", "--detailed-exitcodes"]),
+            puppetmaster=cmd_runner_fmt.as_opt_val("--server"),
+            show_diff=cmd_runner_fmt.as_bool("--show-diff"),
+            confdir=cmd_runner_fmt.as_opt_val("--confdir"),
+            environment=cmd_runner_fmt.as_opt_val("--environment"),
+            tags=cmd_runner_fmt.as_func(lambda v: ["--tags", ",".join(v)]),
+            certname=cmd_runner_fmt.as_opt_eq_val("--certname"),
+            noop=cmd_runner_fmt.as_func(noop_func),
+            use_srv_records=cmd_runner_fmt.as_map({
+                True: "--usr_srv_records",
+                False: "--no-usr_srv_records",
+            }),
+            logdest=cmd_runner_fmt.as_map(_logdest_map, default=[]),
+            modulepath=cmd_runner_fmt.as_opt_eq_val("--modulepath"),
+            _execute=cmd_runner_fmt.as_func(execute_func),
+            summarize=cmd_runner_fmt.as_bool("--summarize"),
+            debug=cmd_runner_fmt.as_bool("--debug"),
+            verbose=cmd_runner_fmt.as_bool("--verbose"),
+        ),
+        check_rc=False,
+    )
+    return runner

plugins/modules/puppet.py

@@ -152,15 +152,9 @@ import json
 import os
 import stat
 
+import ansible_collections.community.general.plugins.module_utils.puppet as puppet_utils
+
 from ansible.module_utils.basic import AnsibleModule
-from ansible.module_utils.six.moves import shlex_quote
-
-
-def _get_facter_dir():
-    if os.getuid() == 0:
-        return '/etc/facter/facts.d'
-    else:
-        return os.path.expanduser('~/.facter/facts.d')
 
 
 def _write_structured_data(basedir, basename, data):
@@ -212,16 +206,6 @@ def main():
     )
     p = module.params
 
-    global PUPPET_CMD
-    PUPPET_CMD = module.get_bin_path("puppet", False, ['/opt/puppetlabs/bin'])
-
-    if not PUPPET_CMD:
-        module.fail_json(
-            msg="Could not find puppet. Please ensure it is installed.")
-
-    global TIMEOUT_CMD
-    TIMEOUT_CMD = module.get_bin_path("timeout", False)
-
     if p['manifest']:
         if not os.path.exists(p['manifest']):
             module.fail_json(
@@ -230,90 +214,24 @@ def main():
 
     # Check if puppet is disabled here
     if not p['manifest']:
-        rc, stdout, stderr = module.run_command(
+        puppet_utils.ensure_agent_enabled(module)
-            PUPPET_CMD + " config print agent_disabled_lockfile")
-        if os.path.exists(stdout.strip()):
-            module.fail_json(
-                msg="Puppet agent is administratively disabled.",
-                disabled=True)
-        elif rc != 0:
-            module.fail_json(
-                msg="Puppet agent state could not be determined.")
 
     if module.params['facts'] and not module.check_mode:
         _write_structured_data(
-            _get_facter_dir(),
+            puppet_utils.get_facter_dir(),
             module.params['facter_basename'],
             module.params['facts'])
 
-    if TIMEOUT_CMD:
+    runner = puppet_utils.puppet_runner(module)
-        base_cmd = "%(timeout_cmd)s -s 9 %(timeout)s %(puppet_cmd)s" % dict(
-            timeout_cmd=TIMEOUT_CMD,
-            timeout=shlex_quote(p['timeout']),
-            puppet_cmd=PUPPET_CMD)
-    else:
-        base_cmd = PUPPET_CMD
 
     if not p['manifest'] and not p['execute']:
-        cmd = ("%(base_cmd)s agent --onetime"
+        args_order = "_agent_fixed puppetmaster show_diff confdir environment tags certname noop use_srv_records"
-               " --no-daemonize --no-usecacheonfailure --no-splay"
+        with runner(args_order) as ctx:
-               " --detailed-exitcodes --verbose --color 0") % dict(base_cmd=base_cmd)
+            rc, stdout, stderr = ctx.run()
-        if p['puppetmaster']:
-            cmd += " --server %s" % shlex_quote(p['puppetmaster'])
-        if p['show_diff']:
-            cmd += " --show_diff"
-        if p['confdir']:
-            cmd += " --confdir %s" % shlex_quote(p['confdir'])
-        if p['environment']:
-            cmd += " --environment '%s'" % p['environment']
-        if p['tags']:
-            cmd += " --tags '%s'" % ','.join(p['tags'])
-        if p['certname']:
-            cmd += " --certname='%s'" % p['certname']
-        if module.check_mode:
-            cmd += " --noop"
-        elif 'noop' in p:
-            if p['noop']:
-                cmd += " --noop"
-            else:
-                cmd += " --no-noop"
-        if p['use_srv_records'] is not None:
-            if not p['use_srv_records']:
-                cmd += " --no-use_srv_records"
-            else:
-                cmd += " --use_srv_records"
     else:
-        cmd = "%s apply --detailed-exitcodes " % base_cmd
+        args_order = "_apply_fixed logdest modulepath environment certname tags noop _execute summarize debug verbose"
-        if p['logdest'] == 'syslog':
+        with runner(args_order) as ctx:
-            cmd += "--logdest syslog "
+            rc, stdout, stderr = ctx.run(_execute=[p['execute'], p['manifest']])
-        if p['logdest'] == 'all':
-            cmd += " --logdest syslog --logdest console"
-        if p['modulepath']:
-            cmd += "--modulepath='%s'" % p['modulepath']
-        if p['environment']:
-            cmd += "--environment '%s' " % p['environment']
-        if p['certname']:
-            cmd += " --certname='%s'" % p['certname']
-        if p['tags']:
-            cmd += " --tags '%s'" % ','.join(p['tags'])
-        if module.check_mode:
-            cmd += "--noop "
-        elif 'noop' in p:
-            if p['noop']:
-                cmd += " --noop"
-            else:
-                cmd += " --no-noop"
-        if p['execute']:
-            cmd += " --execute '%s'" % p['execute']
-        else:
-            cmd += " %s" % shlex_quote(p['manifest'])
-        if p['summarize']:
-            cmd += " --summarize"
-        if p['debug']:
-            cmd += " --debug"
-        if p['verbose']:
-            cmd += " --verbose"
-    rc, stdout, stderr = module.run_command(cmd)
 
     if rc == 0:
         # success
@@ -335,11 +253,11 @@ def main():
     elif rc == 124:
         # timeout
         module.exit_json(
-            rc=rc, msg="%s timed out" % cmd, stdout=stdout, stderr=stderr)
+            rc=rc, msg="%s timed out" % ctx.cmd, stdout=stdout, stderr=stderr)
     else:
         # failure
         module.fail_json(
-            rc=rc, msg="%s failed with return code: %d" % (cmd, rc),
+            rc=rc, msg="%s failed with return code: %d" % (ctx.cmd, rc),
             stdout=stdout, stderr=stderr)
 
 

tests/sanity/ignore-2.11.txt

@@ -18,8 +18,7 @@ plugins/modules/manageiq_provider.py validate-modules:undocumented-parameter
 plugins/modules/manageiq_tags.py validate-modules:parameter-state-invalid-choice
 plugins/modules/osx_defaults.py validate-modules:parameter-state-invalid-choice
 plugins/modules/parted.py validate-modules:parameter-state-invalid-choice
-plugins/modules/puppet.py use-argspec-type-path
+plugins/modules/puppet.py validate-modules:parameter-invalid                          # invalid alias - removed in 7.0.0
-plugins/modules/puppet.py validate-modules:parameter-invalid  # invalid alias - removed in 7.0.0
 plugins/modules/rax_files_objects.py use-argspec-type-path
 plugins/modules/rax_files.py validate-modules:parameter-state-invalid-choice
 plugins/modules/rax.py use-argspec-type-path # fix needed

tests/sanity/ignore-2.12.txt

@@ -13,8 +13,7 @@ plugins/modules/manageiq_provider.py validate-modules:undocumented-parameter
 plugins/modules/manageiq_tags.py validate-modules:parameter-state-invalid-choice
 plugins/modules/osx_defaults.py validate-modules:parameter-state-invalid-choice
 plugins/modules/parted.py validate-modules:parameter-state-invalid-choice
-plugins/modules/puppet.py use-argspec-type-path
+plugins/modules/puppet.py validate-modules:parameter-invalid                          # invalid alias - removed in 7.0.0
-plugins/modules/puppet.py validate-modules:parameter-invalid  # invalid alias - removed in 7.0.0
 plugins/modules/rax_files_objects.py use-argspec-type-path
 plugins/modules/rax_files.py validate-modules:parameter-state-invalid-choice
 plugins/modules/rax.py use-argspec-type-path # fix needed

tests/sanity/ignore-2.13.txt

@@ -13,8 +13,7 @@ plugins/modules/manageiq_provider.py validate-modules:undocumented-parameter
 plugins/modules/manageiq_tags.py validate-modules:parameter-state-invalid-choice
 plugins/modules/osx_defaults.py validate-modules:parameter-state-invalid-choice
 plugins/modules/parted.py validate-modules:parameter-state-invalid-choice
-plugins/modules/puppet.py use-argspec-type-path
+plugins/modules/puppet.py validate-modules:parameter-invalid                          # invalid alias - removed in 7.0.0
-plugins/modules/puppet.py validate-modules:parameter-invalid  # invalid alias - removed in 7.0.0
 plugins/modules/rax_files_objects.py use-argspec-type-path
 plugins/modules/rax_files.py validate-modules:parameter-state-invalid-choice
 plugins/modules/rax.py use-argspec-type-path # fix needed

tests/sanity/ignore-2.14.txt

@@ -14,8 +14,7 @@ plugins/modules/manageiq_provider.py validate-modules:undocumented-parameter
 plugins/modules/manageiq_tags.py validate-modules:parameter-state-invalid-choice
 plugins/modules/osx_defaults.py validate-modules:parameter-state-invalid-choice
 plugins/modules/parted.py validate-modules:parameter-state-invalid-choice
-plugins/modules/puppet.py use-argspec-type-path
+plugins/modules/puppet.py validate-modules:parameter-invalid                          # invalid alias - removed in 7.0.0
-plugins/modules/puppet.py validate-modules:parameter-invalid  # invalid alias - removed in 7.0.0
 plugins/modules/rax_files_objects.py use-argspec-type-path
 plugins/modules/rax_files.py validate-modules:parameter-state-invalid-choice
 plugins/modules/rax.py use-argspec-type-path # fix needed

tests/sanity/ignore-2.15.txt

@@ -14,8 +14,7 @@ plugins/modules/manageiq_provider.py validate-modules:undocumented-parameter
 plugins/modules/manageiq_tags.py validate-modules:parameter-state-invalid-choice
 plugins/modules/osx_defaults.py validate-modules:parameter-state-invalid-choice
 plugins/modules/parted.py validate-modules:parameter-state-invalid-choice
-plugins/modules/puppet.py use-argspec-type-path
+plugins/modules/puppet.py validate-modules:parameter-invalid                          # invalid alias - removed in 7.0.0
-plugins/modules/puppet.py validate-modules:parameter-invalid  # invalid alias - removed in 7.0.0
 plugins/modules/rax_files_objects.py use-argspec-type-path
 plugins/modules/rax_files.py validate-modules:parameter-state-invalid-choice
 plugins/modules/rax.py use-argspec-type-path # fix needed

tests/unit/plugins/modules/test_puppet.py

@@ -0,0 +1,145 @@
+# -*- coding: utf-8 -*-
+# Author: Alexei Znamensky (russoz@gmail.com)
+# Largely adapted from test_redhat_subscription by
+# Jiri Hnidek (jhnidek@redhat.com)
+#
+# Copyright (c) Alexei Znamensky (russoz@gmail.com)
+# Copyright (c) Jiri Hnidek (jhnidek@redhat.com)
+#
+# GNU General Public License v3.0+ (see LICENSES/GPL-3.0-or-later.txt or https://www.gnu.org/licenses/gpl-3.0.txt)
+# SPDX-License-Identifier: GPL-3.0-or-later
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import json
+
+from ansible_collections.community.general.plugins.modules import puppet
+
+import pytest
+
+TESTED_MODULE = puppet.__name__
+
+
+@pytest.fixture
+def patch_get_bin_path(mocker):
+    """
+    Function used for mocking AnsibleModule.get_bin_path
+    """
+    def mockie(self, path, *args, **kwargs):
+        return "/testbin/{0}".format(path)
+    mocker.patch("ansible.module_utils.basic.AnsibleModule.get_bin_path", mockie)
+
+
+TEST_CASES = [
+    [
+        {},
+        {
+            "id": "puppet_agent_plain",
+            "run_command.calls": [
+                (
+                    ["/testbin/puppet", "config", "print", "agent_disabled_lockfile"],
+                    {"environ_update": {"LANGUAGE": "C", "LC_ALL": "C"}, "check_rc": False},
+                    (0, "blah, anything", "",),  # output rc, out, err
+                ),
+                (
+                    [
+                        "/testbin/timeout", "-s", "9", "30m", "/testbin/puppet", "agent", "--onetime", "--no-daemonize",
+                        "--no-usecacheonfailure", "--no-splay", "--detailed-exitcodes", "--verbose", "--color", "0"
+                    ],
+                    {"environ_update": {"LANGUAGE": "C", "LC_ALL": "C"}, "check_rc": False},
+                    (0, "", "",),  # output rc, out, err
+                ),
+            ],
+            "changed": False,
+        }
+    ],
+    [
+        {
+            "certname": "potatobox"
+        },
+        {
+            "id": "puppet_agent_certname",
+            "run_command.calls": [
+                (
+                    ["/testbin/puppet", "config", "print", "agent_disabled_lockfile"],
+                    {"environ_update": {"LANGUAGE": "C", "LC_ALL": "C"}, "check_rc": False},
+                    (0, "blah, anything", "",),  # output rc, out, err
+                ),
+                (
+                    [
+                        "/testbin/timeout", "-s", "9", "30m", "/testbin/puppet", "agent", "--onetime", "--no-daemonize",
+                        "--no-usecacheonfailure", "--no-splay", "--detailed-exitcodes", "--verbose", "--color", "0", "--certname=potatobox"
+                    ],
+                    {"environ_update": {"LANGUAGE": "C", "LC_ALL": "C"}, "check_rc": False},
+                    (0, "", "",),  # output rc, out, err
+                ),
+            ],
+            "changed": False,
+        }
+    ],
+    [
+        {
+            "tags": ["a", "b", "c"]
+        },
+        {
+            "id": "puppet_agent_tags_abc",
+            "run_command.calls": [
+                (
+                    ["/testbin/puppet", "config", "print", "agent_disabled_lockfile"],
+                    {"environ_update": {"LANGUAGE": "C", "LC_ALL": "C"}, "check_rc": False},
+                    (0, "blah, anything", "",),  # output rc, out, err
+                ),
+                (
+                    [
+                        "/testbin/timeout", "-s", "9", "30m", "/testbin/puppet", "agent", "--onetime", "--no-daemonize",
+                        "--no-usecacheonfailure", "--no-splay", "--detailed-exitcodes", "--verbose", "--color", "0", "--tags", "a,b,c"
+                    ],
+                    {"environ_update": {"LANGUAGE": "C", "LC_ALL": "C"}, "check_rc": False},
+                    (0, "", "",),  # output rc, out, err
+                ),
+            ],
+            "changed": False,
+        }
+    ],
+]
+TEST_CASES_IDS = [item[1]["id"] for item in TEST_CASES]
+
+
+@pytest.mark.parametrize("patch_ansible_module, testcase",
+                         TEST_CASES,
+                         ids=TEST_CASES_IDS,
+                         indirect=["patch_ansible_module"])
+@pytest.mark.usefixtures("patch_ansible_module")
+def test_puppet(mocker, capfd, patch_get_bin_path, testcase):
+    """
+    Run unit tests for test cases listen in TEST_CASES
+    """
+
+    # Mock function used for running commands first
+    call_results = [item[2] for item in testcase["run_command.calls"]]
+    mock_run_command = mocker.patch(
+        "ansible.module_utils.basic.AnsibleModule.run_command",
+        side_effect=call_results)
+
+    # Try to run test case
+    with pytest.raises(SystemExit):
+        puppet.main()
+
+    out, err = capfd.readouterr()
+    results = json.loads(out)
+    print("results =\n%s" % results)
+
+    assert mock_run_command.call_count == len(testcase["run_command.calls"])
+    if mock_run_command.call_count:
+        call_args_list = [(item[0][0], item[1]) for item in mock_run_command.call_args_list]
+        expected_call_args_list = [(item[0], item[1]) for item in testcase["run_command.calls"]]
+        print("call args list =\n%s" % call_args_list)
+        print("expected args list =\n%s" % expected_call_args_list)
+        assert call_args_list == expected_call_args_list
+
+    assert results.get("changed", False) == testcase["changed"]
+    if "failed" in testcase:
+        assert results.get("failed", False) == testcase["failed"]
+    if "msg" in testcase:
+        assert results.get("msg", "") == testcase["msg"]