Convert some run_command() string args to lists (#8264)

* Convert some run_command() string args to lists. * Change run_command with pipe and shell to Python code. * Add changelog. * Simplify syntax. Co-authored-by: Alexei Znamensky <[email protected]> --------- Co-authored-by: Alexei Znamensky <[email protected]>
ansible-collections · Apr 29, 2024 · 70adba8 · 70adba8
1 parent b48293c
commit 70adba8
Show file tree

Hide file tree

Showing 14 changed files with 144 additions and 144 deletions.
diff --git a/changelogs/fragments/8264-run_command.yml b/changelogs/fragments/8264-run_command.yml
@@ -0,0 +1,14 @@
+minor_changes:
+  - "aix_lvol - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "apt_rpm - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "btrfs_subvolume - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "installp - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "lvg - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "lvol - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "macports - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "parted - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "pkgin - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "portinstall - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "slackpkg - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "svr4pkg - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
+  - "swdepot - refactor module to pass list of arguments to ``module.run_command()`` instead of relying on interpretation by a shell (https://github.com/ansible-collections/community.general/pull/8264)."
diff --git a/plugins/modules/aix_lvol.py b/plugins/modules/aix_lvol.py
@@ -240,25 +240,23 @@ def main():
     state = module.params['state']
     pvs = module.params['pvs']
 
-    pv_list = ' '.join(pvs)
-
     if policy == 'maximum':
         lv_policy = 'x'
     else:
         lv_policy = 'm'
 
     # Add echo command when running in check-mode
     if module.check_mode:
-        test_opt = 'echo '
+        test_opt = [module.get_bin_path("echo", required=True)]
     else:
-        test_opt = ''
+        test_opt = []
 
     # check if system commands are available
     lsvg_cmd = module.get_bin_path("lsvg", required=True)
     lslv_cmd = module.get_bin_path("lslv", required=True)
 
     # Get information on volume group requested
-    rc, vg_info, err = module.run_command("%s %s" % (lsvg_cmd, vg))
+    rc, vg_info, err = module.run_command([lsvg_cmd, vg])
 
     if rc != 0:
         if state == 'absent':
@@ -273,8 +271,7 @@ def main():
         lv_size = round_ppsize(convert_size(module, size), base=this_vg['pp_size'])
 
     # Get information on logical volume requested
-    rc, lv_info, err = module.run_command(
-        "%s %s" % (lslv_cmd, lv))
+    rc, lv_info, err = module.run_command([lslv_cmd, lv])
 
     if rc != 0:
         if state == 'absent':
@@ -296,7 +293,7 @@ def main():
             # create LV
             mklv_cmd = module.get_bin_path("mklv", required=True)
 
-            cmd = "%s %s -t %s -y %s -c %s  -e %s %s %s %sM %s" % (test_opt, mklv_cmd, lv_type, lv, copies, lv_policy, opts, vg, lv_size, pv_list)
+            cmd = test_opt + [mklv_cmd, "-t", lv_type, "-y", lv, "-c", copies, "-e", lv_policy, opts, vg, "%sM" % (lv_size, )] + pvs
             rc, out, err = module.run_command(cmd)
             if rc == 0:
                 module.exit_json(changed=True, msg="Logical volume %s created." % lv)
@@ -306,7 +303,7 @@ def main():
         if state == 'absent':
             # remove LV
             rmlv_cmd = module.get_bin_path("rmlv", required=True)
-            rc, out, err = module.run_command("%s %s -f %s" % (test_opt, rmlv_cmd, this_lv['name']))
+            rc, out, err = module.run_command(test_opt + [rmlv_cmd, "-f", this_lv['name']])
             if rc == 0:
                 module.exit_json(changed=True, msg="Logical volume %s deleted." % lv)
             else:
@@ -315,7 +312,7 @@ def main():
             if this_lv['policy'] != policy:
                 # change lv allocation policy
                 chlv_cmd = module.get_bin_path("chlv", required=True)
-                rc, out, err = module.run_command("%s %s -e %s %s" % (test_opt, chlv_cmd, lv_policy, this_lv['name']))
+                rc, out, err = module.run_command(test_opt + [chlv_cmd, "-e", lv_policy, this_lv['name']])
                 if rc == 0:
                     module.exit_json(changed=True, msg="Logical volume %s policy changed: %s." % (lv, policy))
                 else:
@@ -331,7 +328,7 @@ def main():
             # resize LV based on absolute values
             if int(lv_size) > this_lv['size']:
                 extendlv_cmd = module.get_bin_path("extendlv", required=True)
-                cmd = "%s %s %s %sM" % (test_opt, extendlv_cmd, lv, lv_size - this_lv['size'])
+                cmd = test_opt + [extendlv_cmd, lv, "%sM" % (lv_size - this_lv['size'], )]
                 rc, out, err = module.run_command(cmd)
                 if rc == 0:
                     module.exit_json(changed=True, msg="Logical volume %s size extended to %sMB." % (lv, lv_size))

diff --git a/plugins/modules/apt_rpm.py b/plugins/modules/apt_rpm.py
@@ -170,7 +170,7 @@ def local_rpm_package_name(path):
 def query_package(module, name):
     # rpm -q returns 0 if the package is installed,
     # 1 if it is not installed
-    rc, out, err = module.run_command("%s -q %s" % (RPM_PATH, name))
+    rc, out, err = module.run_command([RPM_PATH, "-q", name])
     if rc == 0:
         return True
     else:
@@ -203,7 +203,7 @@ def query_package_provides(module, name, allow_upgrade=False):
 
         name = local_rpm_package_name(name)
 
-    rc, out, err = module.run_command("%s -q --provides %s" % (RPM_PATH, name))
+    rc, out, err = module.run_command([RPM_PATH, "-q", "--provides", name])
     if rc == 0:
         if not allow_upgrade:
             return True
@@ -253,7 +253,7 @@ def remove_packages(module, packages):
         if not query_package(module, package):
             continue
 
-        rc, out, err = module.run_command("%s -y remove %s" % (APT_PATH, package), environ_update={"LANG": "C"})
+        rc, out, err = module.run_command([APT_PATH, "-y", "remove", package], environ_update={"LANG": "C"})
 
         if rc != 0:
             module.fail_json(msg="failed to remove %s: %s" % (package, err))
@@ -271,14 +271,14 @@ def install_packages(module, pkgspec, allow_upgrade=False):
     if pkgspec is None:
         return (False, "Empty package list")
 
-    packages = ""
+    packages = []
     for package in pkgspec:
         if not query_package_provides(module, package, allow_upgrade=allow_upgrade):
-            packages += "'%s' " % package
+            packages.append(package)
 
-    if len(packages) != 0:
-
-        rc, out, err = module.run_command("%s -y install %s" % (APT_PATH, packages), environ_update={"LANG": "C"})
+    if packages:
+        command = [APT_PATH, "-y", "install"] + packages
+        rc, out, err = module.run_command(command, environ_update={"LANG": "C"})
 
         installed = True
         for package in pkgspec:
@@ -287,7 +287,7 @@ def install_packages(module, pkgspec, allow_upgrade=False):
 
         # apt-rpm always have 0 for exit code if --force is used
         if rc or not installed:
-            module.fail_json(msg="'apt-get -y install %s' failed: %s" % (packages, err))
+            module.fail_json(msg="'%s' failed: %s" % (" ".join(command), err))
         else:
             return (True, "%s present(s)" % packages)
     else:

diff --git a/plugins/modules/btrfs_subvolume.py b/plugins/modules/btrfs_subvolume.py
@@ -572,10 +572,7 @@ def __mount_subvolume_id_to_tempdir(self, filesystem, subvolid):
         self.__temporary_mounts[cache_key] = mountpoint
 
         mount = self.module.get_bin_path("mount", required=True)
-        command = "%s -o noatime,subvolid=%d %s %s " % (mount,
-                                                        subvolid,
-                                                        device,
-                                                        mountpoint)
+        command = [mount, "-o", "noatime,subvolid=%d" % subvolid, device, mountpoint]
         result = self.module.run_command(command, check_rc=True)
 
         return mountpoint
@@ -586,10 +583,10 @@ def __cleanup_mounts(self):
 
     def __cleanup_mount(self, mountpoint):
         umount = self.module.get_bin_path("umount", required=True)
-        result = self.module.run_command("%s %s" % (umount, mountpoint))
+        result = self.module.run_command([umount, mountpoint])
         if result[0] == 0:
             rmdir = self.module.get_bin_path("rmdir", required=True)
-            self.module.run_command("%s %s" % (rmdir, mountpoint))
+            self.module.run_command([rmdir, mountpoint])
 
     # Format and return results
     def get_results(self):

diff --git a/plugins/modules/installp.py b/plugins/modules/installp.py
@@ -106,7 +106,7 @@ def _check_new_pkg(module, package, repository_path):
 
     if os.path.isdir(repository_path):
         installp_cmd = module.get_bin_path('installp', True)
-        rc, package_result, err = module.run_command("%s -l -MR -d %s" % (installp_cmd, repository_path))
+        rc, package_result, err = module.run_command([installp_cmd, "-l", "-MR", "-d", repository_path])
         if rc != 0:
             module.fail_json(msg="Failed to run installp.", rc=rc, err=err)
 
@@ -142,7 +142,7 @@ def _check_installed_pkg(module, package, repository_path):
     """
 
     lslpp_cmd = module.get_bin_path('lslpp', True)
-    rc, lslpp_result, err = module.run_command("%s -lcq %s*" % (lslpp_cmd, package))
+    rc, lslpp_result, err = module.run_command([lslpp_cmd, "-lcq", "%s*" % (package, )])
 
     if rc == 1:
         package_state = ' '.join(err.split()[-2:])
@@ -173,7 +173,7 @@ def remove(module, installp_cmd, packages):
 
         if pkg_check:
             if not module.check_mode:
-                rc, remove_out, err = module.run_command("%s -u %s" % (installp_cmd, package))
+                rc, remove_out, err = module.run_command([installp_cmd, "-u", package])
                 if rc != 0:
                     module.fail_json(msg="Failed to run installp.", rc=rc, err=err)
             remove_count += 1
@@ -202,8 +202,8 @@ def install(module, installp_cmd, packages, repository_path, accept_license):
     already_installed_pkgs = {}
 
     accept_license_param = {
-        True: '-Y',
-        False: '',
+        True: ['-Y'],
+        False: [],
     }
 
     # Validate if package exists on repository path.
@@ -230,7 +230,8 @@ def install(module, installp_cmd, packages, repository_path, accept_license):
 
             else:
                 if not module.check_mode:
-                    rc, out, err = module.run_command("%s -a %s -X -d %s %s" % (installp_cmd, accept_license_param[accept_license], repository_path, package))
+                    rc, out, err = module.run_command(
+                        [installp_cmd, "-a"] + accept_license_param[accept_license] + ["-X", "-d", repository_path, package])
                     if rc != 0:
                         module.fail_json(msg="Failed to run installp", rc=rc, err=err)
                 installed_pkgs.append(package)

diff --git a/plugins/modules/lvg.py b/plugins/modules/lvg.py
@@ -179,7 +179,7 @@ def parse_vgs(data):
 def find_mapper_device_name(module, dm_device):
     dmsetup_cmd = module.get_bin_path('dmsetup', True)
     mapper_prefix = '/dev/mapper/'
-    rc, dm_name, err = module.run_command("%s info -C --noheadings -o name %s" % (dmsetup_cmd, dm_device))
+    rc, dm_name, err = module.run_command([dmsetup_cmd, "info", "-C", "--noheadings", "-o", "name", dm_device])
     if rc != 0:
         module.fail_json(msg="Failed executing dmsetup command.", rc=rc, err=err)
     mapper_device = mapper_prefix + dm_name.rstrip()
@@ -204,7 +204,7 @@ def find_vg(module, vg):
     if not vg:
         return None
     vgs_cmd = module.get_bin_path('vgs', True)
-    dummy, current_vgs, dummy = module.run_command("%s --noheadings -o vg_name,pv_count,lv_count --separator ';'" % vgs_cmd, check_rc=True)
+    dummy, current_vgs, dummy = module.run_command([vgs_cmd, "--noheadings", "-o", "vg_name,pv_count,lv_count", "--separator", ";"], check_rc=True)
 
     vgs = parse_vgs(current_vgs)
 
@@ -431,10 +431,10 @@ def main():
                 for x in itertools.chain(dev_list, module.params['pvs'])
             )
             pvs_filter_vg_name = 'vg_name = {0}'.format(vg)
-            pvs_filter = "--select '{0} || {1}' ".format(pvs_filter_pv_name, pvs_filter_vg_name)
+            pvs_filter = ["--select", "{0} || {1}".format(pvs_filter_pv_name, pvs_filter_vg_name)]
         else:
-            pvs_filter = ''
-        rc, current_pvs, err = module.run_command("%s --noheadings -o pv_name,vg_name --separator ';' %s" % (pvs_cmd, pvs_filter))
+            pvs_filter = []
+        rc, current_pvs, err = module.run_command([pvs_cmd, "--noheadings", "-o", "pv_name,vg_name", "--separator", ";"] + pvs_filter)
         if rc != 0:
             module.fail_json(msg="Failed executing pvs command.", rc=rc, err=err)
 
@@ -473,7 +473,7 @@ def main():
                 if this_vg['lv_count'] == 0 or force:
                     # remove VG
                     vgremove_cmd = module.get_bin_path('vgremove', True)
-                    rc, dummy, err = module.run_command("%s --force %s" % (vgremove_cmd, vg))
+                    rc, dummy, err = module.run_command([vgremove_cmd, "--force", vg])
                     if rc == 0:
                         module.exit_json(changed=True)
                     else:
@@ -509,7 +509,6 @@ def main():
                     changed = True
                 else:
                     if devs_to_add:
-                        devs_to_add_string = ' '.join(devs_to_add)
                         # create PV
                         pvcreate_cmd = module.get_bin_path('pvcreate', True)
                         for current_dev in devs_to_add:
@@ -520,21 +519,20 @@ def main():
                                 module.fail_json(msg="Creating physical volume '%s' failed" % current_dev, rc=rc, err=err)
                         # add PV to our VG
                         vgextend_cmd = module.get_bin_path('vgextend', True)
-                        rc, dummy, err = module.run_command("%s %s %s" % (vgextend_cmd, vg, devs_to_add_string))
+                        rc, dummy, err = module.run_command([vgextend_cmd, vg] + devs_to_add)
                         if rc == 0:
                             changed = True
                         else:
-                            module.fail_json(msg="Unable to extend %s by %s." % (vg, devs_to_add_string), rc=rc, err=err)
+                            module.fail_json(msg="Unable to extend %s by %s." % (vg, ' '.join(devs_to_add)), rc=rc, err=err)
 
                     # remove some PV from our VG
                     if devs_to_remove:
-                        devs_to_remove_string = ' '.join(devs_to_remove)
                         vgreduce_cmd = module.get_bin_path('vgreduce', True)
-                        rc, dummy, err = module.run_command("%s --force %s %s" % (vgreduce_cmd, vg, devs_to_remove_string))
+                        rc, dummy, err = module.run_command([vgreduce_cmd, "--force", vg] + devs_to_remove)
                         if rc == 0:
                             changed = True
                         else:
-                            module.fail_json(msg="Unable to reduce %s by %s." % (vg, devs_to_remove_string), rc=rc, err=err)
+                            module.fail_json(msg="Unable to reduce %s by %s." % (vg, ' '.join(devs_to_remove)), rc=rc, err=err)
 
     module.exit_json(changed=changed)