Skip to content

Commit

Permalink
Changed error handling to warn log for pipe with shell commands
Browse files Browse the repository at this point in the history
Signed-off-by: pryce-turner <pryce.turner@gmail.com>
  • Loading branch information
pryce-turner committed Jun 17, 2024
1 parent 9132cf3 commit e2f41dd
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 14 deletions.
11 changes: 7 additions & 4 deletions flytekit/extras/tasks/shell.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,14 @@ def subproc_execute(command: typing.Union[List[str], str], **kwargs) -> ProcessR
try:
# Execute the command and capture stdout and stderr
result = subprocess.run(command, **kwargs)
print(result.check_returncode())

assert result.stderr == ""
if "|" in command and kwargs.get("shell"):
logger.warning(
"""Found a pipe in the command and shell=True.
This can lead to silent failures if subsequent commands
succeed despite previous failures."""
)

# Access the stdout and stderr output
return ProcessResult(result.returncode, result.stdout, result.stderr)
Expand All @@ -95,9 +101,6 @@ def subproc_execute(command: typing.Union[List[str], str], **kwargs) -> ProcessR
custom dependencies?\n{e}"""
)

except AssertionError:
raise Exception(f"Command: {command}\nexperienced a silent failure, likely due to shell=True:\n{result.stderr}")


def _dummy_task_func():
"""
Expand Down
10 changes: 0 additions & 10 deletions tests/flytekit/unit/extras/tasks/test_shell.py
Original file line number Diff line number Diff line change
Expand Up @@ -384,13 +384,3 @@ def test_subproc_execute_error():
with pytest.raises(Exception) as e:
subproc_execute(cmd)
assert "Failed with return code" in str(e.value)


def test_subproc_execute_shell_error(tmp_path):
# This is a corner case I ran into that really shouldn't
# ever happen. The assert catches anything in stderr despite
# a 0 exit.
cmd = " ".join(["bcftools", "isec", "|", "gzip", "-c", ">", f"{tmp_path.joinpath('out')}"])
with pytest.raises(Exception) as e:
subproc_execute(cmd, shell=True)
assert "silent failure" in str(e.value)

0 comments on commit e2f41dd

Please sign in to comment.