[PR #6989/5988b9ac backport][stable-7] npm: changed to cmdrunner (#7096)

npm: changed to cmdrunner (#6989)

* npm: refactor to use CmdRunner

- initial commit, not working

* better handling of parameter "production"

* add changelog frag

* fixed command call and tests

* removed extraneous commented debug code

(cherry picked from commit 5988b9acea)

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

changelogs/fragments/6989-npm-cmdrunner.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - npm - module now using ``CmdRunner`` to execute external commands (https://github.com/ansible-collections/community.general/pull/6989).

plugins/modules/npm.py

@@ -150,6 +150,7 @@ import re
 
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.common.text.converters import to_native
+from ansible_collections.community.general.plugins.module_utils.cmd_runner import CmdRunner, cmd_runner_fmt
 
 
 class Npm(object):
@@ -172,33 +173,29 @@ class Npm(object):
         else:
             self.executable = [module.get_bin_path('npm', True)]
 
-        if kwargs['version'] and self.state != 'absent':
+        if kwargs['version'] and kwargs['state'] != 'absent':
-            self.name_version = self.name + '@' + str(self.version)
+            self.name_version = self.name + '@' + str(kwargs['version'])
         else:
             self.name_version = self.name
 
+        self.runner = CmdRunner(
+            module,
+            command=self.executable,
+            arg_formats=dict(
+                exec_args=cmd_runner_fmt.as_list(),
+                global_=cmd_runner_fmt.as_bool('--global'),
+                production=cmd_runner_fmt.as_bool('--production'),
+                ignore_scripts=cmd_runner_fmt.as_bool('--ignore-scripts'),
+                unsafe_perm=cmd_runner_fmt.as_bool('--unsafe-perm'),
+                name_version=cmd_runner_fmt.as_list(),
+                registry=cmd_runner_fmt.as_opt_val('--registry'),
+                no_optional=cmd_runner_fmt.as_bool('--no-optional'),
+                no_bin_links=cmd_runner_fmt.as_bool('--no-bin-links'),
+            )
+        )
+
     def _exec(self, args, run_in_check_mode=False, check_rc=True, add_package_name=True):
         if not self.module.check_mode or (self.module.check_mode and run_in_check_mode):
-            cmd = self.executable + args
-
-            if self.glbl:
-                cmd.append('--global')
-            if self.production and ('install' in cmd or 'update' in cmd or 'ci' in cmd):
-                cmd.append('--production')
-            if self.ignore_scripts:
-                cmd.append('--ignore-scripts')
-            if self.unsafe_perm:
-                cmd.append('--unsafe-perm')
-            if self.name_version and add_package_name:
-                cmd.append(self.name_version)
-            if self.registry:
-                cmd.append('--registry')
-                cmd.append(self.registry)
-            if self.no_optional:
-                cmd.append('--no-optional')
-            if self.no_bin_links:
-                cmd.append('--no-bin-links')
-
             # If path is specified, cd into that path and run the command.
             cwd = None
             if self.path:
@@ -208,8 +205,19 @@ class Npm(object):
                     self.module.fail_json(msg="path %s is not a directory" % self.path)
                 cwd = self.path
 
-            rc, out, err = self.module.run_command(cmd, check_rc=check_rc, cwd=cwd)
+            params = dict(self.module.params)
+            params['exec_args'] = args
+            params['global_'] = self.glbl
+            params['production'] = self.production and ('install' in args or 'update' in args or 'ci' in args)
+            params['name_version'] = self.name_version if add_package_name else None
+
+            with self.runner(
+                "exec_args global_ production ignore_scripts unsafe_perm name_version registry no_optional no_bin_links",
+                check_rc=check_rc, cwd=cwd
+            ) as ctx:
+                rc, out, err = ctx.run(**params)
             return out
+
         return ''
 
     def list(self):
@@ -269,12 +277,12 @@ class Npm(object):
 
 def main():
     arg_spec = dict(
-        name=dict(default=None, type='str'),
+        name=dict(type='str'),
-        path=dict(default=None, type='path'),
+        path=dict(type='path'),
-        version=dict(default=None, type='str'),
+        version=dict(type='str'),
         production=dict(default=False, type='bool'),
-        executable=dict(default=None, type='path'),
+        executable=dict(type='path'),
-        registry=dict(default=None, type='str'),
+        registry=dict(type='str'),
         state=dict(default='present', choices=['present', 'absent', 'latest']),
         ignore_scripts=dict(default=False, type='bool'),
         unsafe_perm=dict(default=False, type='bool'),
@@ -293,25 +301,27 @@ def main():
     path = module.params['path']
     version = module.params['version']
     glbl = module.params['global']
-    production = module.params['production']
-    executable = module.params['executable']
-    registry = module.params['registry']
     state = module.params['state']
-    ignore_scripts = module.params['ignore_scripts']
-    unsafe_perm = module.params['unsafe_perm']
-    ci = module.params['ci']
-    no_optional = module.params['no_optional']
-    no_bin_links = module.params['no_bin_links']
 
     if not path and not glbl:
         module.fail_json(msg='path must be specified when not using global')
 
-    npm = Npm(module, name=name, path=path, version=version, glbl=glbl, production=production,
+    npm = Npm(module,
-              executable=executable, registry=registry, ignore_scripts=ignore_scripts,
+              name=name,
-              unsafe_perm=unsafe_perm, state=state, no_optional=no_optional, no_bin_links=no_bin_links)
+              path=path,
+              version=version,
+              glbl=glbl,
+              production=module.params['production'],
+              executable=module.params['executable'],
+              registry=module.params['registry'],
+              ignore_scripts=module.params['ignore_scripts'],
+              unsafe_perm=module.params['unsafe_perm'],
+              state=state,
+              no_optional=module.params['no_optional'],
+              no_bin_links=module.params['no_bin_links'])
 
     changed = False
-    if ci:
+    if module.params['ci']:
         npm.ci_install()
         changed = True
     elif state == 'present':

tests/unit/plugins/modules/test_npm.py

@@ -48,8 +48,8 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None),
+            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
-            call(['/testbin/npm', 'install', '--global', 'coffee-script'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'install', '--global', 'coffee-script'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_present_missing(self):
@@ -67,8 +67,8 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None),
+            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
-            call(['/testbin/npm', 'install', '--global', 'coffee-script'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'install', '--global', 'coffee-script'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_present_version(self):
@@ -87,8 +87,8 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None),
+            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
-            call(['/testbin/npm', 'install', '--global', 'coffee-script@2.5.1'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'install', '--global', 'coffee-script@2.5.1'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_present_version_update(self):
@@ -107,8 +107,8 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None),
+            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
-            call(['/testbin/npm', 'install', '--global', 'coffee-script@2.5.1'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'install', '--global', 'coffee-script@2.5.1'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_present_version_exists(self):
@@ -127,7 +127,7 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertFalse(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None),
+            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_absent(self):
@@ -145,8 +145,8 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None),
+            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
-            call(['/testbin/npm', 'uninstall', '--global', 'coffee-script'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'uninstall', '--global', 'coffee-script'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_absent_version(self):
@@ -165,8 +165,8 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None),
+            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
-            call(['/testbin/npm', 'uninstall', '--global', 'coffee-script'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'uninstall', '--global', 'coffee-script'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_absent_version_different(self):
@@ -185,8 +185,8 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None),
+            call(['/testbin/npm', 'list', '--json', '--long', '--global'], check_rc=False, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
-            call(['/testbin/npm', 'uninstall', '--global', 'coffee-script'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'uninstall', '--global', 'coffee-script'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_present_package_json(self):
@@ -203,7 +203,7 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'install', '--global'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'install', '--global'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_present_package_json_production(self):
@@ -221,7 +221,7 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'install', '--global', '--production'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'install', '--global', '--production'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_present_package_json_ci(self):
@@ -239,7 +239,7 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'ci', '--global'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'ci', '--global'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])
 
     def test_present_package_json_ci_production(self):
@@ -258,5 +258,5 @@ class NPMModuleTestCase(ModuleTestCase):
 
         self.assertTrue(result['changed'])
         self.module_main_command.assert_has_calls([
-            call(['/testbin/npm', 'ci', '--global', '--production'], check_rc=True, cwd=None),
+            call(['/testbin/npm', 'ci', '--global', '--production'], check_rc=True, cwd=None, environ_update={'LANGUAGE': 'C', 'LC_ALL': 'C'}),
         ])