diff --git a/action/deploy_recipe.py b/action/deploy_recipe.py index 7361511..cb15aa5 100644 --- a/action/deploy_recipe.py +++ b/action/deploy_recipe.py @@ -7,7 +7,8 @@ import requests -def deploy_recipe_cmd(cmd: list[str]): +def call_subprocess_run(cmd: list[str]) -> str: + """Convenience wrapper for `subprocess.run` with stdout/stderr handling.""" print(f"Calling subprocess with {cmd = }") submit_proc = subprocess.run(cmd, capture_output=True) stdout = submit_proc.stdout.decode() @@ -18,21 +19,17 @@ def deploy_recipe_cmd(cmd: list[str]): if submit_proc.returncode != 0: for line in stderr.splitlines(): print(line) - raise ValueError("Job submission failed.") - else: - lastline = json.loads(stdout.splitlines()[-1]) - job_id = lastline["job_id"] - job_name = lastline["job_name"] - print(f"Job submitted with {job_id = } and {job_name = }") + raise ValueError(f"{cmd = } failed. See logging for details.") + return stdout -def pip_install(requirements_file_path: str, conda_env: str): - print(f"Installing extra packages from {requirements_file_path}...") - install_cmd = f"mamba run -n {conda_env} pip install -Ur {requirements_file_path}".split() - install_proc = subprocess.run(install_cmd, capture_output=True, text=True) - if install_proc.returncode != 0: - # installations failed, so record the error and bail early - raise ValueError(f"Installs failed with {install_proc.stderr = }") +def deploy_recipe_cmd(cmd: list[str]): + """Wrapper for `call_subprocess_run` with extra stdout parsing when deploying recipes.""" + stdout = call_subprocess_run(cmd) + lastline = json.loads(stdout.splitlines()[-1]) + job_id = lastline["job_id"] + job_name = lastline["job_name"] + print(f"Job submitted with {job_id = } and {job_name = }") def main(): @@ -92,7 +89,9 @@ def main(): # working directory is the root of the feedstock repo, so we can list feedstock repo # contents directly on the filesystem here, without requesting it from github. if "requirements.txt" in os.listdir(feedstock_subdir): - pip_install(f"{feedstock_subdir}/requirements.txt", conda_env) + call_subprocess_run( + f"mamba run -n {conda_env} pip install -Ur {feedstock_subdir}/requirements.txt".split() + ) with tempfile.NamedTemporaryFile("w", suffix=".json") as f: json.dump(config, f) diff --git a/tests/test_main.py b/tests/test_main.py index 2534d95..4d8c443 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -1,5 +1,7 @@ +import json import os import sys +from dataclasses import dataclass from pathlib import Path from unittest.mock import MagicMock, patch @@ -22,8 +24,17 @@ def select_recipe_by_label(request): return request.param +@pytest.fixture(params=["config", "broken-reqs"]) +def pangeo_forge_runner_config(request): + if request.param == "broken-reqs": + # TODO: possibly improve this, it's an awkward way to parametrize a broken requirements.txt + return json.dumps({"BaseCommand": {"feedstock-subdir": "broken-requirements-feedstock"}}) + else: + return '{"a": "b"}' + + @pytest.fixture -def env(select_recipe_by_label, head_ref): +def env(select_recipe_by_label, head_ref, pangeo_forge_runner_config): return { "CONDA_ENV": "notebook", "GITHUB_REPOSITORY": "my/repo", @@ -35,8 +46,7 @@ def env(select_recipe_by_label, head_ref): "GITHUB_REPOSITORY_ID": "1234567890", "GITHUB_RUN_ID": "0987654321", "GITHUB_RUN_ATTEMPT": "1", - # TODO: parametrize runner config with `BaseCommand.feedstock-subdir` - "INPUT_PANGEO_FORGE_RUNNER_CONFIG": '{"a": "b"}', + "INPUT_PANGEO_FORGE_RUNNER_CONFIG": pangeo_forge_runner_config, "INPUT_SELECT_RECIPE_BY_LABEL": select_recipe_by_label, } @@ -48,24 +58,39 @@ def requests_get_returns_json(): ] +@dataclass +class MockCompletedProcess: + stdout: bytes + stderr: bytes + returncode: int + + @pytest.fixture -def subprocess_return_values(): - return dict( - stdout='{"job_id": "foo", "job_name": "bar"}', - stderr="", - returncode=0, - ) +def subprocess_run_side_effect(): + def _get_proc(cmd: list[str], *args, **kwargs): + if "broken-requirements-feedstock" in " ".join(cmd): + return MockCompletedProcess( + stdout=b"", + stderr=b"", + returncode=1, + ) + else: + return MockCompletedProcess( + stdout=b'{"job_id": "foo", "job_name": "bar"}', + stderr=b"", + returncode=0, + ) + return _get_proc @pytest.fixture( params=[ - (["meta.yaml", "recipe.py"], None), - (["meta.yaml", "recipe.py", "requirements.txt"], None), - (["meta.yaml", "recipe.py", "requirements.txt"], ValueError), + ["meta.yaml", "recipe.py"], + ["meta.yaml", "recipe.py", "requirements.txt"], ], - ids=["no_reqs", "has_reqs", "broken_reqs"], + ids=["no_reqs", "has_reqs"], ) -def listdir_return_value_with_pip_install_raises(request): +def listdir_return_value(request): return request.param @@ -75,7 +100,6 @@ def mock_tempfile_name(): @patch("deploy_recipe.os.listdir") -@patch("deploy_recipe.pip_install") @patch("deploy_recipe.subprocess.run") @patch("deploy_recipe.requests.get") @patch("deploy_recipe.tempfile.NamedTemporaryFile") @@ -88,12 +112,11 @@ def test_main( named_temporary_file: MagicMock, requests_get: MagicMock, subprocess_run: MagicMock, - pip_install: MagicMock, listdir: MagicMock, env: dict, requests_get_returns_json: list, - subprocess_return_values: dict, - listdir_return_value_with_pip_install_raises: list[str], + subprocess_run_side_effect: dict, + listdir_return_value: list[str], mock_tempfile_name: str, ): # mock a context manager, see: https://stackoverflow.com/a/28852060 @@ -102,19 +125,17 @@ def test_main( # mock reponse of requests.get call to github api requests_get.return_value.json.return_value = requests_get_returns_json - # mock result of subprocess call to `pangeo-forge-runner` - subprocess_run.return_value.stdout.decode.return_value = subprocess_return_values["stdout"] - subprocess_run.return_value.stderr.decode.return_value = subprocess_return_values["stderr"] - subprocess_run.return_value.returncode = subprocess_return_values["returncode"] + # mock result of subprocess calls + subprocess_run.side_effect = subprocess_run_side_effect # mock listdir call return value, and pip install side effect - listdir_return, pip_install_raises = listdir_return_value_with_pip_install_raises - listdir.return_value = listdir_return - if pip_install_raises: - pip_install.side_effect = pip_install_raises() + listdir.return_value = listdir_return_value with patch.dict(os.environ, env): - if pip_install_raises: + if ( + "broken-requirements-feedstock" in env["INPUT_PANGEO_FORGE_RUNNER_CONFIG"] + and any([path.endswith("requirements.txt") for path in listdir_return_value]) + ): with pytest.raises(ValueError): main() # if pip install fails, we should bail early & never invoke `bake`. re: `call_args_list`: @@ -124,12 +145,22 @@ def test_main( else: main() - if any([path.endswith("requirements.txt") for path in listdir_return]): - pip_install.assert_any_call("feedstock/requirements.txt", env['CONDA_ENV']) + if any([path.endswith("requirements.txt") for path in listdir_return_value]): + config = json.loads(env["INPUT_PANGEO_FORGE_RUNNER_CONFIG"]) + feedstock_subdir = ( + config["BaseCommand"]["feedstock-subdir"] + if "BaseCommand" in config + else "feedstock" + ) + expected_cmd = ( + f"mamba run -n {env['CONDA_ENV']} " + f"pip install -Ur {feedstock_subdir}/requirements.txt" + ).split() + subprocess_run.assert_any_call(expected_cmd, capture_output=True) else: # if 'requirements.txt' not present, 'pip' is never invoked. # re: `call_args_list`: see link in comment in `if pip_install_raises:` block above. - for call in [args[0][0] for args in pip_install.call_args_list]: + for call in [args[0][0] for args in subprocess_run.call_args_list]: assert "pip" not in call listdir.assert_called_once()