From dd33e43bf47853df38c62b72d77b777e71dc32c3 Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Fri, 28 Jul 2023 15:01:52 -0700 Subject: [PATCH 01/14] add smoke test --- action/deploy_recipe.py | 7 ++++++- action/test_main.py | 27 +++++++++++++++++++++++++++ dev-requirements.txt | 2 ++ 3 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 action/test_main.py create mode 100644 dev-requirements.txt diff --git a/action/deploy_recipe.py b/action/deploy_recipe.py index ed90293..7571e7d 100644 --- a/action/deploy_recipe.py +++ b/action/deploy_recipe.py @@ -1,6 +1,7 @@ import json import os import subprocess +import sys import tempfile from urllib.parse import urljoin @@ -26,7 +27,7 @@ def deploy_recipe_cmd(cmd: list[str]): print(f"Job submitted with {job_id = } and {job_name = }") -if __name__ == "__main__": +def main(): # set in Dockerfile conda_env = os.environ["CONDA_ENV"] @@ -118,3 +119,7 @@ def deploy_recipe_cmd(cmd: list[str]): # passing a `--Bake.job_name_append` option to pangeo-forge-runner, which is a user- # defined string to append to the job names. deploy_recipe_cmd(cmd) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/action/test_main.py b/action/test_main.py new file mode 100644 index 0000000..d21dd5a --- /dev/null +++ b/action/test_main.py @@ -0,0 +1,27 @@ +import os +from unittest.mock import patch + +from deploy_recipe import main + + +@patch("deploy_recipe.open") +@patch("deploy_recipe.os.listdir") +@patch("deploy_recipe.subprocess") +@patch("deploy_recipe.requests") +@patch.dict( + os.environ, + { + "CONDA_ENV": "0", + "GITHUB_REPOSITORY": "1", + "GITHUB_SERVER_URL": "2", + "GITHUB_API_URL": "3", + "GITHUB_HEAD_REF": "4", + "GITHUB_REPOSITORY_ID": "5", + "GITHUB_RUN_ID": "6", + "GITHUB_RUN_ATTEMPT": "7", + "INPUT_PANGEO_FORGE_RUNNER_CONFIG": "8", + "INPUT_SELECT_RECIPE_BY_LABEL": "9", + }, +) +def test_main(open, listdir, subprocess, requests): + main() diff --git a/dev-requirements.txt b/dev-requirements.txt new file mode 100644 index 0000000..547de5c --- /dev/null +++ b/dev-requirements.txt @@ -0,0 +1,2 @@ +pytest +requests From de8bef794b78571eb11c8948c73f32fd5ba366bd Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Fri, 28 Jul 2023 15:15:07 -0700 Subject: [PATCH 02/14] add test workflow --- .github/workflows/test.yaml | 35 +++++++++++++++++++++++++++++++++++ dev-requirements.txt | 1 + 2 files changed, 36 insertions(+) create mode 100644 .github/workflows/test.yaml diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml new file mode 100644 index 0000000..24698c4 --- /dev/null +++ b/.github/workflows/test.yaml @@ -0,0 +1,35 @@ +name: Test + +on: + push: + branches: [ "main" ] + pull_request: + branches: [ "*" ] + +jobs: + test: + + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + python-version: ["3.9.13"] # consistent with Dockerfile + + steps: + - uses: actions/checkout@v3 + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v3 + with: + python-version: ${{ matrix.python-version }} + + - name: Install dependencies + run: | + python -m pip install --upgrade pip + python -m pip install -r dev-requirements.txt + + - name: Test with pytest + run: | + pytest -vvv -s --cov=action action/test_main.py + + - name: Upload Coverage to Codecov + uses: codecov/codecov-action@v2 diff --git a/dev-requirements.txt b/dev-requirements.txt index 547de5c..d985dd2 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -1,2 +1,3 @@ pytest +pytest-cov requests From 64fb4b7ee14d91738e8b7ebe19d4b6114716bb81 Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Fri, 28 Jul 2023 15:17:02 -0700 Subject: [PATCH 03/14] only cov deploy_recipe.py --- .github/workflows/test.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 24698c4..1d95611 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -29,7 +29,7 @@ jobs: - name: Test with pytest run: | - pytest -vvv -s --cov=action action/test_main.py + pytest -vvv -s --cov=action/deploy_recipe.py action/test_main.py - name: Upload Coverage to Codecov uses: codecov/codecov-action@v2 From d20c94d70da3649daa4f228c70454505141600fc Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Fri, 28 Jul 2023 15:20:17 -0700 Subject: [PATCH 04/14] guess we can't do that with cov --- .github/workflows/test.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 1d95611..24698c4 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -29,7 +29,7 @@ jobs: - name: Test with pytest run: | - pytest -vvv -s --cov=action/deploy_recipe.py action/test_main.py + pytest -vvv -s --cov=action action/test_main.py - name: Upload Coverage to Codecov uses: codecov/codecov-action@v2 From 27efcb49c640b2f212ff232bb46f73d6b950b783 Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Fri, 28 Jul 2023 15:46:30 -0700 Subject: [PATCH 05/14] test realism refinements --- .vscode/settings.json | 7 +++++++ action/test_main.py | 43 ++++++++++++++++++++++++++++++------------- 2 files changed, 37 insertions(+), 13 deletions(-) create mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000..f84c67f --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,7 @@ +{ + "python.testing.pytestArgs": [ + "action" + ], + "python.testing.unittestEnabled": false, + "python.testing.pytestEnabled": true +} \ No newline at end of file diff --git a/action/test_main.py b/action/test_main.py index d21dd5a..94a0576 100644 --- a/action/test_main.py +++ b/action/test_main.py @@ -1,27 +1,44 @@ import os -from unittest.mock import patch +from unittest.mock import MagicMock, patch from deploy_recipe import main @patch("deploy_recipe.open") @patch("deploy_recipe.os.listdir") -@patch("deploy_recipe.subprocess") +@patch("deploy_recipe.subprocess.run") @patch("deploy_recipe.requests") @patch.dict( os.environ, { - "CONDA_ENV": "0", - "GITHUB_REPOSITORY": "1", - "GITHUB_SERVER_URL": "2", - "GITHUB_API_URL": "3", - "GITHUB_HEAD_REF": "4", - "GITHUB_REPOSITORY_ID": "5", - "GITHUB_RUN_ID": "6", - "GITHUB_RUN_ATTEMPT": "7", - "INPUT_PANGEO_FORGE_RUNNER_CONFIG": "8", - "INPUT_SELECT_RECIPE_BY_LABEL": "9", + "CONDA_ENV": "notebook", + "GITHUB_REPOSITORY": "my/repo", + "GITHUB_SERVER_URL": "https://github.com", + "GITHUB_API_URL": "https://api.github.com", + "GITHUB_HEAD_REF": "abcdefg", + "GITHUB_REPOSITORY_ID": "1234567890", + "GITHUB_RUN_ID": "0987654321", + "GITHUB_RUN_ATTEMPT": "1", + "INPUT_PANGEO_FORGE_RUNNER_CONFIG": '{"a": "b"}', + "INPUT_SELECT_RECIPE_BY_LABEL": "", }, ) -def test_main(open, listdir, subprocess, requests): +def test_main( + requests: MagicMock, + subprocess_run: MagicMock, + listdir: MagicMock, + open: MagicMock, +): + class MockProcess: + stdout = b'{"job_id": "foo", "job_name": "bar"}' + stderr = b"456" + returncode = 0 + + subprocess_run.return_value = MockProcess() main() + # open is only called if listdir('feedstock') contains requirements.txt + # open.assert_called_once() + listdir.assert_called_once() + subprocess_run.assert_called() + # requests is only called if INPUT_SELECT_RECIPE_BY_LABEL=true + # requests.assert_called() From fd84971afb91bc86977f42a0e159b5a21a53a09e Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Fri, 28 Jul 2023 16:09:19 -0700 Subject: [PATCH 06/14] fixtures --- action/test_main.py | 106 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 83 insertions(+), 23 deletions(-) diff --git a/action/test_main.py b/action/test_main.py index 94a0576..25d5aac 100644 --- a/action/test_main.py +++ b/action/test_main.py @@ -1,16 +1,20 @@ import os +from dataclasses import dataclass from unittest.mock import MagicMock, patch +import pytest + from deploy_recipe import main -@patch("deploy_recipe.open") -@patch("deploy_recipe.os.listdir") -@patch("deploy_recipe.subprocess.run") -@patch("deploy_recipe.requests") -@patch.dict( - os.environ, - { +@pytest.fixture( + params=[ + dict(INPUT_SELECT_RECIPE_BY_LABEL=""), + dict(INPUT_SELECT_RECIPE_BY_LABEL="true"), + ] +) +def env(request): + return { "CONDA_ENV": "notebook", "GITHUB_REPOSITORY": "my/repo", "GITHUB_SERVER_URL": "https://github.com", @@ -20,25 +24,81 @@ "GITHUB_RUN_ID": "0987654321", "GITHUB_RUN_ATTEMPT": "1", "INPUT_PANGEO_FORGE_RUNNER_CONFIG": '{"a": "b"}', - "INPUT_SELECT_RECIPE_BY_LABEL": "", - }, + "INPUT_SELECT_RECIPE_BY_LABEL": request.param["INPUT_SELECT_RECIPE_BY_LABEL"], + } + + +@dataclass +class MockProcess: + stdout: bytes + stderr: bytes + returncode: int + + +@pytest.fixture +def subprocess_return_value(): + return MockProcess( + stdout=b'{"job_id": "foo", "job_name": "bar"}', + stderr=b"", + returncode=0, + ) + + +@pytest.fixture( + params=[ + ["meta.yaml", "recipe.py"], + ["meta.yaml", "recipe.py", "requirements.txt"] + ] ) +def listdir_return_value(request): + return request.param + + +@patch("deploy_recipe.open") +@patch("deploy_recipe.os.listdir") +@patch("deploy_recipe.subprocess.run") +@patch("deploy_recipe.requests.get") def test_main( - requests: MagicMock, + # Note that patches are passed to `test_main` by order, not name. + # See https://docs.python.org/3/library/unittest.mock.html#quick-guide: + # "When you nest patch decorators the mocks are passed in to the decorated + # function in the same order they applied (the normal Python order that + # decorators are applied). This means from the bottom up..." + requests_get: MagicMock, subprocess_run: MagicMock, listdir: MagicMock, open: MagicMock, + env: dict, + subprocess_return_value: MockProcess, + listdir_return_value: list, ): - class MockProcess: - stdout = b'{"job_id": "foo", "job_name": "bar"}' - stderr = b"456" - returncode = 0 - - subprocess_run.return_value = MockProcess() - main() - # open is only called if listdir('feedstock') contains requirements.txt - # open.assert_called_once() - listdir.assert_called_once() - subprocess_run.assert_called() - # requests is only called if INPUT_SELECT_RECIPE_BY_LABEL=true - # requests.assert_called() + + subprocess_run.return_value = subprocess_return_value + listdir.return_value = listdir_return_value + + with patch.dict(os.environ, env): + main() + + # open is only called if listdir('feedstock') contains requirements.txt + # FIXME: Following fix of https://github.com/pangeo-forge/deploy-recipe-action/issues/12 + # open should never be called. Instead, subprocess.run should be called with: + # `pip install -r requirements.txt` + if "requirements.txt" in listdir_return_value: + open.assert_called_once() + else: + open.assert_not_called() + + listdir.assert_called_once() + + if env["INPUT_SELECT_RECIPE_BY_LABEL"]: + # requests.get is always called if INPUT_SELECT_RECIPE_BY_LABEL=true + # (to check github api for PR `run:...` labels) + requests_get.assert_called() + + has_run_labels = False # FIXME: fixturize this + if has_run_labels: + subprocess_run.assert_called() + + else: + # subprocess.run is always called if INPUT_SELECT_RECIPE_BY_LABEL is empty + subprocess_run.assert_called() From f2a11e2a28e82e227101da9a03a92d5b60d53d2d Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Fri, 28 Jul 2023 16:29:43 -0700 Subject: [PATCH 07/14] specify mock requests get response typing --- action/test_main.py | 43 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/action/test_main.py b/action/test_main.py index 25d5aac..f72a974 100644 --- a/action/test_main.py +++ b/action/test_main.py @@ -1,5 +1,6 @@ import os from dataclasses import dataclass +from typing import TypedDict from unittest.mock import MagicMock, patch import pytest @@ -28,6 +29,39 @@ def env(request): } +class MockPRLabels(TypedDict): + name: str + + +class MockPRHead(TypedDict): + ref: str + + +class MockGitHubPullsResponse(TypedDict): + labels: list[MockPRLabels] + head: MockPRHead + + +@dataclass +class MockRequestsGetResponse: + _json: list[MockGitHubPullsResponse] + + def json(self) -> list[MockGitHubPullsResponse]: + return self._json + + def raise_for_status(self): + pass + + +@pytest.fixture +def requests_get_return_value(): + return MockRequestsGetResponse( + _json=[ + {"labels": [{"name": "run:my-recipe"}], "head":{"ref": "abcdefg"}} + ], + ) + + @dataclass class MockProcess: stdout: bytes @@ -69,10 +103,12 @@ def test_main( listdir: MagicMock, open: MagicMock, env: dict, + requests_get_return_value: MockRequestsGetResponse, subprocess_return_value: MockProcess, listdir_return_value: list, ): + requests_get.return_value = requests_get_return_value subprocess_run.return_value = subprocess_return_value listdir.return_value = listdir_return_value @@ -95,8 +131,11 @@ def test_main( # (to check github api for PR `run:...` labels) requests_get.assert_called() - has_run_labels = False # FIXME: fixturize this - if has_run_labels: + if any( + label["name"].startswith("run:") + for pull_request in requests_get_return_value.json() + for label in pull_request["labels"] + ): subprocess_run.assert_called() else: From a1699a71dc29ce3d1c0e8d9985c92713e905ab69 Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Fri, 28 Jul 2023 16:49:48 -0700 Subject: [PATCH 08/14] mock tempfile and assert_called_with --- action/test_main.py | 47 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 5 deletions(-) diff --git a/action/test_main.py b/action/test_main.py index f72a974..cba5f8d 100644 --- a/action/test_main.py +++ b/action/test_main.py @@ -88,16 +88,34 @@ def listdir_return_value(request): return request.param +class MockNamedTemporaryFile: + name = "mock-temp-file" + + def __enter__(self): + return self + + def __exit__(self, *args): + pass + + def write(self, *args): + pass + + def flush(self): + pass + + @patch("deploy_recipe.open") @patch("deploy_recipe.os.listdir") @patch("deploy_recipe.subprocess.run") @patch("deploy_recipe.requests.get") +@patch("deploy_recipe.tempfile.NamedTemporaryFile") def test_main( # Note that patches are passed to `test_main` by order, not name. # See https://docs.python.org/3/library/unittest.mock.html#quick-guide: # "When you nest patch decorators the mocks are passed in to the decorated # function in the same order they applied (the normal Python order that # decorators are applied). This means from the bottom up..." + named_temporary_file: MagicMock, requests_get: MagicMock, subprocess_run: MagicMock, listdir: MagicMock, @@ -106,7 +124,9 @@ def test_main( requests_get_return_value: MockRequestsGetResponse, subprocess_return_value: MockProcess, listdir_return_value: list, -): +): + mock_named_temporary_file = MockNamedTemporaryFile() + named_temporary_file.return_value = mock_named_temporary_file requests_get.return_value = requests_get_return_value subprocess_run.return_value = subprocess_return_value @@ -131,12 +151,29 @@ def test_main( # (to check github api for PR `run:...` labels) requests_get.assert_called() - if any( - label["name"].startswith("run:") + run_labels = [ + label["name"].split("run:")[-1] for pull_request in requests_get_return_value.json() for label in pull_request["labels"] - ): - subprocess_run.assert_called() + if label["name"].startswith("run:") + ] + if run_labels: + subprocess_run.assert_called_with( + [ + 'pangeo-forge-runner', + 'bake', + '--repo', + f'{env["GITHUB_SERVER_URL"]}/{env["GITHUB_REPOSITORY"]}', + '--ref', + env["GITHUB_HEAD_REF"], + '--json', + '-f', + mock_named_temporary_file.name, + f'--Bake.recipe_id={run_labels[-1]}', + f'--Bake.job_name=my-recipe-1234567890-0987654321-1' + ], + capture_output=True, + ) else: # subprocess.run is always called if INPUT_SELECT_RECIPE_BY_LABEL is empty From c1deb0a3ae899a40f8e7a0fb992acc2b3b6024cb Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Mon, 31 Jul 2023 12:18:03 -0700 Subject: [PATCH 09/14] simplify mocks (oh, that's how it works) --- action/test_main.py | 86 ++++++++++----------------------------------- 1 file changed, 19 insertions(+), 67 deletions(-) diff --git a/action/test_main.py b/action/test_main.py index cba5f8d..e3b357c 100644 --- a/action/test_main.py +++ b/action/test_main.py @@ -1,6 +1,4 @@ import os -from dataclasses import dataclass -from typing import TypedDict from unittest.mock import MagicMock, patch import pytest @@ -29,51 +27,18 @@ def env(request): } -class MockPRLabels(TypedDict): - name: str - - -class MockPRHead(TypedDict): - ref: str - - -class MockGitHubPullsResponse(TypedDict): - labels: list[MockPRLabels] - head: MockPRHead - - -@dataclass -class MockRequestsGetResponse: - _json: list[MockGitHubPullsResponse] - - def json(self) -> list[MockGitHubPullsResponse]: - return self._json - - def raise_for_status(self): - pass - - @pytest.fixture -def requests_get_return_value(): - return MockRequestsGetResponse( - _json=[ - {"labels": [{"name": "run:my-recipe"}], "head":{"ref": "abcdefg"}} - ], - ) - - -@dataclass -class MockProcess: - stdout: bytes - stderr: bytes - returncode: int +def requests_get_returns_json(): + return [ + {"labels": [{"name": "run:my-recipe"}], "head":{"ref": "abcdefg"}} + ] @pytest.fixture -def subprocess_return_value(): - return MockProcess( - stdout=b'{"job_id": "foo", "job_name": "bar"}', - stderr=b"", +def subprocess_return_values(): + return dict( + stdout='{"job_id": "foo", "job_name": "bar"}', + stderr="", returncode=0, ) @@ -88,22 +53,6 @@ def listdir_return_value(request): return request.param -class MockNamedTemporaryFile: - name = "mock-temp-file" - - def __enter__(self): - return self - - def __exit__(self, *args): - pass - - def write(self, *args): - pass - - def flush(self): - pass - - @patch("deploy_recipe.open") @patch("deploy_recipe.os.listdir") @patch("deploy_recipe.subprocess.run") @@ -121,15 +70,18 @@ def test_main( listdir: MagicMock, open: MagicMock, env: dict, - requests_get_return_value: MockRequestsGetResponse, - subprocess_return_value: MockProcess, + requests_get_returns_json: list, + subprocess_return_values: dict, listdir_return_value: list, ): - mock_named_temporary_file = MockNamedTemporaryFile() - named_temporary_file.return_value = mock_named_temporary_file + mock_tempfile_name = "mock-temp-file.json" + named_temporary_file.return_value.__enter__.return_value.name = mock_tempfile_name + requests_get.return_value.json.return_value = requests_get_returns_json + + 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"] - requests_get.return_value = requests_get_return_value - subprocess_run.return_value = subprocess_return_value listdir.return_value = listdir_return_value with patch.dict(os.environ, env): @@ -153,7 +105,7 @@ def test_main( run_labels = [ label["name"].split("run:")[-1] - for pull_request in requests_get_return_value.json() + for pull_request in requests_get_returns_json for label in pull_request["labels"] if label["name"].startswith("run:") ] @@ -168,7 +120,7 @@ def test_main( env["GITHUB_HEAD_REF"], '--json', '-f', - mock_named_temporary_file.name, + mock_tempfile_name, f'--Bake.recipe_id={run_labels[-1]}', f'--Bake.job_name=my-recipe-1234567890-0987654321-1' ], From 0274520a1f4d32d797f6f60e0c6b63341a6a7cf0 Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Mon, 31 Jul 2023 12:24:27 -0700 Subject: [PATCH 10/14] clarify mock return values in comments --- action/test_main.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/action/test_main.py b/action/test_main.py index e3b357c..7180c77 100644 --- a/action/test_main.py +++ b/action/test_main.py @@ -53,6 +53,11 @@ def listdir_return_value(request): return request.param +@pytest.fixture +def mock_tempfile_name(): + return "mock-temp-file.json" + + @patch("deploy_recipe.open") @patch("deploy_recipe.os.listdir") @patch("deploy_recipe.subprocess.run") @@ -73,15 +78,20 @@ def test_main( requests_get_returns_json: list, subprocess_return_values: dict, listdir_return_value: list, + mock_tempfile_name: str, ): - mock_tempfile_name = "mock-temp-file.json" + # mock a context manager, see: https://stackoverflow.com/a/28852060 named_temporary_file.return_value.__enter__.return_value.name = mock_tempfile_name + + # 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 listdir call return value listdir.return_value = listdir_return_value with patch.dict(os.environ, env): From 881c679b2cfac6d0d7e7f1e57c1e70d80508b84a Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Mon, 31 Jul 2023 14:11:57 -0700 Subject: [PATCH 11/14] fix for issue #11 --- action/deploy_recipe.py | 14 ++------------ action/test_main.py | 9 ++------- 2 files changed, 4 insertions(+), 19 deletions(-) diff --git a/action/deploy_recipe.py b/action/deploy_recipe.py index 7571e7d..c8912b7 100644 --- a/action/deploy_recipe.py +++ b/action/deploy_recipe.py @@ -3,7 +3,6 @@ import subprocess import sys import tempfile -from urllib.parse import urljoin import requests @@ -33,7 +32,6 @@ def main(): # injected by github actions repository = os.environ["GITHUB_REPOSITORY"] # will this fail for external prs? - server_url = os.environ["GITHUB_SERVER_URL"] api_url = os.environ["GITHUB_API_URL"] ref = os.environ["GITHUB_HEAD_REF"] repository_id = os.environ["GITHUB_REPOSITORY_ID"] @@ -44,12 +42,8 @@ def main(): config = json.loads(os.environ["INPUT_PANGEO_FORGE_RUNNER_CONFIG"]) select_recipe_by_label = os.environ["INPUT_SELECT_RECIPE_BY_LABEL"] - # assemble https url for pangeo-forge-runner - repo = urljoin(server_url, repository) - # log variables to stdout print(f"{conda_env = }") - print(f"{repo = }") print(f"{ref = }") print(f"{config = }") @@ -91,13 +85,9 @@ def main(): cmd = [ "pangeo-forge-runner", "bake", - "--repo", - repo, - "--ref", - ref, + "--repo=.", "--json", - "-f", - f.name, + f"-f={f.name}", ] print("\nSubmitting job...") print(f"{recipe_ids = }") diff --git a/action/test_main.py b/action/test_main.py index 7180c77..2ae4c0e 100644 --- a/action/test_main.py +++ b/action/test_main.py @@ -16,7 +16,6 @@ def env(request): return { "CONDA_ENV": "notebook", "GITHUB_REPOSITORY": "my/repo", - "GITHUB_SERVER_URL": "https://github.com", "GITHUB_API_URL": "https://api.github.com", "GITHUB_HEAD_REF": "abcdefg", "GITHUB_REPOSITORY_ID": "1234567890", @@ -124,13 +123,9 @@ def test_main( [ 'pangeo-forge-runner', 'bake', - '--repo', - f'{env["GITHUB_SERVER_URL"]}/{env["GITHUB_REPOSITORY"]}', - '--ref', - env["GITHUB_HEAD_REF"], + '--repo=.', '--json', - '-f', - mock_tempfile_name, + f'-f={mock_tempfile_name}', f'--Bake.recipe_id={run_labels[-1]}', f'--Bake.job_name=my-recipe-1234567890-0987654321-1' ], From 29cd2736f71eb00addb540632eb286e90da2d17f Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Mon, 31 Jul 2023 16:02:05 -0700 Subject: [PATCH 12/14] fix for #12 --- action/deploy_recipe.py | 19 +++++++++++++------ action/test_main.py | 19 +++++++++++-------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/action/deploy_recipe.py b/action/deploy_recipe.py index c8912b7..6f50200 100644 --- a/action/deploy_recipe.py +++ b/action/deploy_recipe.py @@ -65,15 +65,22 @@ def main(): recipe_ids = [l.replace("run:", "") for l in labels if l.startswith("run:")] # dynamically install extra deps if requested. + # if calling `pangeo-forge-runner` directly, `--feedstock-subdir` can be passed as a CLI arg. + # in the action context, users do not compose their own `pangeo-forge-runner` CLI calls, so if + # they want to use a non-default value for feedstock-subdir, it must be passed via the long-form + # name in the config JSON (i.e, `{"BaseCommand": "feedstock-subdir": ...}}`). + feedstock_subdir = ( + config["BaseCommand"]["feedstock-subdir"] + if "BaseCommand" in config and "feedstock-subdir" in config["BaseCommand"] + else "feedstock" + ) # because we've run the actions/checkout step before reaching this point, our current # 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"): - with open("feedstock/requirements.txt") as f: - to_install = f.read().splitlines() - - print(f"Installing extra packages {to_install}...") - install_cmd = f"mamba run -n {conda_env} pip install -U".split() + to_install + if "requirements.txt" in os.listdir(feedstock_subdir): + to_install = f"{feedstock_subdir}/requirements.txt" + print(f"Installing extra packages from {to_install}...") + install_cmd = f"mamba run -n {conda_env} pip install -Ur {to_install}".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 diff --git a/action/test_main.py b/action/test_main.py index 2ae4c0e..e865286 100644 --- a/action/test_main.py +++ b/action/test_main.py @@ -21,6 +21,7 @@ def env(request): "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_SELECT_RECIPE_BY_LABEL": request.param["INPUT_SELECT_RECIPE_BY_LABEL"], } @@ -57,7 +58,6 @@ def mock_tempfile_name(): return "mock-temp-file.json" -@patch("deploy_recipe.open") @patch("deploy_recipe.os.listdir") @patch("deploy_recipe.subprocess.run") @patch("deploy_recipe.requests.get") @@ -72,7 +72,6 @@ def test_main( requests_get: MagicMock, subprocess_run: MagicMock, listdir: MagicMock, - open: MagicMock, env: dict, requests_get_returns_json: list, subprocess_return_values: dict, @@ -96,14 +95,18 @@ def test_main( with patch.dict(os.environ, env): main() - # open is only called if listdir('feedstock') contains requirements.txt - # FIXME: Following fix of https://github.com/pangeo-forge/deploy-recipe-action/issues/12 - # open should never be called. Instead, subprocess.run should be called with: - # `pip install -r requirements.txt` if "requirements.txt" in listdir_return_value: - open.assert_called_once() + to_install = "feedstock/requirements.txt" # TODO: parametrize + subprocess_run.assert_any_call( + f"mamba run -n {env['CONDA_ENV']} pip install -Ur {to_install}".split(), + capture_output=True, + text=True, + ) else: - open.assert_not_called() + # if 'requirements.txt' not present, 'pip' is never invoked. re: `call_args_list`, see: + # https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.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() From 6eab39f26d1d2ee63dc17f69c2fd582d605d2ad6 Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Mon, 31 Jul 2023 18:20:07 -0700 Subject: [PATCH 13/14] fix for #4 --- action/deploy_recipe.py | 27 +++++++++++++++----------- action/test_main.py | 43 +++++++++++++++++++++++++++-------------- 2 files changed, 45 insertions(+), 25 deletions(-) diff --git a/action/deploy_recipe.py b/action/deploy_recipe.py index 6f50200..d4b3526 100644 --- a/action/deploy_recipe.py +++ b/action/deploy_recipe.py @@ -33,7 +33,8 @@ def main(): # injected by github actions repository = os.environ["GITHUB_REPOSITORY"] # will this fail for external prs? api_url = os.environ["GITHUB_API_URL"] - ref = os.environ["GITHUB_HEAD_REF"] + head_ref = os.environ["GITHUB_HEAD_REF"] + sha = os.environ["GITHUB_SHA"] repository_id = os.environ["GITHUB_REPOSITORY_ID"] run_id = os.environ["GITHUB_RUN_ID"] run_attempt = os.environ["GITHUB_RUN_ATTEMPT"] @@ -44,24 +45,28 @@ def main(): # log variables to stdout print(f"{conda_env = }") - print(f"{ref = }") + print(f"{head_ref = }") + print(f"{sha = }") print(f"{config = }") labels = [] if select_recipe_by_label: - # iterate through PRs on the repo. if the PR's head ref is the same - # as our ``ref`` here, get the labels from that PR, and stop iteration. - # FIXME: what if this is a push event, and not a pull_request event? - pulls_url = "/".join([api_url, "repos", repository, "pulls"]) - print(f"Fetching pulls from {pulls_url}") + # `head_ref` is available for `pull_request` events. on a `push` event, the `head_ref` + # environment variable will be empty, so in that case we use `sha` to find the PR instead. + commit_sha = head_ref if head_ref else sha + # querying the following endpoint should give us the PR containing the desired labels, + # regardless of whether this is a `pull_request` or `push` event. see official docs here: + # https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#list-pull-requests-associated-with-a-commit + pulls_url = "/".join([api_url, "repos", repository, "commits", commit_sha, "pulls"]) + headers = {"X-GitHub-Api-Version": "2022-11-28", "Accept": "application/vnd.github+json"} - pulls_response = requests.get(pulls_url) + print(f"Fetching pulls from {pulls_url}") + pulls_response = requests.get(pulls_url, headers=headers) pulls_response.raise_for_status() pulls = pulls_response.json() + assert len(pulls) == 1 # pretty sure this is always true, but just making sure + labels: list[str] = [label["name"] for label in pulls[0]["labels"]] - for p in pulls: - if p["head"]["ref"] == ref: - labels: list[str] = [label["name"] for label in p["labels"]] recipe_ids = [l.replace("run:", "") for l in labels if l.startswith("run:")] # dynamically install extra deps if requested. diff --git a/action/test_main.py b/action/test_main.py index e865286..0b16e30 100644 --- a/action/test_main.py +++ b/action/test_main.py @@ -6,31 +6,39 @@ from deploy_recipe import main -@pytest.fixture( - params=[ - dict(INPUT_SELECT_RECIPE_BY_LABEL=""), - dict(INPUT_SELECT_RECIPE_BY_LABEL="true"), - ] -) -def env(request): +@pytest.fixture(params=["", "abcdefg"]) +def head_ref(request): + return request.param + + +@pytest.fixture(params=["", "true"]) +def select_recipe_by_label(request): + return request.param + + +@pytest.fixture +def env(select_recipe_by_label, head_ref): return { "CONDA_ENV": "notebook", "GITHUB_REPOSITORY": "my/repo", "GITHUB_API_URL": "https://api.github.com", - "GITHUB_HEAD_REF": "abcdefg", + # fixturing of `head_ref` reflects that on `push` + # events, `GITHUB_HEAD_REF` env var is empty + "GITHUB_HEAD_REF": head_ref, + "GITHUB_SHA": "gfedcba", "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_SELECT_RECIPE_BY_LABEL": request.param["INPUT_SELECT_RECIPE_BY_LABEL"], + "INPUT_SELECT_RECIPE_BY_LABEL": select_recipe_by_label, } @pytest.fixture def requests_get_returns_json(): return [ - {"labels": [{"name": "run:my-recipe"}], "head":{"ref": "abcdefg"}} + {"labels": [{"name": "run:my-recipe"}]} ] @@ -109,11 +117,18 @@ def test_main( assert "pip" not in call listdir.assert_called_once() - + if env["INPUT_SELECT_RECIPE_BY_LABEL"]: - # requests.get is always called if INPUT_SELECT_RECIPE_BY_LABEL=true - # (to check github api for PR `run:...` labels) - requests_get.assert_called() + # requests.get is always called if INPUT_SELECT_RECIPE_BY_LABEL=true (to check github + # api for PR `run:...` labels). if this is a `pull_request` event, the called gh api + # url should contain the `GITHUB_HEAD_REF` for that PR. if this is a `push` event + # (reflected in the env by the fact that `GITHUB_HEAD_REF` is empty), then we expect to + # be calling an api url for the `GITHUB_SHA` associated with the `push`. + called_gh_api_url = requests_get.call_args[0][0] + if env["GITHUB_HEAD_REF"]: + assert env["GITHUB_HEAD_REF"] in called_gh_api_url + else: + assert env["GITHUB_SHA"] in called_gh_api_url run_labels = [ label["name"].split("run:")[-1] From 54dc84b691002a1c6603940fa3273190ff23510b Mon Sep 17 00:00:00 2001 From: Charles Stern <62192187+cisaacstern@users.noreply.github.com> Date: Tue, 1 Aug 2023 12:37:18 -0700 Subject: [PATCH 14/14] ignore vscode config --- .gitignore | 3 +++ .vscode/settings.json | 7 ------- 2 files changed, 3 insertions(+), 7 deletions(-) delete mode 100644 .vscode/settings.json diff --git a/.gitignore b/.gitignore index 68bc17f..6a9390b 100644 --- a/.gitignore +++ b/.gitignore @@ -158,3 +158,6 @@ cython_debug/ # and can be added to the global gitignore or merged into this file. For a more nuclear # option (not recommended) you can uncomment the following to ignore the entire idea folder. #.idea/ + +# vscode +.vscode diff --git a/.vscode/settings.json b/.vscode/settings.json deleted file mode 100644 index f84c67f..0000000 --- a/.vscode/settings.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "python.testing.pytestArgs": [ - "action" - ], - "python.testing.unittestEnabled": false, - "python.testing.pytestEnabled": true -} \ No newline at end of file