diff --git a/src-docs/app.py.md b/src-docs/app.py.md index a04153a..1e88dee 100644 --- a/src-docs/app.py.md +++ b/src-docs/app.py.md @@ -32,7 +32,7 @@ Configure the application. --- - + ## function `health_check` @@ -50,7 +50,7 @@ Health check endpoint. --- - + ## function `handle_github_webhook` diff --git a/src-docs/parse.py.md b/src-docs/parse.py.md index ce6f3a3..551865f 100644 --- a/src-docs/parse.py.md +++ b/src-docs/parse.py.md @@ -11,12 +11,12 @@ Module for parsing the webhook payload. --- - + ## function `webhook_to_job` ```python -webhook_to_job(webhook: dict) → Job +webhook_to_job(payload: dict, ignore_labels: Collection[str]) → Job ``` Parse a raw json payload and extract the required information. @@ -25,7 +25,8 @@ Parse a raw json payload and extract the required information. **Args:** - - `webhook`: The webhook in json to parse. + - `payload`: The webhook's payload in json to parse. + - `ignore_labels`: The labels to ignore when parsing. For example, "self-hosted" or "linux". diff --git a/src-docs/router.py.md b/src-docs/router.py.md index cea962b..75ee279 100644 --- a/src-docs/router.py.md +++ b/src-docs/router.py.md @@ -11,14 +11,13 @@ Module for routing webhooks to the appropriate message queue. --- - + ## function `to_routing_table` ```python to_routing_table( flavor_label_mapping_list: list[tuple[str, list[str]]], - ignore_labels: set[str], default_flavor: str ) → RoutingTable ``` @@ -30,7 +29,6 @@ Convert the flavor label mapping to a route table. **Args:** - `flavor_label_mapping_list`: The list of mappings of flavors to labels. - - `ignore_labels`: The labels to ignore (e.g. "self-hosted" or "linux"). - `default_flavor`: The default flavor to use if no labels are provided. @@ -41,7 +39,7 @@ Convert the flavor label mapping to a route table. --- - + ## function `forward` @@ -67,7 +65,7 @@ Forward the job to the appropriate message queue. --- - + ## function `can_forward` @@ -102,7 +100,6 @@ A class to represent how to route jobs to the appropriate message queue. **Attributes:** - `value`: The mapping of labels to flavors. - - `ignore_labels`: The labels to ignore (e.g. "self-hosted" or "linux"). - `default_flavor`: The default flavor. diff --git a/tests/integration/test_app.py b/tests/integration/test_app.py index d42a4ee..5587f9c 100644 --- a/tests/integration/test_app.py +++ b/tests/integration/test_app.py @@ -100,14 +100,14 @@ async def test_forward_webhook( # pylint: disable=too-many-locals } for flavour in flavours: - actual_jobs = jobs_by_flavour.get(flavour, set()) + actual_jobs = jobs_by_flavour.get(flavour, []) expected_jobs_for_flavour = expected_jobs_by_flavour[flavour] assert len(actual_jobs) == len( expected_jobs_for_flavour ), f"Expected: {expected_jobs_for_flavour}, Actual: {actual_jobs}" for job in expected_jobs_for_flavour: assert ( - job.json() in actual_jobs + job in actual_jobs ), f"Expected: {expected_jobs_for_flavour}, Actual: {actual_jobs}" @@ -293,7 +293,9 @@ def _request(payload: dict, webhook_secret: Optional[str], base_url: str) -> req return requests.post(f"{base_url}/webhook", data=payload_bytes, headers=headers, timeout=1) -async def _get_jobs_from_mq(ops_test: OpsTest, unit: Unit, flavors: list[str]) -> dict[str, set]: +async def _get_jobs_from_mq( + ops_test: OpsTest, unit: Unit, flavors: list[str] +) -> dict[str, list[Job]]: """Get the gunicorn log from the charm unit. Args: @@ -349,7 +351,11 @@ async def _get_jobs_from_mq(ops_test: OpsTest, unit: Unit, flavors: list[str]) - "ssh", "--container", "flask-app", unit.name, "python3", "/kombu_script.py" ) assert code == 0, f"Failed to execute kombu script: {stderr}" - return json.loads(stdout) + jobs_raw_by_flavor = json.loads(stdout) + jobs_by_flavor = {} + for flavor, jobs_raw in jobs_raw_by_flavor.items(): + jobs_by_flavor[flavor] = [Job.model_validate_json(job_raw) for job_raw in jobs_raw] + return jobs_by_flavor async def _get_mongodb_uri_from_integration_data(ops_test: OpsTest, unit: Unit) -> str | None: diff --git a/tests/unit/test_app.py b/tests/unit/test_app.py index 084cf48..2dfdf4f 100644 --- a/tests/unit/test_app.py +++ b/tests/unit/test_app.py @@ -20,6 +20,8 @@ from webhook_router.router import RouterError, RoutingTable TEST_PATH = "/webhook" +TEST_LABELS = ["self-hosted", "linux", "arm64"] +DEFAULT_SELF_HOSTED_LABELS = {"self-hosted", "linux"} @pytest.fixture(name="flavours_yaml") @@ -55,7 +57,6 @@ def route_table_fixture() -> RoutingTable: ("small", "x64"): "small", }, default_flavor="small", - ignore_labels={"self-hosted", "linux"}, ) @@ -109,7 +110,7 @@ def test_webhook_logs( """ data = _create_valid_data(JobStatus.QUEUED) expected_job = Job( - labels=data["workflow_job"]["labels"], + labels=set(TEST_LABELS) - DEFAULT_SELF_HOSTED_LABELS, status=JobStatus.QUEUED, url=data["workflow_job"]["url"], ) @@ -392,7 +393,7 @@ def _create_valid_data(action: str) -> dict: "run_id": 987654321, "status": "completed", "conclusion": "success", - "labels": ["self-hosted", "linux", "arm64"], + "labels": TEST_LABELS, "url": "https://api.github.com/repos/f/actions/jobs/8200803099", }, } diff --git a/tests/unit/test_parse.py b/tests/unit/test_parse.py index cc3d9a1..cef670a 100644 --- a/tests/unit/test_parse.py +++ b/tests/unit/test_parse.py @@ -2,13 +2,15 @@ # See LICENSE file for licensing details. """Unit tests for the parse module.""" +from itertools import permutations import pytest -from webhook_router.parse import Job, JobStatus, ParseError, webhook_to_job +from webhook_router.parse import Job, JobStatus, Labels, ParseError, webhook_to_job FAKE_JOB_URL = "https://api.github.com/repos/fakeusergh-runner-test/actions/jobs/8200803099" FAKE_LABELS = ["self-hosted", "linux", "arm64"] +DEFAULT_SELF_HOSTED_LABELS = {"self-hosted", "linux"} @pytest.mark.parametrize( @@ -56,11 +58,15 @@ def test_webhook_to_job(labels: list[str], status: JobStatus): }, } - result = webhook_to_job(payload) + result = webhook_to_job(payload, DEFAULT_SELF_HOSTED_LABELS) # mypy does not understand that we can pass strings instead of HttpUrl objects # because of the underlying pydantic magic - assert result == Job(labels=labels, status=status, url=FAKE_JOB_URL) # type: ignore + assert result == Job( + labels=set(labels) - DEFAULT_SELF_HOSTED_LABELS, + status=status, + url=FAKE_JOB_URL, # type: ignore + ) @pytest.mark.parametrize( @@ -88,7 +94,7 @@ def test_webhook_invalid_values(labels: list[str], status: JobStatus, url: str): "workflow_job": {"id": 22428484402, "url": url, "labels": labels}, } with pytest.raises(ParseError) as exc_info: - webhook_to_job(payload) + webhook_to_job(payload, DEFAULT_SELF_HOSTED_LABELS) assert "Failed to create Webhook object for webhook " in str(exc_info.value) @@ -103,7 +109,7 @@ def test_webhook_workflow_job_not_dict(): "workflow_job": "not a dict", } with pytest.raises(ParseError) as exc_info: - webhook_to_job(payload) + webhook_to_job(payload, DEFAULT_SELF_HOSTED_LABELS) assert f"workflow_job is not a dict in {payload}" in str(exc_info.value) @@ -122,7 +128,7 @@ def test_webhook_missing_keys(): }, } with pytest.raises(ParseError) as exc_info: - webhook_to_job(payload) + webhook_to_job(payload, DEFAULT_SELF_HOSTED_LABELS) assert f"action key not found in {payload}" in str(exc_info.value) # workflow_job key missing payload = { @@ -132,7 +138,7 @@ def test_webhook_missing_keys(): "labels": FAKE_LABELS, } with pytest.raises(ParseError) as exc_info: - webhook_to_job(payload) + webhook_to_job(payload, DEFAULT_SELF_HOSTED_LABELS) assert f"workflow_job key not found in {payload}" in str(exc_info.value) # labels key missing @@ -141,7 +147,7 @@ def test_webhook_missing_keys(): "workflow_job": {"id": 22428484402, "url": FAKE_JOB_URL}, } with pytest.raises(ParseError) as exc_info: - webhook_to_job(payload) + webhook_to_job(payload, DEFAULT_SELF_HOSTED_LABELS) assert f"labels key not found in {payload}" in str(exc_info.value) # url key missing @@ -150,5 +156,62 @@ def test_webhook_missing_keys(): "workflow_job": {"id": 22428484402, "labels": FAKE_LABELS}, } with pytest.raises(ParseError) as exc_info: - webhook_to_job(payload) + webhook_to_job(payload, DEFAULT_SELF_HOSTED_LABELS) assert f"url key not found in {payload}" in str(exc_info.value) + + +@pytest.mark.parametrize( + "labels, ignore_labels,expected_labels", + [ + pytest.param( + ["self-hosted", "linux", "arm64"], set(), {"self-hosted", "linux", "arm64"}, id="empty" + ), + pytest.param(["self-hosted", "linux"], {"self-hosted", "linux"}, set(), id="equal"), + pytest.param( + ["self-hosted", "linux"], {"self-hosted", "linux", "arm64"}, set(), id="superset" + ), + pytest.param(["ubuntu-latest"], {"self-hosted"}, {"ubuntu-latest"}, id="non overlapping"), + pytest.param( + ["self-hosted", "linux", "amd"], {"self-hosted", "linux"}, {"amd"}, id="overlapping" + ), + ], +) +def test_ignore_labels(labels: list[str], ignore_labels: Labels, expected_labels: Labels): + """ + arrange: A valid payload dict with different combinations of ignore labels. + act: Call webhook_to_job with the payload. + assert: The ignore labels are removed from the labels. + """ + # check all permutations of labels to check if the ignore labels are removed correctly + # in order to check for bugs around ignoring certain elements + for label_permutation in permutations(labels): + payload = { + "action": JobStatus.QUEUED, + "workflow_job": {"id": 22428484402, "url": FAKE_JOB_URL, "labels": label_permutation}, + } + result = webhook_to_job(payload, ignore_labels) + + assert result.labels == expected_labels + + +def test_labels_are_parsed_in_lowercase(): + """ + arrange: A valid payload dict with labels in mixed_case. + act: Call webhook_to_job with the payload. + assert: The labels are parsed and compared with the ignore labels in lowercase. + """ + labels = ["Self-Hosted", "Linux", "ARM64"] + ignore_labels = {"self-Hosted", "linUX"} + payload = { + "action": JobStatus.QUEUED, + "workflow_job": {"id": 22428484402, "url": FAKE_JOB_URL, "labels": labels}, + } + result = webhook_to_job(payload, ignore_labels) + + # mypy does not understand that we can pass strings instead of HttpUrl objects + # because of the underlying pydantic magic + assert result == Job( + labels={"arm64"}, + status=JobStatus.QUEUED, + url=FAKE_JOB_URL, # type: ignore + ) diff --git a/tests/unit/test_router.py b/tests/unit/test_router.py index f3f1566..f9efb7a 100644 --- a/tests/unit/test_router.py +++ b/tests/unit/test_router.py @@ -50,15 +50,13 @@ def test_job_is_forwarded( # mypy does not understand that we can pass strings instead of HttpUrl objects # because of the underlying pydantic magic job = Job( - labels=["arm64"], + labels={"arm64"}, status=job_status, url="https://api.github.com/repos/f/actions/jobs/8200803099", # type: ignore ) forward( job, - routing_table=RoutingTable( - value={("arm64",): "arm64"}, default_flavor="arm64", ignore_labels=set() - ), + routing_table=RoutingTable(value={("arm64",): "arm64"}, default_flavor="arm64"), ) assert add_job_to_queue_mock.called == is_forwarded @@ -73,14 +71,14 @@ def test_invalid_label_combination(): # mypy does not understand that we can pass strings instead of HttpUrl objects # because of the underlying pydantic magic job = Job( - labels=["self-hosted", "linux", "arm64", "x64"], + labels={"self-hosted", "linux", "arm64", "x64"}, status=JobStatus.QUEUED, url="https://api.github.com/repos/f/actions/jobs/8200803099", # type: ignore ) with pytest.raises(RouterError) as e: forward( job, - routing_table=RoutingTable(value={}, default_flavor="default", ignore_labels=set()), + routing_table=RoutingTable(value={}, default_flavor="default"), ) assert "Not able to forward job: Invalid label combination:" in str(e.value) @@ -93,9 +91,8 @@ def test_to_routing_table(): """ flavor_mapping = [("large", ["arm64", "large"]), ("x64-large", ["large", "x64", "jammy"])] - ignore_labels = {"self-hosted", "linux"} default_flavor = "x64-large" - routing_table = to_routing_table(flavor_mapping, ignore_labels, default_flavor) + routing_table = to_routing_table(flavor_mapping, default_flavor) assert routing_table == RoutingTable( value={ ("arm64",): "large", @@ -109,7 +106,6 @@ def test_to_routing_table(): ("jammy", "large", "x64"): "x64-large", }, default_flavor="x64-large", - ignore_labels=ignore_labels, ) @@ -120,9 +116,8 @@ def test_to_routing_table_case_insensitive(): assert: A LabelsFlavorMapping object is returned which has all labels in lower case. """ flavor_mapping = [("large", ["arM64", "LaRgE"])] - ignore_labels = {"self-HOSTED", "LINux"} default_flavor = "large" - routing_table = to_routing_table(flavor_mapping, ignore_labels, default_flavor) + routing_table = to_routing_table(flavor_mapping, default_flavor) assert routing_table == RoutingTable( value={ ("arm64",): "large", @@ -130,7 +125,6 @@ def test_to_routing_table_case_insensitive(): ("arm64", "large"): "large", }, default_flavor="large", - ignore_labels={"self-hosted", "linux"}, ) @@ -172,7 +166,6 @@ def test__labels_to_flavor(): **{label_combination: "x64-large" for label_combination in x64_label_combination}, }, default_flavor="large", - ignore_labels={"self-hosted", "linux"}, ) for label_combination in arm_label_combination: @@ -200,20 +193,19 @@ def test__labels_to_flavor_case_insensitive(): ("arm64", "large"): "large", }, default_flavor="large", - ignore_labels={"self-hosted", "linux"}, ) assert _labels_to_flavor({"SMALl"}, routing_table) == "small" assert _labels_to_flavor({"ARM64"}, routing_table) == "large" assert _labels_to_flavor({"lARGE"}, routing_table) == "large" assert _labels_to_flavor({"arM64", "lArge"}, routing_table) == "large" - assert _labels_to_flavor({"small", "SELF-hosted"}, routing_table) == "small" + assert _labels_to_flavor({"small"}, routing_table) == "small" -def test__labels_to_flavor_default_label(): +def test__labels_to_flavor_default_flavor(): """ arrange: Two flavors and a labels to flavor routing_table - act: Call labels_to_flavor with empty labels or ignored labels. + act: Call labels_to_flavor with empty labels. assert: The default flavor is returned. """ routing_table = RoutingTable( @@ -225,12 +217,8 @@ def test__labels_to_flavor_default_label(): ("large", "x64"): "x64-large", }, default_flavor="large", - ignore_labels={"self-hosted", "linux"}, ) assert _labels_to_flavor(set(), routing_table) == "large" - assert _labels_to_flavor({"self-hosted"}, routing_table) == "large" - assert _labels_to_flavor({"linux"}, routing_table) == "large" - assert _labels_to_flavor({"self-hosted", "linux"}, routing_table) == "large" def test__labels_to_flavor_invalid_combination(): @@ -248,7 +236,6 @@ def test__labels_to_flavor_invalid_combination(): ("large", "x64"): "x64-large", }, default_flavor="large", - ignore_labels={"self-hosted", "linux"}, ) labels = {"self-hosted", "linux", "arm64", "large", "x64"} with pytest.raises(_InvalidLabelCombinationError) as exc_info: @@ -271,30 +258,8 @@ def test__labels_to_flavor_unrecognised_label(): ("large", "x64"): "x64-large", }, default_flavor="large", - ignore_labels={"self-hosted", "linux"}, ) labels = {"self-hosted", "linux", "arm64", "large", "noble"} with pytest.raises(_InvalidLabelCombinationError) as exc_info: _labels_to_flavor(labels, routing_table) assert "Invalid label combination:" in str(exc_info.value) - - -def test__labels_to_flavor_ignore_labels(): - """ - arrange: Two flavors and a labels to flavor routing_table - act: Call labels_to_flavor with an ignored label. - assert: The correct flavor is returned. - """ - routing_table = RoutingTable( - value={ - ("arm64",): "large", - ("large",): "large", - ("arm64", "large"): "large", - ("x64",): "large-x64", - ("large", "x64"): "x64-large", - }, - default_flavor="large", - ignore_labels={"self-hosted", "linux"}, - ) - labels = {"self-hosted", "linux", "arm64", "large"} - assert _labels_to_flavor(labels, routing_table) == "large" diff --git a/webhook_router/app.py b/webhook_router/app.py index 173570d..6401faa 100644 --- a/webhook_router/app.py +++ b/webhook_router/app.py @@ -49,11 +49,10 @@ def config_app(flask_app: Flask) -> None: default_self_hosted_labels = _parse_default_self_hosted_labels_config( flask_app.config.get("DEFAULT_SELF_HOSTED_LABELS", "") ) + flask_app.config["DEFAULT_SELF_HOSTED_LABELS"] = default_self_hosted_labels flavor_labels_mapping_list = [tuple(item.items())[0] for item in flavors_config.flavor_list] flask_app.config["ROUTING_TABLE"] = to_routing_table( - flavor_label_mapping_list=flavor_labels_mapping_list, - ignore_labels=default_self_hosted_labels, - default_flavor=default_flavor, + flavor_label_mapping_list=flavor_labels_mapping_list, default_flavor=default_flavor ) @@ -241,7 +240,7 @@ def _parse_job() -> Job: """ payload = request.get_json() app.logger.debug("Received payload: %s", payload) - return webhook_to_job(payload) + return webhook_to_job(payload=payload, ignore_labels=app.config["DEFAULT_SELF_HOSTED_LABELS"]) # Exclude from coverage since unit tests should not run as __main__ diff --git a/webhook_router/parse.py b/webhook_router/parse.py index bd6c517..316265d 100644 --- a/webhook_router/parse.py +++ b/webhook_router/parse.py @@ -4,6 +4,7 @@ """Module for parsing the webhook payload.""" from collections import namedtuple from enum import Enum +from typing import Collection from pydantic import BaseModel, HttpUrl @@ -11,6 +12,7 @@ ValidationResult = namedtuple("ValidationResult", ["is_valid", "msg"]) +Labels = set[str] class ParseError(Exception): @@ -42,16 +44,17 @@ class Job(BaseModel): url: The URL of the job to be able to check its status. """ - labels: list[str] + labels: Labels status: JobStatus url: HttpUrl -def webhook_to_job(webhook: dict) -> Job: +def webhook_to_job(payload: dict, ignore_labels: Collection[str]) -> Job: """Parse a raw json payload and extract the required information. Args: - webhook: The webhook in json to parse. + payload: The webhook's payload in json to parse. + ignore_labels: The labels to ignore when parsing. For example, "self-hosted" or "linux". Returns: The parsed Job. @@ -59,35 +62,41 @@ def webhook_to_job(webhook: dict) -> Job: Raises: ParseError: An error occurred during parsing. """ - validation_result = _validate_webhook(webhook) + validation_result = _validate_webhook(payload) if not validation_result.is_valid: raise ParseError(f"Could not parse webhook: {validation_result.msg}") # The enclosed code will be removed when compiling to optimised byte code. - assert "action" in webhook, f"action key not found in {webhook}" # nosec - assert "workflow_job" in webhook, f"workflow_job key not found in {webhook}" # nosec + assert "action" in payload, f"action key not found in {payload}" # nosec + assert "workflow_job" in payload, f"workflow_job key not found in {payload}" # nosec assert ( # nosec - "labels" in webhook["workflow_job"] - ), f"labels key not found in {webhook['workflow_job']}" + "labels" in payload["workflow_job"] + ), f"labels key not found in {payload['workflow_job']}" assert ( # nosec - "url" in webhook["workflow_job"] - ), f"url key not found in {webhook['workflow_job']}" + "url" in payload["workflow_job"] + ), f"url key not found in {payload['workflow_job']}" - status = webhook["action"] - workflow_job = webhook["workflow_job"] + status = payload["action"] + workflow_job = payload["workflow_job"] labels = workflow_job["labels"] + + if labels is None: + raise ParseError( + f"Failed to create Webhook object for webhook {payload}: Labels are missing" + ) + job_url = workflow_job["url"] try: return Job( - labels=labels, + labels=_parse_labels(labels=labels, ignore_labels=ignore_labels), status=status, url=job_url, ) except ValueError as exc: - raise ParseError(f"Failed to create Webhook object for webhook {webhook}: {exc}") from exc + raise ParseError(f"Failed to create Webhook object for webhook {payload}: {exc}") from exc def _validate_webhook(webhook: dict) -> ValidationResult: @@ -131,3 +140,18 @@ def _validate_missing_keys(webhook: dict) -> ValidationResult: False, f"{expected_workflow_job_key} key not found in {webhook}" ) return ValidationResult(True, "") + + +def _parse_labels(labels: Collection[str], ignore_labels: Collection[str]) -> Labels: + """Parse the labels coming from the payload and remove the ignore labels. + + Args: + labels: The labels to parse from the payload. + ignore_labels: The labels to ignore. + + Returns: + The parsed labels in lowercase. + """ + return {label.lower() for label in labels} - { + ignore_label.lower() for ignore_label in ignore_labels + } diff --git a/webhook_router/router.py b/webhook_router/router.py index d45fda9..0f030d5 100644 --- a/webhook_router/router.py +++ b/webhook_router/router.py @@ -26,12 +26,10 @@ class RoutingTable(BaseModel): Attributes: value: The mapping of labels to flavors. - ignore_labels: The labels to ignore (e.g. "self-hosted" or "linux"). default_flavor: The default flavor. """ value: dict[LabelCombinationIdentifier, Label] - ignore_labels: set[Label] default_flavor: Flavor @@ -39,15 +37,12 @@ class RoutingTable(BaseModel): def to_routing_table( - flavor_label_mapping_list: FlavorLabelsMappingList, - ignore_labels: set[Label], - default_flavor: Flavor, + flavor_label_mapping_list: FlavorLabelsMappingList, default_flavor: Flavor ) -> RoutingTable: """Convert the flavor label mapping to a route table. Args: flavor_label_mapping_list: The list of mappings of flavors to labels. - ignore_labels: The labels to ignore (e.g. "self-hosted" or "linux"). default_flavor: The default flavor to use if no labels are provided. Returns: @@ -90,7 +85,6 @@ def to_routing_table( return RoutingTable( default_flavor=default_flavor, value=routing_table, - ignore_labels={label.lower() for label in ignore_labels}, ) @@ -147,11 +141,10 @@ def _labels_to_flavor(labels: set[str], routing_table: RoutingTable) -> Flavor: The flavor. """ labels_lowered = {label.lower() for label in labels} - final_labels = labels_lowered - routing_table.ignore_labels - if not final_labels: + if not labels_lowered: return routing_table.default_flavor - label_key = tuple(sorted(final_labels)) + label_key = tuple(sorted(labels_lowered)) if label_key not in routing_table.value: raise _InvalidLabelCombinationError(f"Invalid label combination: {labels}") return routing_table.value[label_key]