From fe4def75dc0a316789cec068a574713a2b2af92f Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Tue, 12 Jul 2022 09:25:05 +0400 Subject: Add GitHub Artifact API View Adds an API route to fetch GitHub build artifacts through a GitHub app. Signed-off-by: Hassan Abouelela --- pydis_site/apps/api/tests/test_github_utils.py | 287 +++++++++++++++++++++++++ 1 file changed, 287 insertions(+) create mode 100644 pydis_site/apps/api/tests/test_github_utils.py (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/tests/test_github_utils.py b/pydis_site/apps/api/tests/test_github_utils.py new file mode 100644 index 00000000..dc17d609 --- /dev/null +++ b/pydis_site/apps/api/tests/test_github_utils.py @@ -0,0 +1,287 @@ +import asyncio +import datetime +import random +import unittest +from unittest import mock + +import django.test +import httpx +import jwt +import rest_framework.response +import rest_framework.test +from django.urls import reverse + +from .. import github_utils + + +def patched_raise_for_status(response: httpx.Response): + """Fake implementation of raise_for_status which does not need a request to be set.""" + if response.status_code // 100 != 2: # pragma: no cover + raise httpx.HTTPStatusError( + f"Non 2xx response code: {response.status_code}", + request=getattr(response, "_request", httpx.Request("GET", "")), + response=response + ) + + +class GeneralUtilityTests(unittest.TestCase): + """Test the utility methods which do not fit in another class.""" + + def test_token_generation(self): + """Test that the a valid JWT token is generated.""" + def encode(payload: dict, _: str, algorithm: str, *args, **kwargs) -> str: + """ + Intercept the encode method. + + It is performed with an algorithm which does not require a PEM key, as it may + not be available in testing environments. + """ + self.assertEqual("RS256", algorithm, "The GitHub App JWT must be signed using RS256.") + return original_encode( + payload, "secret-encoding-key", algorithm="HS256", *args, **kwargs + ) + + original_encode = jwt.encode + with mock.patch("jwt.encode", new=encode): + token = github_utils.generate_token() + decoded = jwt.decode(token, "secret-encoding-key", algorithms=["HS256"]) + + delta = datetime.timedelta(minutes=10) + self.assertAlmostEqual(decoded["exp"] - decoded["iat"], delta.total_seconds()) + self.assertLess(decoded["exp"], (datetime.datetime.now() + delta).timestamp()) + + +@mock.patch("httpx.AsyncClient", autospec=True) +@mock.patch("asyncio.sleep", new=mock.AsyncMock(return_value=asyncio.Future)) +@mock.patch("httpx.Response.raise_for_status", new=patched_raise_for_status) +class WaitForTests(unittest.IsolatedAsyncioTestCase): + """Tests the wait_for utility.""" + + async def test_wait_for_successful_run(self, client_mock: mock.Mock): + """Test that the wait_for method handles successfully runs.""" + final_url = "some_url" + str(random.randint(0, 10)) + + client_mock.get.side_effect = responses = [ + httpx.Response(200, json={"status": "queued", "url": ""}), + httpx.Response(200, json={"status": "pending", "url": ""}), + httpx.Response(200, json={ + "status": "completed", + "conclusion": "success", + "url": "", + "artifacts_url": final_url + }) + ] + + result = await github_utils.wait_for_run(client_mock, responses[0].json()) + self.assertEqual(final_url, result) + + async def test_wait_for_failed_run(self, client_mock: mock.Mock): + """Test that the wait_for method handles failed runs.""" + client_mock.get.return_value = httpx.Response(200, json={ + "status": "completed", + "conclusion": "failed", + }) + + with self.assertRaises(github_utils.ActionFailedError): + await github_utils.wait_for_run(client_mock, {"status": "pending", "url": ""}) + + async def test_wait_for_timeout(self, client_mock: mock.Mock): + """Test that the wait_for method quits after a few attempts.""" + client_mock.get.side_effect = responses = [ + httpx.Response(200, json={"status": "pending", "url": ""}) + ] * (github_utils.MAX_POLLS + 5) + + with self.assertRaises(github_utils.RunTimeoutError): + await github_utils.wait_for_run(client_mock, responses[0].json()) + + +async def get_response_authorize( + _: httpx.AsyncClient, request: httpx.Request, **__ +) -> httpx.Response: + """ + Helper method for the authorize tests. + + Requests are intercepted before being sent out, and the appropriate responses are returned. + """ + path = request.url.path + auth = request.headers.get("Authorization") + + if request.method == "GET": + if path == "/app/installations": + if auth == "bearer JWT initial token": + return httpx.Response(200, request=request, json=[{ + "account": {"login": "VALID_OWNER"}, + "access_tokens_url": "https://example.com/ACCESS_TOKEN_URL" + }]) + else: + return httpx.Response( + 401, json={"error": "auth app/installations"}, request=request + ) + + elif path == "/installation/repositories": + if auth == "bearer app access token": + return httpx.Response(200, request=request, json={ + "repositories": [{ + "name": "VALID_REPO" + }] + }) + else: # pragma: no cover + return httpx.Response( + 401, json={"error": "auth installation/repositories"}, request=request + ) + + elif request.method == "POST": + if path == "/ACCESS_TOKEN_URL": + if auth == "bearer JWT initial token": + return httpx.Response(200, request=request, json={"token": "app access token"}) + else: # pragma: no cover + return httpx.Response(401, json={"error": "auth access_token"}, request=request) + + # Reaching this point means something has gone wrong + return httpx.Response(500, request=request) # pragma: no cover + + +@mock.patch("httpx.AsyncClient.send", new=get_response_authorize) +@mock.patch.object(github_utils, "generate_token", new=mock.Mock(return_value="JWT initial token")) +class AuthorizeTests(unittest.IsolatedAsyncioTestCase): + """Test the authorize utility.""" + + async def test_invalid_apps_auth(self): + """Test that an exception is raised if authorization was attempted with an invalid token.""" + with mock.patch.object(github_utils, "generate_token", return_value="Invalid token"): + with self.assertRaises(httpx.HTTPStatusError) as error: + await github_utils.authorize("VALID_OWNER", "VALID_REPO") + + exception: httpx.HTTPStatusError = error.exception + self.assertEqual(401, exception.response.status_code) + self.assertEqual("auth app/installations", exception.response.json()["error"]) + + async def test_missing_repo(self): + """Test that an exception is raised when the selected owner or repo are not available.""" + with self.assertRaises(github_utils.NotFoundError): + await github_utils.authorize("INVALID_OWNER", "VALID_REPO") + with self.assertRaises(github_utils.NotFoundError): + await github_utils.authorize("VALID_OWNER", "INVALID_REPO") + + async def test_valid_authorization(self): + """Test that an accessible repository can be accessed.""" + client = await github_utils.authorize("VALID_OWNER", "VALID_REPO") + self.assertEqual("bearer app access token", client.headers.get("Authorization")) + + +async def get_response_get_artifact(request: httpx.Request, **_) -> httpx.Response: + """ + Helper method for the get_artifact tests. + + Requests are intercepted before being sent out, and the appropriate responses are returned. + """ + path = request.url.path + + if "force_error" in path: + return httpx.Response(404, request=request) + + if request.method == "GET": + if path == "/repos/owner/repo/actions/runs": + return httpx.Response(200, request=request, json={"workflow_runs": [{ + "name": "action_name", + "head_sha": "action_sha" + }]}) + elif path == "/artifact_url": + return httpx.Response(200, request=request, json={"artifacts": [{ + "name": "artifact_name", + "archive_download_url": "artifact_download_url" + }]}) + elif path == "/artifact_download_url": + response = httpx.Response(302, request=request) + response.next_request = httpx.Request("GET", httpx.URL("https://final_download.url")) + return response + + # Reaching this point means something has gone wrong + return httpx.Response(500, request=request) # pragma: no cover + + +class ArtifactFetcherTests(unittest.IsolatedAsyncioTestCase): + """Test the get_artifact utility.""" + + def setUp(self) -> None: + self.call_args = ["owner", "repo", "action_sha", "action_name", "artifact_name"] + self.client = httpx.AsyncClient(base_url="https://example.com") + + self.patchers = [ + mock.patch.object(self.client, "send", new=get_response_get_artifact), + mock.patch.object(github_utils, "authorize", return_value=self.client), + mock.patch.object(github_utils, "wait_for_run", return_value="artifact_url"), + ] + + for patcher in self.patchers: + patcher.start() + + def tearDown(self) -> None: + for patcher in self.patchers: + patcher.stop() + + def test_client_closed_on_errors(self): + """Test that the client is terminated even if an error occurs at some point.""" + self.call_args[0] = "force_error" + with self.assertRaises(httpx.HTTPStatusError): + github_utils.get_artifact(*self.call_args) + self.assertTrue(self.client.is_closed) + + def test_missing(self): + """Test that an exception is raised if the requested artifact was not found.""" + cases = ( + "invalid sha", + "invalid action name", + "invalid artifact name", + ) + for i, name in enumerate(cases, 2): + with self.subTest(f"Test {name} raises an error"): + new_args = self.call_args.copy() + new_args[i] = name + + with self.assertRaises(github_utils.NotFoundError): + github_utils.get_artifact(*new_args) + + def test_valid(self): + """Test that the correct download URL is returned for valid requests.""" + url = github_utils.get_artifact(*self.call_args) + self.assertEqual("https://final_download.url", url) + self.assertTrue(self.client.is_closed) + + +@mock.patch.object(github_utils, "get_artifact") +class GitHubArtifactViewTests(django.test.TestCase): + """Test the GitHub artifact fetch API view.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + + cls.kwargs = { + "owner": "test_owner", + "repo": "test_repo", + "sha": "test_sha", + "action_name": "test_action", + "artifact_name": "test_artifact", + } + cls.url = reverse("api:github-artifacts", kwargs=cls.kwargs) + + async def test_successful(self, artifact_mock: mock.Mock): + """Test a proper response is returned with proper input.""" + artifact_mock.return_value = "final download url" + result = self.client.get(self.url) + + self.assertIsInstance(result, rest_framework.response.Response) + self.assertEqual({"url": artifact_mock.return_value}, result.data) + + async def test_failed_fetch(self, artifact_mock: mock.Mock): + """Test that a proper error is returned when the request fails.""" + artifact_mock.side_effect = github_utils.NotFoundError("Test error message") + result = self.client.get(self.url) + + self.assertIsInstance(result, rest_framework.response.Response) + self.assertEqual({ + "error_type": github_utils.NotFoundError.__name__, + "error": "Test error message", + "requested_resource": "/".join(self.kwargs.values()) + }, result.data) -- cgit v1.2.3 From 26a3c19b53883015e8ba87db2a668c3eece2ce20 Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Tue, 12 Jul 2022 14:45:22 +0400 Subject: Make Awaiting Workflow Run A User Responsibility Moves the responsibility of re-requesting a workflow run from the API to the user. This makes the requests much shorter-lived, and allows the client to control how they want to handle sleeping and retrying. This also has the benefit of removing the only real piece of async code, so now the view is completely sync once again. Signed-off-by: Hassan Abouelela --- pydis_site/apps/api/github_utils.py | 79 +++++------ pydis_site/apps/api/tests/test_github_utils.py | 174 ++++++++++++------------- static-builds/netlify_build.py | 9 +- 3 files changed, 131 insertions(+), 131 deletions(-) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/github_utils.py b/pydis_site/apps/api/github_utils.py index 70dccdff..707b36e5 100644 --- a/pydis_site/apps/api/github_utils.py +++ b/pydis_site/apps/api/github_utils.py @@ -1,17 +1,17 @@ """Utilities for working with the GitHub API.""" -import asyncio import datetime import math import httpx import jwt -from asgiref.sync import async_to_sync from pydis_site import settings -MAX_POLLS = 20 -"""The maximum number of attempts at fetching a workflow run.""" +MAX_RUN_TIME = datetime.timedelta(minutes=3) +"""The maximum time allowed before an action is declared timed out.""" +ISO_FORMAT_STRING = "%Y-%m-%dT%H:%M:%SZ" +"""The datetime string format GitHub uses.""" class ArtifactProcessingError(Exception): @@ -44,6 +44,12 @@ class RunTimeoutError(ArtifactProcessingError): status = 408 +class RunPendingError(ArtifactProcessingError): + """The requested workflow run is still pending, try again later.""" + + status = 202 + + def generate_token() -> str: """ Generate a JWT token to access the GitHub API. @@ -66,7 +72,7 @@ def generate_token() -> str: ) -async def authorize(owner: str, repo: str) -> httpx.AsyncClient: +def authorize(owner: str, repo: str) -> httpx.Client: """ Get an access token for the requested repository. @@ -75,7 +81,7 @@ async def authorize(owner: str, repo: str) -> httpx.AsyncClient: - POST to get a token to access the given app - GET installation/repositories and check if the requested one is part of those """ - client = httpx.AsyncClient( + client = httpx.Client( base_url=settings.GITHUB_API, headers={"Authorization": f"bearer {generate_token()}"}, timeout=settings.TIMEOUT_PERIOD, @@ -83,7 +89,7 @@ async def authorize(owner: str, repo: str) -> httpx.AsyncClient: try: # Get a list of app installations we have access to - apps = await client.get("app/installations") + apps = client.get("app/installations") apps.raise_for_status() for app in apps.json(): @@ -92,11 +98,11 @@ async def authorize(owner: str, repo: str) -> httpx.AsyncClient: continue # Get the repositories of the specified owner - app_token = await client.post(app["access_tokens_url"]) + app_token = client.post(app["access_tokens_url"]) app_token.raise_for_status() client.headers["Authorization"] = f"bearer {app_token.json()['token']}" - repos = await client.get("installation/repositories") + repos = client.get("installation/repositories") repos.raise_for_status() # Search for the request repository @@ -111,44 +117,39 @@ async def authorize(owner: str, repo: str) -> httpx.AsyncClient: except BaseException as e: # Close the client if we encountered an unexpected exception - await client.aclose() + client.close() raise e -async def wait_for_run(client: httpx.AsyncClient, run: dict) -> str: - """Wait for the provided `run` to finish, and return the URL to its artifacts.""" - polls = 0 - while polls <= MAX_POLLS: - if run["status"] != "completed": - # The action is still processing, wait a bit longer - polls += 1 - await asyncio.sleep(10) - - elif run["conclusion"] != "success": - # The action failed, or did not run - raise ActionFailedError(f"The requested workflow ended with: {run['conclusion']}") +def check_run_status(run: dict) -> str: + """Check if the provided run has been completed, otherwise raise an exception.""" + created_at = datetime.datetime.strptime(run["created_at"], ISO_FORMAT_STRING) + run_time = datetime.datetime.now() - created_at + if run["status"] != "completed": + if run_time <= MAX_RUN_TIME: + raise RunPendingError( + f"The requested run is still pending. It was created " + f"{run_time.seconds // 60}:{run_time.seconds % 60 :>02} minutes ago." + ) else: - # The desired action was found, and it ended successfully - return run["artifacts_url"] + raise RunTimeoutError("The requested workflow was not ready in time.") - run = await client.get(run["url"]) - run.raise_for_status() - run = run.json() + if run["conclusion"] != "success": + # The action failed, or did not run + raise ActionFailedError(f"The requested workflow ended with: {run['conclusion']}") - raise RunTimeoutError("The requested workflow was not ready in time.") + # The requested action is ready + return run["artifacts_url"] -@async_to_sync -async def get_artifact( - owner: str, repo: str, sha: str, action_name: str, artifact_name: str -) -> str: +def get_artifact(owner: str, repo: str, sha: str, action_name: str, artifact_name: str) -> str: """Get a download URL for a build artifact.""" - client = await authorize(owner, repo) + client = authorize(owner, repo) try: # Get the workflow runs for this repository - runs = await client.get(f"/repos/{owner}/{repo}/actions/runs", params={"per_page": 100}) + runs = client.get(f"/repos/{owner}/{repo}/actions/runs", params={"per_page": 100}) runs.raise_for_status() runs = runs.json() @@ -161,16 +162,16 @@ async def get_artifact( "Could not find a run matching the provided settings in the previous hundred runs." ) - # Wait for the workflow to finish - url = await wait_for_run(client, run) + # Check the workflow status + url = check_run_status(run) # Filter the artifacts, and return the download URL - artifacts = await client.get(url) + artifacts = client.get(url) artifacts.raise_for_status() for artifact in artifacts.json()["artifacts"]: if artifact["name"] == artifact_name: - data = await client.get(artifact["archive_download_url"]) + data = client.get(artifact["archive_download_url"]) if data.status_code == 302: return str(data.next_request.url) @@ -180,4 +181,4 @@ async def get_artifact( raise NotFoundError("Could not find an artifact matching the provided name.") finally: - await client.aclose() + client.close() diff --git a/pydis_site/apps/api/tests/test_github_utils.py b/pydis_site/apps/api/tests/test_github_utils.py index dc17d609..78f2979d 100644 --- a/pydis_site/apps/api/tests/test_github_utils.py +++ b/pydis_site/apps/api/tests/test_github_utils.py @@ -1,6 +1,4 @@ -import asyncio import datetime -import random import unittest from unittest import mock @@ -14,16 +12,6 @@ from django.urls import reverse from .. import github_utils -def patched_raise_for_status(response: httpx.Response): - """Fake implementation of raise_for_status which does not need a request to be set.""" - if response.status_code // 100 != 2: # pragma: no cover - raise httpx.HTTPStatusError( - f"Non 2xx response code: {response.status_code}", - request=getattr(response, "_request", httpx.Request("GET", "")), - response=response - ) - - class GeneralUtilityTests(unittest.TestCase): """Test the utility methods which do not fit in another class.""" @@ -51,53 +39,50 @@ class GeneralUtilityTests(unittest.TestCase): self.assertLess(decoded["exp"], (datetime.datetime.now() + delta).timestamp()) -@mock.patch("httpx.AsyncClient", autospec=True) -@mock.patch("asyncio.sleep", new=mock.AsyncMock(return_value=asyncio.Future)) -@mock.patch("httpx.Response.raise_for_status", new=patched_raise_for_status) -class WaitForTests(unittest.IsolatedAsyncioTestCase): - """Tests the wait_for utility.""" - - async def test_wait_for_successful_run(self, client_mock: mock.Mock): - """Test that the wait_for method handles successfully runs.""" - final_url = "some_url" + str(random.randint(0, 10)) - - client_mock.get.side_effect = responses = [ - httpx.Response(200, json={"status": "queued", "url": ""}), - httpx.Response(200, json={"status": "pending", "url": ""}), - httpx.Response(200, json={ - "status": "completed", - "conclusion": "success", - "url": "", - "artifacts_url": final_url - }) - ] +class WaitForTests(unittest.TestCase): + """Tests the check_run_status utility.""" - result = await github_utils.wait_for_run(client_mock, responses[0].json()) - self.assertEqual(final_url, result) + def test_completed_run(self): + final_url = "some_url_string_1234" - async def test_wait_for_failed_run(self, client_mock: mock.Mock): - """Test that the wait_for method handles failed runs.""" - client_mock.get.return_value = httpx.Response(200, json={ + result = github_utils.check_run_status({ "status": "completed", - "conclusion": "failed", + "conclusion": "success", + "created_at": datetime.datetime.now().strftime(github_utils.ISO_FORMAT_STRING), + "artifacts_url": final_url, }) + self.assertEqual(final_url, result) - with self.assertRaises(github_utils.ActionFailedError): - await github_utils.wait_for_run(client_mock, {"status": "pending", "url": ""}) + def test_pending_run(self): + """Test that a pending run raises the proper exception.""" + with self.assertRaises(github_utils.RunPendingError): + github_utils.check_run_status({ + "status": "pending", + "created_at": datetime.datetime.now().strftime(github_utils.ISO_FORMAT_STRING), + }) - async def test_wait_for_timeout(self, client_mock: mock.Mock): - """Test that the wait_for method quits after a few attempts.""" - client_mock.get.side_effect = responses = [ - httpx.Response(200, json={"status": "pending", "url": ""}) - ] * (github_utils.MAX_POLLS + 5) + def test_timeout_error(self): + """Test that a timeout is declared after a certain duration.""" + # Set the creation time to well before the MAX_RUN_TIME + # to guarantee the right conclusion + created = ( + datetime.datetime.now() - github_utils.MAX_RUN_TIME - datetime.timedelta(minutes=10) + ).strftime(github_utils.ISO_FORMAT_STRING) with self.assertRaises(github_utils.RunTimeoutError): - await github_utils.wait_for_run(client_mock, responses[0].json()) + github_utils.check_run_status({"status": "pending", "created_at": created}) + + def test_failed_run(self): + """Test that a failed run raises the proper exception.""" + with self.assertRaises(github_utils.ActionFailedError): + github_utils.check_run_status({ + "status": "completed", + "conclusion": "failed", + "created_at": datetime.datetime.now().strftime(github_utils.ISO_FORMAT_STRING), + }) -async def get_response_authorize( - _: httpx.AsyncClient, request: httpx.Request, **__ -) -> httpx.Response: +def get_response_authorize(_: httpx.Client, request: httpx.Request, **__) -> httpx.Response: """ Helper method for the authorize tests. @@ -141,76 +126,83 @@ async def get_response_authorize( return httpx.Response(500, request=request) # pragma: no cover -@mock.patch("httpx.AsyncClient.send", new=get_response_authorize) +@mock.patch("httpx.Client.send", new=get_response_authorize) @mock.patch.object(github_utils, "generate_token", new=mock.Mock(return_value="JWT initial token")) -class AuthorizeTests(unittest.IsolatedAsyncioTestCase): +class AuthorizeTests(unittest.TestCase): """Test the authorize utility.""" - async def test_invalid_apps_auth(self): + def test_invalid_apps_auth(self): """Test that an exception is raised if authorization was attempted with an invalid token.""" with mock.patch.object(github_utils, "generate_token", return_value="Invalid token"): with self.assertRaises(httpx.HTTPStatusError) as error: - await github_utils.authorize("VALID_OWNER", "VALID_REPO") + github_utils.authorize("VALID_OWNER", "VALID_REPO") exception: httpx.HTTPStatusError = error.exception self.assertEqual(401, exception.response.status_code) self.assertEqual("auth app/installations", exception.response.json()["error"]) - async def test_missing_repo(self): + def test_missing_repo(self): """Test that an exception is raised when the selected owner or repo are not available.""" with self.assertRaises(github_utils.NotFoundError): - await github_utils.authorize("INVALID_OWNER", "VALID_REPO") + github_utils.authorize("INVALID_OWNER", "VALID_REPO") with self.assertRaises(github_utils.NotFoundError): - await github_utils.authorize("VALID_OWNER", "INVALID_REPO") + github_utils.authorize("VALID_OWNER", "INVALID_REPO") - async def test_valid_authorization(self): + def test_valid_authorization(self): """Test that an accessible repository can be accessed.""" - client = await github_utils.authorize("VALID_OWNER", "VALID_REPO") + client = github_utils.authorize("VALID_OWNER", "VALID_REPO") self.assertEqual("bearer app access token", client.headers.get("Authorization")) -async def get_response_get_artifact(request: httpx.Request, **_) -> httpx.Response: - """ - Helper method for the get_artifact tests. +class ArtifactFetcherTests(unittest.TestCase): + """Test the get_artifact utility.""" - Requests are intercepted before being sent out, and the appropriate responses are returned. - """ - path = request.url.path + @staticmethod + def get_response_get_artifact(request: httpx.Request, **_) -> httpx.Response: + """ + Helper method for the get_artifact tests. - if "force_error" in path: - return httpx.Response(404, request=request) + Requests are intercepted before being sent out, and the appropriate responses are returned. + """ + path = request.url.path - if request.method == "GET": - if path == "/repos/owner/repo/actions/runs": - return httpx.Response(200, request=request, json={"workflow_runs": [{ - "name": "action_name", - "head_sha": "action_sha" - }]}) - elif path == "/artifact_url": - return httpx.Response(200, request=request, json={"artifacts": [{ - "name": "artifact_name", - "archive_download_url": "artifact_download_url" - }]}) - elif path == "/artifact_download_url": - response = httpx.Response(302, request=request) - response.next_request = httpx.Request("GET", httpx.URL("https://final_download.url")) - return response - - # Reaching this point means something has gone wrong - return httpx.Response(500, request=request) # pragma: no cover + if "force_error" in path: + return httpx.Response(404, request=request) + if request.method == "GET": + if path == "/repos/owner/repo/actions/runs": + return httpx.Response( + 200, request=request, json={"workflow_runs": [{ + "name": "action_name", + "head_sha": "action_sha" + }]} + ) + elif path == "/artifact_url": + return httpx.Response( + 200, request=request, json={"artifacts": [{ + "name": "artifact_name", + "archive_download_url": "artifact_download_url" + }]} + ) + elif path == "/artifact_download_url": + response = httpx.Response(302, request=request) + response.next_request = httpx.Request( + "GET", + httpx.URL("https://final_download.url") + ) + return response -class ArtifactFetcherTests(unittest.IsolatedAsyncioTestCase): - """Test the get_artifact utility.""" + # Reaching this point means something has gone wrong + return httpx.Response(500, request=request) # pragma: no cover def setUp(self) -> None: self.call_args = ["owner", "repo", "action_sha", "action_name", "artifact_name"] - self.client = httpx.AsyncClient(base_url="https://example.com") + self.client = httpx.Client(base_url="https://example.com") self.patchers = [ - mock.patch.object(self.client, "send", new=get_response_get_artifact), + mock.patch.object(self.client, "send", new=self.get_response_get_artifact), mock.patch.object(github_utils, "authorize", return_value=self.client), - mock.patch.object(github_utils, "wait_for_run", return_value="artifact_url"), + mock.patch.object(github_utils, "check_run_status", return_value="artifact_url"), ] for patcher in self.patchers: @@ -266,7 +258,7 @@ class GitHubArtifactViewTests(django.test.TestCase): } cls.url = reverse("api:github-artifacts", kwargs=cls.kwargs) - async def test_successful(self, artifact_mock: mock.Mock): + def test_successful(self, artifact_mock: mock.Mock): """Test a proper response is returned with proper input.""" artifact_mock.return_value = "final download url" result = self.client.get(self.url) @@ -274,7 +266,7 @@ class GitHubArtifactViewTests(django.test.TestCase): self.assertIsInstance(result, rest_framework.response.Response) self.assertEqual({"url": artifact_mock.return_value}, result.data) - async def test_failed_fetch(self, artifact_mock: mock.Mock): + def test_failed_fetch(self, artifact_mock: mock.Mock): """Test that a proper error is returned when the request fails.""" artifact_mock.side_effect = github_utils.NotFoundError("Test error message") result = self.client.get(self.url) diff --git a/static-builds/netlify_build.py b/static-builds/netlify_build.py index 13cd0279..a473bd91 100644 --- a/static-builds/netlify_build.py +++ b/static-builds/netlify_build.py @@ -8,6 +8,7 @@ import json import os +import time import zipfile from pathlib import Path from urllib import parse @@ -29,7 +30,7 @@ if __name__ == "__main__": print(f"Fetching download URL from {download_url}") response = httpx.get(download_url, follow_redirects=True) - if response.status_code != 200: + if response.status_code // 100 != 2: try: print(response.json()) except json.JSONDecodeError: @@ -37,6 +38,12 @@ if __name__ == "__main__": response.raise_for_status() + # The workflow is still pending, retry in a bit + while response.status_code == 202: + print(f"{response.json()['error']}. Retrying in 10 seconds.") + time.sleep(10) + response = httpx.get(download_url, follow_redirects=True) + url = response.json()["url"] print(f"Downloading build from {url}") zipped_content = httpx.get(url, follow_redirects=True) -- cgit v1.2.3 From 124c84b9e4f8195485b6ff9b3896cc87e640e02b Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Thu, 14 Jul 2022 05:57:05 +0400 Subject: Clean Up Artifact Tests Signed-off-by: Hassan Abouelela --- pydis_site/apps/api/tests/test_github_utils.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/tests/test_github_utils.py b/pydis_site/apps/api/tests/test_github_utils.py index 78f2979d..a9eab9a5 100644 --- a/pydis_site/apps/api/tests/test_github_utils.py +++ b/pydis_site/apps/api/tests/test_github_utils.py @@ -21,7 +21,7 @@ class GeneralUtilityTests(unittest.TestCase): """ Intercept the encode method. - It is performed with an algorithm which does not require a PEM key, as it may + The result is encoded with an algorithm which does not require a PEM key, as it may not be available in testing environments. """ self.assertEqual("RS256", algorithm, "The GitHub App JWT must be signed using RS256.") @@ -39,10 +39,11 @@ class GeneralUtilityTests(unittest.TestCase): self.assertLess(decoded["exp"], (datetime.datetime.now() + delta).timestamp()) -class WaitForTests(unittest.TestCase): +class CheckRunTests(unittest.TestCase): """Tests the check_run_status utility.""" def test_completed_run(self): + """Test that an already completed run returns the correct URL.""" final_url = "some_url_string_1234" result = github_utils.check_run_status({ @@ -245,20 +246,17 @@ class ArtifactFetcherTests(unittest.TestCase): class GitHubArtifactViewTests(django.test.TestCase): """Test the GitHub artifact fetch API view.""" - @classmethod - def setUpClass(cls): - super().setUpClass() - - cls.kwargs = { + def setUp(self): + self.kwargs = { "owner": "test_owner", "repo": "test_repo", "sha": "test_sha", "action_name": "test_action", "artifact_name": "test_artifact", } - cls.url = reverse("api:github-artifacts", kwargs=cls.kwargs) + self.url = reverse("api:github-artifacts", kwargs=self.kwargs) - def test_successful(self, artifact_mock: mock.Mock): + def test_correct_artifact(self, artifact_mock: mock.Mock): """Test a proper response is returned with proper input.""" artifact_mock.return_value = "final download url" result = self.client.get(self.url) -- cgit v1.2.3 From 37001bca59c1d3d5fc8a8dadffda00d55fc9e0b6 Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sun, 24 Jul 2022 08:32:43 +0200 Subject: Use Dataclass For Workflow Run Signed-off-by: Hassan Abouelela --- pydis_site/apps/api/github_utils.py | 41 ++++++++++++++++----- pydis_site/apps/api/tests/test_github_utils.py | 50 +++++++++++++++----------- 2 files changed, 62 insertions(+), 29 deletions(-) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/github_utils.py b/pydis_site/apps/api/github_utils.py index 707b36e5..c4ace6b7 100644 --- a/pydis_site/apps/api/github_utils.py +++ b/pydis_site/apps/api/github_utils.py @@ -1,7 +1,8 @@ """Utilities for working with the GitHub API.""" - +import dataclasses import datetime import math +import typing import httpx import jwt @@ -50,6 +51,29 @@ class RunPendingError(ArtifactProcessingError): status = 202 +@dataclasses.dataclass(frozen=True) +class WorkflowRun: + """ + A workflow run from the GitHub API. + + https://docs.github.com/en/rest/actions/workflow-runs#get-a-workflow-run + """ + + name: str + head_sha: str + created_at: str + status: str + conclusion: str + artifacts_url: str + + @classmethod + def from_raw(cls, data: dict[str, typing.Any]): + """Create an instance using the raw data from the API, discarding unused fields.""" + return cls(**{ + key.name: data[key.name] for key in dataclasses.fields(cls) + }) + + def generate_token() -> str: """ Generate a JWT token to access the GitHub API. @@ -121,12 +145,12 @@ def authorize(owner: str, repo: str) -> httpx.Client: raise e -def check_run_status(run: dict) -> str: +def check_run_status(run: WorkflowRun) -> str: """Check if the provided run has been completed, otherwise raise an exception.""" - created_at = datetime.datetime.strptime(run["created_at"], ISO_FORMAT_STRING) + created_at = datetime.datetime.strptime(run.created_at, ISO_FORMAT_STRING) run_time = datetime.datetime.now() - created_at - if run["status"] != "completed": + if run.status != "completed": if run_time <= MAX_RUN_TIME: raise RunPendingError( f"The requested run is still pending. It was created " @@ -135,12 +159,12 @@ def check_run_status(run: dict) -> str: else: raise RunTimeoutError("The requested workflow was not ready in time.") - if run["conclusion"] != "success": + if run.conclusion != "success": # The action failed, or did not run - raise ActionFailedError(f"The requested workflow ended with: {run['conclusion']}") + raise ActionFailedError(f"The requested workflow ended with: {run.conclusion}") # The requested action is ready - return run["artifacts_url"] + return run.artifacts_url def get_artifact(owner: str, repo: str, sha: str, action_name: str, artifact_name: str) -> str: @@ -155,7 +179,8 @@ def get_artifact(owner: str, repo: str, sha: str, action_name: str, artifact_nam # Filter the runs for the one associated with the given SHA for run in runs["workflow_runs"]: - if run["name"] == action_name and sha == run["head_sha"]: + run = WorkflowRun.from_raw(run) + if run.name == action_name and sha == run.head_sha: break else: raise NotFoundError( diff --git a/pydis_site/apps/api/tests/test_github_utils.py b/pydis_site/apps/api/tests/test_github_utils.py index a9eab9a5..f5e072a9 100644 --- a/pydis_site/apps/api/tests/test_github_utils.py +++ b/pydis_site/apps/api/tests/test_github_utils.py @@ -1,4 +1,6 @@ +import dataclasses import datetime +import typing import unittest from unittest import mock @@ -42,45 +44,46 @@ class GeneralUtilityTests(unittest.TestCase): class CheckRunTests(unittest.TestCase): """Tests the check_run_status utility.""" + run_kwargs: typing.Mapping = { + "name": "run_name", + "head_sha": "sha", + "status": "completed", + "conclusion": "success", + "created_at": datetime.datetime.now().strftime(github_utils.ISO_FORMAT_STRING), + "artifacts_url": "url", + } + def test_completed_run(self): """Test that an already completed run returns the correct URL.""" final_url = "some_url_string_1234" - result = github_utils.check_run_status({ - "status": "completed", - "conclusion": "success", - "created_at": datetime.datetime.now().strftime(github_utils.ISO_FORMAT_STRING), - "artifacts_url": final_url, - }) + kwargs = dict(self.run_kwargs, artifacts_url=final_url) + result = github_utils.check_run_status(github_utils.WorkflowRun(**kwargs)) self.assertEqual(final_url, result) def test_pending_run(self): """Test that a pending run raises the proper exception.""" + kwargs = dict(self.run_kwargs, status="pending") with self.assertRaises(github_utils.RunPendingError): - github_utils.check_run_status({ - "status": "pending", - "created_at": datetime.datetime.now().strftime(github_utils.ISO_FORMAT_STRING), - }) + github_utils.check_run_status(github_utils.WorkflowRun(**kwargs)) def test_timeout_error(self): """Test that a timeout is declared after a certain duration.""" + kwargs = dict(self.run_kwargs, status="pending") # Set the creation time to well before the MAX_RUN_TIME # to guarantee the right conclusion - created = ( + kwargs["created_at"] = ( datetime.datetime.now() - github_utils.MAX_RUN_TIME - datetime.timedelta(minutes=10) ).strftime(github_utils.ISO_FORMAT_STRING) with self.assertRaises(github_utils.RunTimeoutError): - github_utils.check_run_status({"status": "pending", "created_at": created}) + github_utils.check_run_status(github_utils.WorkflowRun(**kwargs)) def test_failed_run(self): """Test that a failed run raises the proper exception.""" + kwargs = dict(self.run_kwargs, conclusion="failed") with self.assertRaises(github_utils.ActionFailedError): - github_utils.check_run_status({ - "status": "completed", - "conclusion": "failed", - "created_at": datetime.datetime.now().strftime(github_utils.ISO_FORMAT_STRING), - }) + github_utils.check_run_status(github_utils.WorkflowRun(**kwargs)) def get_response_authorize(_: httpx.Client, request: httpx.Request, **__) -> httpx.Response: @@ -172,11 +175,16 @@ class ArtifactFetcherTests(unittest.TestCase): if request.method == "GET": if path == "/repos/owner/repo/actions/runs": + run = github_utils.WorkflowRun( + name="action_name", + head_sha="action_sha", + created_at=datetime.datetime.now().strftime(github_utils.ISO_FORMAT_STRING), + status="completed", + conclusion="success", + artifacts_url="artifacts_url" + ) return httpx.Response( - 200, request=request, json={"workflow_runs": [{ - "name": "action_name", - "head_sha": "action_sha" - }]} + 200, request=request, json={"workflow_runs": [dataclasses.asdict(run)]} ) elif path == "/artifact_url": return httpx.Response( -- cgit v1.2.3 From f16d3b1b1d14cdf0de1e56ae2bc466152e930f34 Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sun, 24 Jul 2022 10:06:47 +0200 Subject: Use UTC Time For GitHub API When reading the created_at time from the GitHub API, it'll be a naive date string with UTC time, so we use that instead of the system's time. Signed-off-by: Hassan Abouelela --- pydis_site/apps/api/github_utils.py | 2 +- pydis_site/apps/api/tests/test_github_utils.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/github_utils.py b/pydis_site/apps/api/github_utils.py index c4ace6b7..7d26b147 100644 --- a/pydis_site/apps/api/github_utils.py +++ b/pydis_site/apps/api/github_utils.py @@ -148,7 +148,7 @@ def authorize(owner: str, repo: str) -> httpx.Client: def check_run_status(run: WorkflowRun) -> str: """Check if the provided run has been completed, otherwise raise an exception.""" created_at = datetime.datetime.strptime(run.created_at, ISO_FORMAT_STRING) - run_time = datetime.datetime.now() - created_at + run_time = datetime.datetime.utcnow() - created_at if run.status != "completed": if run_time <= MAX_RUN_TIME: diff --git a/pydis_site/apps/api/tests/test_github_utils.py b/pydis_site/apps/api/tests/test_github_utils.py index f5e072a9..f642f689 100644 --- a/pydis_site/apps/api/tests/test_github_utils.py +++ b/pydis_site/apps/api/tests/test_github_utils.py @@ -49,7 +49,7 @@ class CheckRunTests(unittest.TestCase): "head_sha": "sha", "status": "completed", "conclusion": "success", - "created_at": datetime.datetime.now().strftime(github_utils.ISO_FORMAT_STRING), + "created_at": datetime.datetime.utcnow().strftime(github_utils.ISO_FORMAT_STRING), "artifacts_url": "url", } @@ -73,7 +73,7 @@ class CheckRunTests(unittest.TestCase): # Set the creation time to well before the MAX_RUN_TIME # to guarantee the right conclusion kwargs["created_at"] = ( - datetime.datetime.now() - github_utils.MAX_RUN_TIME - datetime.timedelta(minutes=10) + datetime.datetime.utcnow() - github_utils.MAX_RUN_TIME - datetime.timedelta(minutes=10) ).strftime(github_utils.ISO_FORMAT_STRING) with self.assertRaises(github_utils.RunTimeoutError): -- cgit v1.2.3 From 25db8f564c0f5c473b165ccab14413ca4471ac7d Mon Sep 17 00:00:00 2001 From: Hassan Abouelela Date: Sun, 14 Aug 2022 07:20:34 +0200 Subject: Explicitly Specify Infraction Time In Tests The infraction tests checked that the route returned infractions in the correct order, which is based on insertion time. This can be fragile however, since the insertion time can be very close (or identical) during the tests. That became especially more likely with PR #741 (commit 149e67b4) which improved database access speed. This is fixed by explicitly specifying the insertion time, and spacing them out properly. Signed-off-by: Hassan Abouelela --- pydis_site/apps/api/tests/test_infractions.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) (limited to 'pydis_site/apps/api/tests') diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index f1107734..89ee4e23 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -56,15 +56,17 @@ class InfractionTests(AuthenticatedAPITestCase): type='ban', reason='He terk my jerb!', hidden=True, + inserted_at=dt(2020, 10, 10, 0, 0, 0, tzinfo=timezone.utc), expires_at=dt(5018, 11, 20, 15, 52, tzinfo=timezone.utc), - active=True + active=True, ) cls.ban_inactive = Infraction.objects.create( user_id=cls.user.id, actor_id=cls.user.id, type='ban', reason='James is an ass, and we won\'t be working with him again.', - active=False + active=False, + inserted_at=dt(2020, 10, 10, 0, 1, 0, tzinfo=timezone.utc), ) cls.mute_permanent = Infraction.objects.create( user_id=cls.user.id, @@ -72,7 +74,8 @@ class InfractionTests(AuthenticatedAPITestCase): type='mute', reason='He has a filthy mouth and I am his soap.', active=True, - expires_at=None + inserted_at=dt(2020, 10, 10, 0, 2, 0, tzinfo=timezone.utc), + expires_at=None, ) cls.superstar_expires_soon = Infraction.objects.create( user_id=cls.user.id, @@ -80,7 +83,8 @@ class InfractionTests(AuthenticatedAPITestCase): type='superstar', reason='This one doesn\'t matter anymore.', active=True, - expires_at=dt.now(timezone.utc) + datetime.timedelta(hours=5) + inserted_at=dt(2020, 10, 10, 0, 3, 0, tzinfo=timezone.utc), + expires_at=dt.now(timezone.utc) + datetime.timedelta(hours=5), ) cls.voiceban_expires_later = Infraction.objects.create( user_id=cls.user.id, @@ -88,7 +92,8 @@ class InfractionTests(AuthenticatedAPITestCase): type='voice_ban', reason='Jet engine mic', active=True, - expires_at=dt.now(timezone.utc) + datetime.timedelta(days=5) + inserted_at=dt(2020, 10, 10, 0, 4, 0, tzinfo=timezone.utc), + expires_at=dt.now(timezone.utc) + datetime.timedelta(days=5), ) def test_list_all(self): -- cgit v1.2.3