Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions cg/services/analysis_starter/submitters/subprocess/submitter.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,21 @@ def get_workflow_version(case_config: SubprocessCaseConfig) -> str:
result: subprocess.CompletedProcess = subprocess.run(
args=command,
shell=True,
check=False,
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)

stdout: str = result.stdout.decode("utf-8").rstrip()
return stdout.split()[-1]
except Exception:
LOG.warning(f"Could not retrieve {case_config.workflow} workflow version!")

except Exception as e:

stderr = ""
if isinstance(e, subprocess.CalledProcessError):
stderr = e.stderr.decode("utf-8").rstrip()

LOG.warning(
f"Could not retrieve {case_config.workflow} workflow version: {e} : {stderr}"
)
Comment on lines +49 to +58

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we remove the spaces? not a fan of sparse code

Suggested change
except Exception as e:
stderr = ""
if isinstance(e, subprocess.CalledProcessError):
stderr = e.stderr.decode("utf-8").rstrip()
LOG.warning(
f"Could not retrieve {case_config.workflow} workflow version: {e} : {stderr}"
)
except Exception as e:
stderr = ""
if isinstance(e, subprocess.CalledProcessError):
stderr = e.stderr.decode("utf-8").rstrip()
LOG.warning(
f"Could not retrieve {case_config.workflow} workflow version: {e} : {stderr}"
)

return "0.0.0"
124 changes: 120 additions & 4 deletions tests/services/analysis_starter/test_subprocess_submitter.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import pytest
from pytest_mock import MockerFixture

from cg.constants import Workflow
from cg.constants import EXIT_FAIL, EXIT_SUCCESS, Workflow
from cg.constants.priority import SlurmQos
from cg.services.analysis_starter.configurator.models.balsamic import BalsamicCaseConfig
from cg.services.analysis_starter.configurator.models.microsalt import MicrosaltCaseConfig
Expand Down Expand Up @@ -72,7 +72,7 @@
)


def test_microsalt_get_workflow_version(mocker: MockerFixture):
def test_get_workflow_version_returns_version(mocker: MockerFixture):
# GIVEN a SubprocessSubmitter
subprocess_submitter = SubprocessSubmitter()

Expand All @@ -91,7 +91,9 @@
subprocess,
"run",
return_value=create_autospec(
subprocess.CompletedProcess, stdout=b"microSALT, version 4.2.2 \n"
subprocess.CompletedProcess,
stdout=b"microSALT, version 4.2.2 \n",
returncode=EXIT_SUCCESS,
),
)

Expand All @@ -99,13 +101,127 @@
workflow_version = subprocess_submitter.get_workflow_version(case_config)

# THEN the subprocess should have been called with the expected call
mock_run.assert_called_once_with(
args=f"{case_config.conda_binary} run {case_config.binary} --version",
shell=True,
check=False,
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)

Check warning on line 110 in tests/services/analysis_starter/test_subprocess_submitter.py

View check run for this annotation

codefactor.io / CodeFactor

tests/services/analysis_starter/test_subprocess_submitter.py#L104-L110

Function call with shell=True parameter identified, possible security issue. (B604)

# THEN the workflow version should have been returned
assert workflow_version == "4.2.2"


def test_get_workflow_version_raises_when_command_fails(mocker: MockerFixture):
# GIVEN a SubprocessSubmitter
subprocess_submitter = SubprocessSubmitter()

# GIVEN a microSALT case config
case_config = MicrosaltCaseConfig(
case_id="case_id",
binary="binary",
conda_binary="conda_binary",
config_file="microSALT.yml",
environment="S_microSALT",
fastq_directory="fastq/dir",
)

# GIVEN that running a subprocess fails and CalledProcessError is raised
mock_run = mocker.patch.object(
subprocess,
"run",
side_effect=subprocess.CalledProcessError(
returncode=EXIT_FAIL, stderr=b"some stderr", cmd="Some command"
),
)

# WHEN getting the workflow version
workflow_version = subprocess_submitter.get_workflow_version(case_config)

# THEN the subprocess should have been called with the expected call
mock_run.assert_called_once_with(
args=f"{case_config.conda_binary} run {case_config.binary} --version",
shell=True,
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)

Check warning on line 149 in tests/services/analysis_starter/test_subprocess_submitter.py

View check run for this annotation

codefactor.io / CodeFactor

tests/services/analysis_starter/test_subprocess_submitter.py#L143-L149

Function call with shell=True parameter identified, possible security issue. (B604)

# THEN the fallback workflow version should have been returned
assert workflow_version == "0.0.0"


def test_get_workflow_version_returns_fallback_when_stdout_is_empty(mocker: MockerFixture):
# GIVEN a SubprocessSubmitter
subprocess_submitter = SubprocessSubmitter()

# GIVEN a microSALT case config
case_config = MicrosaltCaseConfig(
case_id="case_id",
binary="binary",
conda_binary="conda_binary",
config_file="microSALT.yml",
environment="S_microSALT",
fastq_directory="fastq/dir",
)

# GIVEN that stdout is none and IndexError is raised
mock_run = mocker.patch.object(
subprocess,
"run",
return_value=create_autospec(subprocess.CompletedProcess, stdout=b""),
side_effect=IndexError(),
)

# WHEN getting the workflow version
workflow_version = subprocess_submitter.get_workflow_version(case_config)

# THEN the subprocess should have been called with the expected call
mock_run.assert_called_once_with(
args=f"{case_config.conda_binary} run {case_config.binary} --version",
shell=True,
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)

Check warning on line 187 in tests/services/analysis_starter/test_subprocess_submitter.py

View check run for this annotation

codefactor.io / CodeFactor

tests/services/analysis_starter/test_subprocess_submitter.py#L181-L187

Function call with shell=True parameter identified, possible security issue. (B604)

# THEN the fallback workflow version should have been returned
assert workflow_version == "0.0.0"


def test_get_workflow_version_returns_fallback_when_binary_not_found(mocker: MockerFixture):
# GIVEN a SubprocessSubmitter
subprocess_submitter = SubprocessSubmitter()

# GIVEN a microSALT case config
case_config = MicrosaltCaseConfig(
case_id="case_id",
binary="binary",
conda_binary="conda_binary",
config_file="microSALT.yml",
environment="S_microSALT",
fastq_directory="fastq/dir",
)

# GIVEN that running a subprocess raises an OSError
mock_run = mocker.patch.object(
subprocess,
"run",
side_effect=OSError("Binary not found"),
)

# WHEN getting the workflow version
workflow_version = subprocess_submitter.get_workflow_version(case_config)

# THEN the subprocess should have been called with the expected call
mock_run.assert_called_once_with(
args=f"{case_config.conda_binary} run {case_config.binary} --version",
shell=True,
check=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)

Check warning on line 224 in tests/services/analysis_starter/test_subprocess_submitter.py

View check run for this annotation

codefactor.io / CodeFactor

tests/services/analysis_starter/test_subprocess_submitter.py#L218-L224

Function call with shell=True parameter identified, possible security issue. (B604)

# THEN the fallback workflow version should have been returned
assert workflow_version == "0.0.0"
Loading