diff --git a/packit_service/worker/helpers/testing_farm.py b/packit_service/worker/helpers/testing_farm.py index a1a3c4ffc..0adf6af4c 100644 --- a/packit_service/worker/helpers/testing_farm.py +++ b/packit_service/worker/helpers/testing_farm.py @@ -1,9 +1,10 @@ # Copyright Contributors to the Packit project. # SPDX-License-Identifier: MIT - +import argparse import logging import re from re import Pattern +import shlex from typing import Dict, Any, Optional, Set, List, Union, Tuple, Callable import requests @@ -60,40 +61,92 @@ class CommentArguments: Parse arguments from trigger comment and provide the attributes to Testing Farm helper. """ - packit_command: str = None - identifier: str = None - labels: List[str] = None - pr_argument: str = None - def __init__(self, command_prefix: str, comment: str): + self._parser: argparse.ArgumentParser = None + self.packit_command: str = None + self.identifier: str = None + self.labels: List[str] = None + self.pr_argument: str = None + self.envs: Dict[str, str] = None + if comment is None: return - # Try to parse identifier argument from comment + # Remove the command prefix from the comment logger.debug(f"Parsing comment -> {comment}") logger.debug(f"Used command prefix -> {command_prefix}") - match = re.search( - r"^" + re.escape(command_prefix) + r"\s(?P\S+)", comment - ) + # Match the command prefix and extract the rest of the comment + match = re.match(r"^" + re.escape(command_prefix) + r"\s+(.*)", comment) if match: - self.packit_command = match.group("packit_command") - logger.debug(f"Parsed packit_command: {self.packit_command}") + arguments_str = match.group(1) + else: + # If the command prefix is not found, nothing to parse + logger.debug("Command prefix not found in the comment.") + return - match = re.search(r"(--identifier|--id|-i)[\s=](?P\S+)", comment) - if match: - self.identifier = match.group("identifier") - logger.debug(f"Parsed test argument -> identifier: {self.identifier}") + # Use shlex to split the arguments string into a list + args_list = shlex.split(arguments_str) + logger.debug(f"Arguments list after shlex splitting: {args_list}") - match = re.search(r"--labels[\s=](?P\S+)", comment) - if match: - self.labels = match.group("labels").split(",") - logger.debug(f"Parsed test argument -> labels: {self.labels}") + # Parse known arguments + try: + args, unknown_args = self.parser.parse_known_args(args_list) + logger.debug(f"Parsed known args: {args}") + logger.debug(f"Unknown args: {unknown_args}") + except argparse.ArgumentError as e: + logger.error(f"Argument parsing error: {e}") + return - match = re.search(r"(?P[^/\s]+/[^#]+#\d+)", comment) - if match: - self.pr_argument = match.group("pr_arg") - logger.debug(f"Parsed test argument -> pr_argument: {self.pr_argument}") + self.parse_known_arguments(args) + self.parse_unknown_arguments(unknown_args) + + @property + def parser(self) -> argparse.ArgumentParser: + if self._parser is None: + # Set up argparse + self._parser = argparse.ArgumentParser() + self._parser.add_argument("packit_command") + self._parser.add_argument("--identifier", "--id", "-i") + self._parser.add_argument("--labels", type=lambda s: s.split(",")) + # Allows multiple --env arguments + self._parser.add_argument("--env", action="append") + + return self._parser + + def parse_known_arguments(self, args: argparse.Namespace) -> None: + # Assign the parsed arguments to the class attributes + self.packit_command = args.packit_command + logger.debug(f"Parsed packit_command: {self.packit_command}") + + self.identifier = args.identifier + logger.debug(f"Parsed identifier: {self.identifier}") + + if args.labels: + self.labels = args.labels + logger.debug(f"Parsed labels: {self.labels}") + + if args.env: + self.envs = {} + for env in args.env: + if "=" in env: + key, value = env.split("=", 1) + self.envs[key] = value + logger.debug(f"Parsed env variable: {key}={value}") + else: + logger.error( + f"Invalid format for '--env' argument: '{env}'. Expected VAR_NAME=value." + ) + continue + + def parse_unknown_arguments(self, unknown_args: List[str]) -> None: + # Process unknown_args to find pr_argument + pr_argument_pattern = re.compile(r"^[^/\s]+/[^#\s]+#\d+$") + for arg in unknown_args: + if pr_argument_pattern.match(arg): + self.pr_argument = arg + logger.debug(f"Parsed pr_argument: {self.pr_argument}") + break class TestingFarmJobHelper(CoprBuildJobHelper): @@ -523,6 +576,17 @@ def _payload( env_variables = self.job_config.env if hasattr(self.job_config, "env") else {} predefined_environment.update(env_variables) + # User-defined variables from comments have priority + if self.is_comment_event and self.comment_arguments.envs is not None: + for k, v in self.comment_arguments.envs.items(): + # Set env variable + logger.debug(f"Key: {k} -> Value: '{v}'") + if v is not None and v != "": + predefined_environment[k] = v + # Unset env variable if it doesn't have value + else: + predefined_environment.pop(k, None) + environment: Dict[str, Any] = { "arch": arch, "os": {"compose": compose}, diff --git a/tests/unit/test_checkers.py b/tests/unit/test_checkers.py index 82d5abd75..05ee62e95 100644 --- a/tests/unit/test_checkers.py +++ b/tests/unit/test_checkers.py @@ -507,27 +507,27 @@ def test_koji_branch_merge_queue(): "comment, result", ( pytest.param( - "/packit-dev test --identifier my-id-1", + "/packit test --identifier my-id-1", True, id="Matching identifier specified", ), pytest.param( - "/packit-dev test --id my-id-1", + "/packit test --id my-id-1", True, id="Matching identifier specified", ), pytest.param( - "/packit-dev test -i my-id-1", + "/packit test -i my-id-1", True, id="Matching identifier specified", ), pytest.param( - "/packit-dev test", + "/packit test", True, id="No identifier specified", ), pytest.param( - "/packit-dev test --identifier my-id-2", + "/packit test --identifier my-id-2", False, id="Non-matching identifier specified", ), @@ -582,35 +582,35 @@ def test_tf_comment_identifier(comment, result): "comment, default_identifier, job_identifier, result", ( pytest.param( - "/packit-dev test --identifier my-id2", + "/packit test --identifier my-id2", "id1", "id1", False, id="Identifier specified in comment", ), pytest.param( - "/packit-dev test", + "/packit test", None, "id1", True, id="No identifier specified, no default identifier", ), pytest.param( - "/packit-dev test", + "/packit test", "id1", "id1", True, id="No identifier specified, default identifier matching", ), pytest.param( - "/packit-dev test", + "/packit test", "id1", "id2", False, id="No identifier specified, default identifier not matching", ), pytest.param( - "/packit-dev test", + "/packit test", "id1", None, False, @@ -670,17 +670,17 @@ def test_tf_comment_default_identifier( "comment, result", ( pytest.param( - "/packit-dev test --labels label1,label2", + "/packit test --labels label1,label2", True, id="Matching label specified", ), pytest.param( - "/packit-dev test", + "/packit test", True, id="No labels specified", ), pytest.param( - "/packit-dev test --labels random-label1,random-label2", + "/packit test --labels random-label1,random-label2", False, id="Non-matching label specified", ), @@ -736,35 +736,35 @@ def test_tf_comment_labels(comment, result): "comment, default_labels, job_labels, result", ( pytest.param( - "/packit-dev test --labels label1,label2", + "/packit test --labels label1,label2", ["label3"], ["label3"], False, id="Labels specified in comment", ), pytest.param( - "/packit-dev test", + "/packit test", None, ["label1"], True, id="No labels specified, no default labels", ), pytest.param( - "/packit-dev test", + "/packit test", ["label2"], ["label1", "label2"], True, id="No labels specified, default labels matching", ), pytest.param( - "/packit-dev test", + "/packit test", ["label3"], ["label1", "label2"], False, id="No labels specified, default labels not matching", ), pytest.param( - "/packit-dev test", + "/packit test", ["label3"], [], False, @@ -829,12 +829,12 @@ def test_tf_comment_default_labels(comment, default_labels, job_labels, result): "comment, result", ( pytest.param( - "/packit-dev test", + "/packit test", True, id="No labels specified, none in config: should pass", ), pytest.param( - "/packit-dev test --labels should_fail,should_fail_hard", + "/packit test --labels should_fail,should_fail_hard", False, id="Labels specified, none in config: should fail", ), diff --git a/tests/unit/test_testing_farm.py b/tests/unit/test_testing_farm.py index a9095df52..e59165b50 100644 --- a/tests/unit/test_testing_farm.py +++ b/tests/unit/test_testing_farm.py @@ -2,7 +2,7 @@ # SPDX-License-Identifier: MIT import re from datetime import datetime, timezone -from typing import List +from typing import List, Dict import pytest from flexmock import flexmock @@ -365,7 +365,9 @@ def test_is_compose_matching(compose, composes, result): "tmt_plan," "tf_post_install_script," "tf_extra_params," - "copr_rpms" + "copr_rpms," + "comment," + "expected_envs" ), [ ( @@ -392,6 +394,8 @@ def test_is_compose_matching(compose, composes, result): None, None, None, + None, + None, ), ( "https://api.dev.testing-farm.io/v0.1/", @@ -417,6 +421,8 @@ def test_is_compose_matching(compose, composes, result): None, None, None, + None, + None, ), ( "https://api.dev.testing-farm.io/v0.1/", @@ -442,6 +448,8 @@ def test_is_compose_matching(compose, composes, result): None, None, None, + None, + None, ), # Testing built_packages ( @@ -474,6 +482,8 @@ def test_is_compose_matching(compose, composes, result): None, None, "cool-project-0:0.1.0-2.el8.x86_64", + None, + None, ), # Test tmt_plan and tf_post_install_script ( @@ -500,6 +510,8 @@ def test_is_compose_matching(compose, composes, result): "echo 'hi packit'", None, None, + None, + None, ), # Testing built_packages for more builds (additional build from other PR) ( @@ -536,6 +548,8 @@ def test_is_compose_matching(compose, composes, result): None, None, "not-cool-project-0:0.1.0-2.el8.x86_64", + None, + None, ), # Testing built_packages for more builds (additional build from other PR) and more packages ( @@ -580,6 +594,8 @@ def test_is_compose_matching(compose, composes, result): None, "cool-project-0:0.1.0-2.el8.x86_64 cool-project-2-0:0.1.0-2.el8.x86_64 " "not-cool-project-0:0.1.0-2.el8.x86_64 not-cool-project-2-0:0.1.0-2.el8.x86_64", + None, + None, ), # Test that API key and notifications is not overriden by extra-params ( @@ -609,6 +625,101 @@ def test_is_compose_matching(compose, composes, result): "notification": {"webhook": {"url": "https://malicious.net"}}, }, None, + None, + None, + ), + # Test that comment env vars are loaded properly to TF payload + ( + "https://api.dev.testing-farm.io/v0.1/", + "very-secret", + "", # without internal TF configured + False, + "test", + "packit", + "packit-service", + "feb41e5", + "1.0", + "https://github.com/source/packit", + "master", + "me", + "cool-project", + "123456", + "centos-stream-x86_64", + "centos-stream", + "Fedora-Rawhide", + "x86_64", + [{"id": "123456:centos-stream-x86_64", "type": "fedora-copr-build"}], + None, + None, + { + "api_key": "foo", + "notification": {"webhook": {"url": "https://malicious.net"}}, + }, + None, + "/packit test --labels suite1 --env IP_FAMILY=ipv6 --env INSTALL_TYPE=bundle", + {"IP_FAMILY": "ipv6", "INSTALL_TYPE": "bundle"}, + ), + # Test that comment env vars has the highest priority + ( + "https://api.dev.testing-farm.io/v0.1/", + "very-secret", + "", # without internal TF configured + False, + "test", + "packit", + "packit-service", + "feb41e5", + "1.0", + "https://github.com/source/packit", + "master", + "me", + "cool-project", + "123456", + "centos-stream-x86_64", + "centos-stream", + "Fedora-Rawhide", + "x86_64", + [{"id": "123456:centos-stream-x86_64", "type": "fedora-copr-build"}], + None, + None, + { + "api_key": "foo", + "notification": {"webhook": {"url": "https://malicious.net"}}, + }, + None, + "/packit test --labels suite1 --env IP_FAMILY=ipv6 --env MY_ENV_VARIABLE=my-value2", + {"IP_FAMILY": "ipv6", "MY_ENV_VARIABLE": "my-value2"}, + ), + # Test unseting the env variable + ( + "https://api.dev.testing-farm.io/v0.1/", + "very-secret", + "", # without internal TF configured + False, + "test", + "packit", + "packit-service", + "feb41e5", + "1.0", + "https://github.com/source/packit", + "master", + "me", + "cool-project", + "123456", + "centos-stream-x86_64", + "centos-stream", + "Fedora-Rawhide", + "x86_64", + [{"id": "123456:centos-stream-x86_64", "type": "fedora-copr-build"}], + None, + None, + { + "api_key": "foo", + "notification": {"webhook": {"url": "https://malicious.net"}}, + }, + None, + "/packit test --labels suite1 --env IP_FAMILY=ipv6 --env MY_ENV_VARIABLE=", + {"IP_FAMILY": "ipv6", "MY_ENV_VARIABLE": ""}, ), ], ) @@ -636,12 +747,15 @@ def test_payload( tf_post_install_script, tf_extra_params, copr_rpms, + comment, + expected_envs, ): service_config = ServiceConfig.get_service_config() service_config.testing_farm_api_url = tf_api service_config.testing_farm_secret = tf_token service_config.internal_testing_farm_secret = internal_tf_token service_config.deployment = ps_deployment + service_config.comment_command_prefix = "/packit" package_config = flexmock(jobs=[]) pr = flexmock( @@ -667,6 +781,7 @@ def test_payload( git_ref=git_ref, project_url=project_url, pr_id=123, + event_dict={"comment": comment}, ) db_project_object = flexmock() @@ -692,6 +807,8 @@ def test_payload( }, ), ) + # Add custom env var to job_config + job_helper.job_config.env = {"MY_ENV_VARIABLE": "my-value"} token_to_use = internal_tf_token if use_internal_tf else tf_token assert job_helper.tft_token == token_to_use @@ -768,6 +885,7 @@ def test_payload( "PACKIT_TARGET_URL": "https://github.com/packit/packit", "PACKIT_PR_ID": 123, "PACKIT_COPR_PROJECT": "builder/some_package", + "MY_ENV_VARIABLE": "my-value", }, } ] @@ -779,6 +897,12 @@ def test_payload( "provisioning": {"post_install_script": tf_post_install_script} } + if comment is not None: + expected_environments[0]["variables"].update(expected_envs) + # If MY_ENV_VARIABLE="" then it should be unset from payload and removed from expected envs + if expected_envs.get("MY_ENV_VARIABLE") == "": + expected_environments[0]["variables"].pop("MY_ENV_VARIABLE") + assert payload["environments"] == expected_environments assert payload["notification"]["webhook"]["url"].endswith("/testing-farm/results") if tf_extra_params: @@ -874,10 +998,16 @@ def test_merge_payload_with_extra_params(payload, params, result): def test_merge_extra_params_with_install(): tf_settings = {"provisioning": {"tags": {"BusinessUnit": "sst_upgrades"}}} - service_config = flexmock(testing_farm_secret="secret token", deployment="prod") + service_config = flexmock( + testing_farm_secret="secret token", + deployment="prod", + comment_command_prefix="/packit-dev", + ) package_config = flexmock() project = flexmock(full_repo_name="test/merge") - metadata = flexmock(commit_sha="0000000", pr_id=None, tag_name=None) + metadata = flexmock( + commit_sha="0000000", pr_id=None, tag_name=None, event_dict={"comment": ""} + ) db_project_event = ( flexmock() .should_receive("get_project_event_object") @@ -1020,6 +1150,7 @@ def test_test_repo( service_config.testing_farm_api_url = tf_api service_config.testing_farm_secret = tf_token service_config.deployment = ps_deployment + service_config.comment_command_prefix = "/packit-dev" package_config = flexmock(jobs=[]) pr = flexmock( @@ -1047,6 +1178,7 @@ def test_test_repo( git_ref=git_ref, project_url=source_project_url, pr_id=123, + event_dict={"comment": ""}, ) db_project_object = flexmock() @@ -1905,37 +2037,66 @@ def test_is_supported_architecture(target, use_internal_tf, supported): @pytest.mark.parametrize( - "comment,expected_identifier,expected_labels,expected_pr_arg", + "comment,expected_identifier,expected_labels,expected_pr_arg,expected_envs", [ ( "/packit-dev test --identifier my-id-1 --labels label1,label2 namespace-1/repo-1#33", "my-id-1", ["label1", "label2"], "namespace-1/repo-1#33", + None, ), ( "/packit-dev test namespace-2/repo-2#36 --identifier my-id-2", "my-id-2", None, "namespace-2/repo-2#36", + None, ), ( "/packit-dev test namespace-2/repo-2#36 --labels label1 --identifier my-id-2", "my-id-2", ["label1"], "namespace-2/repo-2#36", + None, ), ( "/packit-dev test namespace-2/repo-2#36 --labels label1 --id my-id-2", "my-id-2", ["label1"], "namespace-2/repo-2#36", + None, ), ( "/packit-dev test namespace-2/repo-2#36 --labels label1 -i my-id-2", "my-id-2", ["label1"], "namespace-2/repo-2#36", + None, + ), + ( + "/packit-dev test namespace-2/repo-2#36 --labels label1 -i my-id-2 " + "--env IP_FAMILY=ipv6", + "my-id-2", + ["label1"], + "namespace-2/repo-2#36", + {"IP_FAMILY": "ipv6"}, + ), + ( + "/packit-dev test namespace-2/repo-2#36 --env INSTALL_TYPE=bundle --labels label1" + " -i my-id-2 --env IP_FAMILY=ipv6", + "my-id-2", + ["label1"], + "namespace-2/repo-2#36", + {"INSTALL_TYPE": "bundle", "IP_FAMILY": "ipv6"}, + ), + ( + "/packit-dev test namespace-2/repo-2#36 --env INSTALL_TYPE= --labels label1" + " -i my-id-2 --env IP_FAMILY=ipv6", + "my-id-2", + ["label1"], + "namespace-2/repo-2#36", + {"IP_FAMILY": "ipv6", "INSTALL_TYPE": ""}, ), ], ) @@ -1944,6 +2105,7 @@ def test_parse_comment_arguments( expected_identifier: str, expected_labels: List[str], expected_pr_arg: str, + expected_envs: Dict[str, str], ): job_config = JobConfig( trigger=JobConfigTriggerType.pull_request, @@ -1973,3 +2135,4 @@ def test_parse_comment_arguments( assert helper.comment_arguments.pr_argument == expected_pr_arg assert helper.comment_arguments.identifier == expected_identifier assert helper.comment_arguments.labels == expected_labels + assert helper.comment_arguments.envs == expected_envs