Repository: packit-service/packit
Pull request #913 (2020-07-27 11:29:24)

Revert "invoke all commands in shell"
This reverts commit c87aa49fdb53e258c4c33647e1bab9551ab657ce.

also:
```
Revert "[.packit.yaml] remove 'sh -c' from create-archive"

This reverts commit b2f9d54a057b0ead84824585ea3fb619723872e4.
```

Sorry about the problems, this seems to be much harder to implement than I
originally thought.


Patch:
diff --git a/.packit.yaml b/.packit.yaml
index a6cb8845..a2f67c6a 100644
--- a/.packit.yaml
+++ b/.packit.yaml
@@ -9,7 +9,7 @@ upstream_project_url: https://github.com/packit-service/packit
 actions:
   create-archive:
     - "python3 setup.py sdist --dist-dir ."
-    - "echo packitos-$(python3 setup.py --version).tar.gz"
+    - "sh -c 'echo packitos-$(python3 setup.py --version).tar.gz'"
   get-current-version:
     - "python3 setup.py --version"
 
diff --git a/packit/api.py b/packit/api.py
index 7507e541..acf920b7 100644
--- a/packit/api.py
+++ b/packit/api.py
@@ -464,6 +464,7 @@ class PackitAPI:
         :return: a path to the srpm
         """
         self.up.run_action(actions=ActionName.post_upstream_clone)
+
         try:
             self.up.prepare_upstream_for_srpm_creation(upstream_ref=upstream_ref)
         except Exception as ex:
diff --git a/packit/base_git.py b/packit/base_git.py
index 978c8274..9d2da9cf 100644
--- a/packit/base_git.py
+++ b/packit/base_git.py
@@ -282,7 +282,7 @@ class PackitRepositoryBase:
             commands_to_run = self.get_commands_for_actions(action)
             logger.info(f"Using user-defined script for {action}: {commands_to_run}")
             for cmd in commands_to_run:
-                self.command_handler.run_command(command=cmd, env=env, shell=True)
+                self.command_handler.run_command(command=cmd, env=env)
             return False
         logger.debug(f"Running default implementation for {action}.")
         return True
@@ -302,9 +302,7 @@ class PackitRepositoryBase:
         logger.info(f"Using user-defined script for {action}: {commands_to_run}")
         for cmd in commands_to_run:
             outputs.append(
-                self.command_handler.run_command(
-                    cmd, return_output=True, env=env, shell=True
-                )
+                self.command_handler.run_command(cmd, return_output=True, env=env)
             )
         logger.debug(f"Action command output: {outputs}")
         return outputs
diff --git a/packit/command_handler.py b/packit/command_handler.py
index ede7686d..e8bbc491 100644
--- a/packit/command_handler.py
+++ b/packit/command_handler.py
@@ -58,7 +58,6 @@ class CommandHandler:
         return_output: bool = True,
         env: Optional[Dict] = None,
         cwd: Union[str, Path] = None,
-        shell: bool = False,
     ):
         """
         exec a command
@@ -67,7 +66,6 @@ class CommandHandler:
         :param return_output: return output from this method if True
         :param env: dict with env vars to set for the command
         :param cwd: working directory to run command in
-        :param shell: invoke in shell? (aka bash -c $cmd)
         """
         raise NotImplementedError("This should be implemented")
 
@@ -86,7 +84,6 @@ class LocalCommandHandler(CommandHandler):
         return_output: bool = True,
         env: Optional[Dict] = None,
         cwd: Union[str, Path] = None,
-        shell: bool = False,
     ):
         """
         exec a command
@@ -95,14 +92,12 @@ class LocalCommandHandler(CommandHandler):
         :param return_output: return output from this method if True
         :param env: dict with env vars to set for the command
         :param cwd: working directory to run command in
-        :param shell: invoke in shell? (aka bash -c $cmd)
         """
         return utils.run_command(
             cmd=command,
             cwd=cwd or self.local_project.working_dir,
             output=return_output,
             env=env,
-            shell=shell,
         )
 
 
@@ -116,7 +111,6 @@ class SandcastleCommandHandler(CommandHandler):
         return_output: bool = True,
         env: Optional[Dict] = None,
         cwd: Union[str, Path] = None,
-        shell: bool = True,
     ):
         """
         Executes command in a sandbox provided by sandcastle.
@@ -125,7 +119,6 @@ class SandcastleCommandHandler(CommandHandler):
         :param return_output: return output from this method if True
         :param env: dict with env vars to set for the command
         :param cwd: working directory to run command in
-        :param shell: sandcastle invokes everything in shell, this is no-op
         """
         # we import here so that packit does not depend on sandcastle (and thus python-kube)
         from sandcastle.api import Sandcastle, MappedDir
diff --git a/packit/utils.py b/packit/utils.py
index 3b5084d4..338fa90b 100644
--- a/packit/utils.py
+++ b/packit/utils.py
@@ -24,6 +24,7 @@ import json
 import logging
 import os
 import re
+import shlex
 import subprocess
 import sys
 import tempfile
@@ -92,7 +93,6 @@ def run_command(
     env: Optional[Dict] = None,
     decode: bool = True,
     print_live: bool = False,
-    shell: bool = False,
 ):
     """
     run provided command in a new subprocess
