Repository: packit-service/packit (python)

Issue:
run_command - run all the user defined commands in shell
```
[dhodovsk@shiny hello-world]$ ll .print_version.sh 
-rwxrwxr-x. 1 dhodovsk dhodovsk 60 Jan 27 13:47 .print_version.sh
[dhodovsk@shiny hello-world]$ cat .packit.yaml 
---
specfile_path: hello.spec
synced_files:
  - hello.spec
upstream_project_name: hello
downstream_package_name: hello
actions:
  create-archive:
  - python3 setup.py sdist --dist-dir .
  - .print_version.sh
[dhodovsk@shiny hello-world]$ packit srpm
Packit 0.8.1 is being used.
Input is a directory: /home/dhodovsk/repos/github.com/packit-service/hello-world
WARNING  'upstream_project_name' configuration key was renamed to 'upstream_package_name', please update your configuration file
Input directory is an upstream repository.
Using user-defined script for ActionName.create_archive: [['python3', 'setup.py', 'sdist', '--dist-dir', '.'], ['.print_version.sh']]
ERROR    Preparing of the upstream to the SRPM build failed: [Errno 2] No such file or directory: '.print_version.sh': '.print_version.sh'
[dhodovsk@shiny hello-world]$ 
```

It works OK when its called using bash:
```
[dhodovsk@shiny hello-world]$ cat .packit.yaml 
---
specfile_path: hello.spec
synced_files:
  - hello.spec
upstream_project_name: hello
downstream_package_name: hello
actions:
  create-archive:
  - python3 setup.py sdist --dist-dir .
  - bash .print_version.sh
[dhodovsk@shiny hello-world]$ packit srpm
Packit 0.8.1 is being used.
Input is a directory: /home/dhodovsk/repos/github.com/packit-service/hello-world
WARNING  'upstream_project_name' configuration key was renamed to 'upstream_package_name', please update your configuration file
Input directory is an upstream repository.
Using user-defined script for ActionName.create_archive: [['python3', 'setup.py', 'sdist', '--dist-dir', '.'], ['bash', '.print_version.sh']]
Created archive: /home/dhodovsk/repos/github.com/packit-service/hello-world/hello-0.1.0.tar.gz
SRPM is /home/dhodovsk/repos/github.com/packit-service/hello-world/hello-0.7.0.4.gace1326-1.fc30.src.rpm
SRPM: /home/dhodovsk/repos/github.com/packit-service/hello-world/hello-0.7.0.4.gace1326-1.fc30.src.rpm
```


If it is not possible, we should document it clearer in doc.

Pull request:
invoke all commands in shell
Invoke user-defined commands with shell=True, while our commands are running as shell=False

* 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

Fixes #692

Patch:
diff --git a/packit/api.py b/packit/api.py
index 23879a86..d936c640 100644
--- a/packit/api.py
+++ b/packit/api.py
@@ -447,7 +447,6 @@ 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 e619f3c7..6a75971e 100644
--- a/packit/base_git.py
+++ b/packit/base_git.py
@@ -281,7 +281,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)
+                self.command_handler.run_command(command=cmd, env=env, shell=True)
             return False
         logger.debug(f"Running default implementation for {action}.")
         return True
@@ -301,7 +301,9 @@ 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)
+                self.command_handler.run_command(
+                    cmd, return_output=True, env=env, shell=True
+                )
             )
         logger.debug(f"Action command output: {outputs}")
         return outputs
diff --git a/packit/command_handler.py b/packit/command_handler.py
index e77f9f0c..5c213eca 100644
--- a/packit/command_handler.py
+++ b/packit/command_handler.py
@@ -36,6 +36,7 @@ class CommandHandler:
         return_output: bool = True,
         env: Optional[Dict] = None,
         cwd: Union[str, Path] = None,
+        shell: bool = False,
     ):
         """
         exec a command
@@ -44,6 +45,7 @@ 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")
 
@@ -62,6 +64,7 @@ class LocalCommandHandler(CommandHandler):
         return_output: bool = True,
         env: Optional[Dict] = None,
         cwd: Union[str, Path] = None,
+        shell: bool = False,
     ):
         """
         exec a command
@@ -70,12 +73,14 @@ 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,
         )
 
 
@@ -89,6 +94,7 @@ 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.
@@ -97,6 +103,7 @@ 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 0cefe940..0f70263f 100644
--- a/packit/utils.py
+++ b/packit/utils.py
@@ -24,7 +24,6 @@ import json
 import logging
 import os
 import re
-import shlex
 import subprocess
 import sys
 import tempfile
@@ -93,6 +92,7 @@ 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,16 +105,19 @@ 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 not isinstance(cmd, list):
-        cmd = shlex.split(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)
 
-    escaped_command = " ".join(cmd)
+    cmd_str = cmd
+    if isinstance(cmd, list):
+        cmd_str = " ".join(cmd)
 
-    logger.debug(f"Command: {escaped_command}")
+    logger.debug(f"Command: {cmd_str}")
 
     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
@@ -124,33 +127,38 @@ 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
-    shell = subprocess.Popen(
+    popen_proc = subprocess.Popen(
         cmd,
         stdout=subprocess.PIPE,
         stderr=subprocess.PIPE,
-        shell=False,
+        # 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,
         cwd=cwd,
         env=cmd_env,
     )
 
     stdout = StreamLogger(
-        shell.stdout,
+        popen_proc.stdout,
         log_level=logging.DEBUG if not print_live else logging.INFO,
         decode=decode,
     )
     stderr = StreamLogger(
-        shell.stderr,
+        popen_proc.stderr,
         log_level=logging.DEBUG if not print_live else logging.INFO,
         decode=decode,
     )
 
     stdout.start()
     stderr.start()
-    shell.wait()
+    popen_proc.wait()
     stdout.join()
     stderr.join()
 
-    if shell.returncode != 0:
+    if popen_proc.returncode != 0:
+        error_message = error_message or f"Command {cmd_str!r} failed."
         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 422600fe..0b08dd50 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/
-    - bash -c "ls -1t ./fedora/*.tar.gz | head -n 1"
+    - 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 9c2e2bf4..f4a55129 100644
--- a/tests/unit/test_base_git.py
+++ b/tests/unit/test_base_git.py
@@ -122,7 +122,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
+        command=["command", "--a"], env=None, shell=True
     ).and_return("command --a").once()
 
     packit_repository_base.local_project = flexmock(working_dir="my/working/dir")
@@ -158,7 +158,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
+        command=["command", "--a"], env=None, shell=True
     ).and_return("command --a").once()
 
     packit_repository_base.local_project = flexmock(working_dir="my/working/dir")
@@ -185,7 +185,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
+        command=["command", "-a"], env=None, shell=True
     ).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 1961a5f4..4d58f5d5 100644
--- a/tests/unit/test_utils.py
+++ b/tests/unit/test_utils.py
@@ -80,7 +80,12 @@ def test_remote_to_https(inp, ok):
 
 
 def test_run_command_w_env():
-    run_command(["bash", "-c", "env | grep PATH"], env={"X": "Y"})
+    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)
 
 
 def test_get_packit_version_not_installed():