@@ -105,19 +105,16 @@ def run_command(
     :param env: set these env vars in the subprocess
     :param decode: decode stdout from utf8 to string
     :param print_live: print output from the command realtime to INFO log
-    :param shell: invoke in shell? (aka bash -c $cmd)
     """
-    if shell and isinstance(cmd, list):  # we are running a user defined action
-        # Popen( accepts cmd as a string, not list when shell=True - freaky
-        cmd = " ".join(cmd)
+    if not isinstance(cmd, list):
+        cmd = shlex.split(cmd)
 
-    cmd_str = cmd
-    if isinstance(cmd, list):
-        cmd_str = " ".join(cmd)
+    escaped_command = " ".join(cmd)
 
-    logger.debug(f"Command: {cmd_str}")
+    logger.debug(f"Command: {escaped_command}")
 
     cwd = str(cwd) if cwd else str(Path.cwd())
+    error_message = error_message or f"Command {escaped_command!r} failed."
 
     # we need to pass complete env to Popen, otherwise we lose everything from os.environ
     cmd_env = os.environ
@@ -127,38 +124,33 @@ def run_command(
     # we can't use universal newlines here b/c the output from the command can be encoded
     # in something alien and we would "can't decode this using utf-8" errors
     # https://github.com/packit-service/systemd-rhel8-flock/pull/9#issuecomment-550184016
-    popen_proc = subprocess.Popen(
+    shell = subprocess.Popen(
         cmd,
         stdout=subprocess.PIPE,
         stderr=subprocess.PIPE,
-        # we know what we are doing here:
-        #  * we already run commands defined by users - it's gonna be more convenient for them
-        #    to do `ls *.tar.gz` instead of `bash -c "ls *.tar.gz"`
-        #  * and we have the sandbox to minimize the damage
-        shell=shell,
+        shell=False,
         cwd=cwd,
         env=cmd_env,
     )
 
     stdout = StreamLogger(
-        popen_proc.stdout,
+        shell.stdout,
         log_level=logging.DEBUG if not print_live else logging.INFO,
         decode=decode,
     )
     stderr = StreamLogger(
-        popen_proc.stderr,
+        shell.stderr,
         log_level=logging.DEBUG if not print_live else logging.INFO,
         decode=decode,
     )
 
     stdout.start()
     stderr.start()
-    popen_proc.wait()
+    shell.wait()
     stdout.join()
     stderr.join()
 
-    if popen_proc.returncode != 0:
-        error_message = error_message or f"Command {cmd_str!r} failed."
+    if shell.returncode != 0:
         logger.error(f"{error_message}")
         if fail:
             stderr_output = (


Test patch:
diff --git a/tests/data/dg-ogr/.packit.yaml b/tests/data/dg-ogr/.packit.yaml
index 0b08dd50..422600fe 100644
--- a/tests/data/dg-ogr/.packit.yaml
+++ b/tests/data/dg-ogr/.packit.yaml
@@ -12,7 +12,7 @@ actions:
   #   Make sure you're either building from a fully intact git repository or PyPI tarballs.
   create-archive:
     - python3 setup.py sdist --dist-dir ./fedora/
-    - ls -1t ./fedora/*.tar.gz | head -n 1
+    - bash -c "ls -1t ./fedora/*.tar.gz | head -n 1"
   get-current-version: python3 setup.py --version
 jobs:
   - job: sync_from_downstream
diff --git a/tests/unit/test_base_git.py b/tests/unit/test_base_git.py
index d4928b4e..e29a1335 100644
--- a/tests/unit/test_base_git.py
+++ b/tests/unit/test_base_git.py
@@ -144,7 +144,7 @@ def test_with_action_defined(packit_repository_base):
 
 def test_with_action_working_dir(packit_repository_base):
     flexmock(LocalCommandHandler).should_receive("run_command").with_args(
-        command=["command", "--a"], env=None, shell=True
+        command=["command", "--a"], env=None
     ).and_return("command --a").once()
 
     packit_repository_base.local_project = flexmock(working_dir="my/working/dir")
@@ -180,7 +180,7 @@ def test_run_action_not_defined(packit_repository_base):
 
 def test_run_action_defined(packit_repository_base):
     flexmock(LocalCommandHandler).should_receive("run_command").with_args(
-        command=["command", "--a"], env=None, shell=True
+        command=["command", "--a"], env=None
     ).and_return("command --a").once()
 
     packit_repository_base.local_project = flexmock(working_dir="my/working/dir")
@@ -207,7 +207,7 @@ def test_run_action_in_sandcastle(packit_repository_base_with_sandcastle_object)
 
     flexmock(Sandcastle).should_receive("get_api_client").and_return(None)
     flexmock(SandcastleCommandHandler).should_receive("run_command").with_args(
-        command=["command", "-a"], env=None, shell=True
+        command=["command", "-a"], env=None
     ).and_return(None).once()
     packit_repository_base_with_sandcastle_object.config.actions_handler = "sandcastle"
     packit_repository_base_with_sandcastle_object.run_action(
diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py
index ab273a7d..3a0ea847 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -80,12 +80,7 @@ def test_remote_to_https(inp, ok):
 
 
 def test_run_command_w_env():
-    run_command("env | grep PATH", env={"X": "Y"}, shell=True)
-
-
-def test_run_command_shell():
-    # make sure the `ls` succeeds - shell expands the star, not ls
-    run_command(["ls", "/etc/*wd"], shell=True)
+    run_command(["bash", "-c", "env | grep PATH"], env={"X": "Y"})
 
 
 def test_get_packit_version_not_installed():