From e6e27e17bc4f4afcd22e9efcd716d9ea71ded0a8 Mon Sep 17 00:00:00 2001 From: Matej Focko Date: Tue, 12 Mar 2024 13:25:26 +0100 Subject: [PATCH 1/6] style: early return for no TF jobs after Copr build Signed-off-by: Matej Focko --- packit_service/worker/handlers/copr.py | 77 +++++++++++++------------- 1 file changed, 39 insertions(+), 38 deletions(-) diff --git a/packit_service/worker/handlers/copr.py b/packit_service/worker/handlers/copr.py index 7368a58ab..5ba34ae0c 100644 --- a/packit_service/worker/handlers/copr.py +++ b/packit_service/worker/handlers/copr.py @@ -422,43 +422,44 @@ def handle_srpm_end(self): return TaskResults(success=True, details={"msg": msg}) def handle_testing_farm(self): - if self.copr_build_helper.job_tests_all: - event_dict = self.data.get_dict() - - for job_config in self.copr_build_helper.job_tests_all: - if ( - not job_config.skip_build - and not job_config.manual_trigger - # we need to check the labels here - # the same way as when scheduling jobs for event - and ( - job_config.trigger != JobConfigTriggerType.pull_request - or not ( - job_config.require.label.present - or job_config.require.label.absent - ) - or pr_labels_match_configuration( - pull_request=self.copr_build_helper.pull_request_object, - configured_labels_absent=job_config.require.label.absent, - configured_labels_present=job_config.require.label.present, - ) + if not self.copr_build_helper.job_tests_all: + logger.debug("Testing farm not in the job config.") + return + + event_dict = self.data.get_dict() + + for job_config in self.copr_build_helper.job_tests_all: + if ( + not job_config.skip_build + and not job_config.manual_trigger + # we need to check the labels here + # the same way as when scheduling jobs for event + and ( + job_config.trigger != JobConfigTriggerType.pull_request + or not ( + job_config.require.label.present + or job_config.require.label.absent ) - and self.copr_event.chroot - in self.copr_build_helper.build_targets_for_test_job(job_config) - ): - event_dict["tests_targets_override"] = list( - self.copr_build_helper.build_target2test_targets_for_test_job( - self.copr_event.chroot, job_config - ) + or pr_labels_match_configuration( + pull_request=self.copr_build_helper.pull_request_object, + configured_labels_absent=job_config.require.label.absent, + configured_labels_present=job_config.require.label.present, ) - signature( - TaskName.testing_farm.value, - kwargs={ - "package_config": dump_package_config(self.package_config), - "job_config": dump_job_config(job_config), - "event": event_dict, - "build_id": self.build.id, - }, - ).apply_async() - else: - logger.debug("Testing farm not in the job config.") + ) + and self.copr_event.chroot + in self.copr_build_helper.build_targets_for_test_job(job_config) + ): + event_dict["tests_targets_override"] = list( + self.copr_build_helper.build_target2test_targets_for_test_job( + self.copr_event.chroot, job_config + ) + ) + signature( + TaskName.testing_farm.value, + kwargs={ + "package_config": dump_package_config(self.package_config), + "job_config": dump_job_config(job_config), + "event": event_dict, + "build_id": self.build.id, + }, + ).apply_async() From f6b6826fe41e4b0df72057910dbd7ca6489f489b Mon Sep 17 00:00:00 2001 From: Matej Focko Date: Mon, 15 Apr 2024 16:07:41 +0200 Subject: [PATCH 2/6] feat(reporting): allow templating status name When given a specific template for the status name, just use it instead of applying the default one. Related to packit/packit-service#2266 Signed-off-by: Matej Focko --- .../worker/helpers/build/build_helper.py | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/packit_service/worker/helpers/build/build_helper.py b/packit_service/worker/helpers/build/build_helper.py index bdc480f1a..0b20c4033 100644 --- a/packit_service/worker/helpers/build/build_helper.py +++ b/packit_service/worker/helpers/build/build_helper.py @@ -385,14 +385,24 @@ def get_check_cls( chroot: str = None, project_event_identifier: Optional[str] = None, identifier: Optional[str] = None, + package: Optional[str] = None, + template: Optional[str] = None, ): + if project_event_identifier: + project_event_identifier = project_event_identifier.replace(":", "-") + + if template is not None: + return template.format( + job_name=job_name, + chroot=chroot, + event=project_event_identifier, + identifier=identifier, + package=package, + ) + chroot_str = f":{chroot}" if chroot else "" # replace ':' in the project event identifier - trigger_str = ( - f":{project_event_identifier.replace(':', '-')}" - if project_event_identifier - else "" - ) + trigger_str = f":{project_event_identifier}" if project_event_identifier else "" optional_suffix = f":{identifier}" if identifier else "" return f"{job_name}{trigger_str}{chroot_str}{optional_suffix}" From d5fb01f0388981e5e9556ab9df9077300ce108ee Mon Sep 17 00:00:00 2001 From: Matej Focko Date: Mon, 15 Apr 2024 16:09:08 +0200 Subject: [PATCH 3/6] feat(reporting): pass status name template Related to packit/packit-service#2266 Signed-off-by: Matej Focko --- .../worker/helpers/build/build_helper.py | 22 +++++++++++++++++-- packit_service/worker/helpers/testing_farm.py | 6 ++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/packit_service/worker/helpers/build/build_helper.py b/packit_service/worker/helpers/build/build_helper.py index 0b20c4033..e2d09e2d5 100644 --- a/packit_service/worker/helpers/build/build_helper.py +++ b/packit_service/worker/helpers/build/build_helper.py @@ -412,9 +412,16 @@ def get_build_check_cls( chroot: str = None, project_event_identifier: Optional[str] = None, identifier: Optional[str] = None, + package: Optional[str] = None, + template: Optional[str] = None, ): return cls.get_check_cls( - cls.status_name_build, chroot, project_event_identifier, identifier + cls.status_name_build, + chroot, + project_event_identifier, + identifier, + package=package, + template=template, ) @classmethod @@ -423,9 +430,16 @@ def get_test_check_cls( chroot: str = None, project_event_identifier: Optional[str] = None, identifier: Optional[str] = None, + package: Optional[str] = None, + template: Optional[str] = None, ): return cls.get_check_cls( - cls.status_name_test, chroot, project_event_identifier, identifier + cls.status_name_test, + chroot, + project_event_identifier, + identifier, + package=package, + template=template, ) @property @@ -447,6 +461,8 @@ def get_build_check(self, chroot: str = None) -> str: chroot, self.project_event_identifier_for_status, self.job_build_or_job_config.identifier, + package=None, # [TODO] + template=self.job_build_or_job_config.status_name_template, ) def test_check_names_for_test_job(self, test_job_config: JobConfig) -> List[str]: @@ -460,6 +476,8 @@ def test_check_names_for_test_job(self, test_job_config: JobConfig) -> List[str] target, self.project_event_identifier_for_status, test_job_config.identifier, + package=None, # [TODO] + template=test_job_config.status_name_template, ) for target in self.tests_targets_for_test_job(test_job_config) ] diff --git a/packit_service/worker/helpers/testing_farm.py b/packit_service/worker/helpers/testing_farm.py index 68407ef15..eff49bbad 100644 --- a/packit_service/worker/helpers/testing_farm.py +++ b/packit_service/worker/helpers/testing_farm.py @@ -1283,7 +1283,11 @@ def tests_targets(self) -> Set[str]: def get_test_check(self, chroot: str = None) -> str: return self.get_test_check_cls( - chroot, self.project_event_identifier_for_status, self.job_config.identifier + chroot, + self.project_event_identifier_for_status, + self.job_config.identifier, + package=None, # [TODO] + template=self.job_config.status_name_template, ) @property From 444b3df99a07fad47fa44df06581eac5268e62e9 Mon Sep 17 00:00:00 2001 From: Matej Focko Date: Mon, 15 Apr 2024 16:11:03 +0200 Subject: [PATCH 4/6] test(reporting): add tests for status name template Related to packit/packit-service#2266 Signed-off-by: Matej Focko --- tests/unit/test_status_names.py | 154 ++++++++++++++++++++++++++++++++ 1 file changed, 154 insertions(+) create mode 100644 tests/unit/test_status_names.py diff --git a/tests/unit/test_status_names.py b/tests/unit/test_status_names.py new file mode 100644 index 000000000..22ca647b1 --- /dev/null +++ b/tests/unit/test_status_names.py @@ -0,0 +1,154 @@ +# Copyright Contributors to the Packit project. +# SPDX-License-Identifier: MIT + +# import re +from dataclasses import dataclass +from typing import Optional +import logging + +# from flexmock import flexmock +import pytest + +from packit_service.worker.helpers.build.build_helper import BaseBuildJobHelper +from packit_service.worker.helpers.build.copr_build import CoprBuildJobHelper +from packit_service.worker.helpers.testing_farm import TestingFarmJobHelper + +logger = logging.getLogger(__name__) + + +@dataclass +class StatusNameTestcase: + job_name: Optional[str] = None + chroot: Optional[str] = None + event: Optional[str] = None + identifier: Optional[str] = None + package: Optional[str] = None + template: Optional[str] = None + expected: str = None + + +@pytest.mark.parametrize( + "testcase", + [ + pytest.param( + StatusNameTestcase( + job_name="copr", + chroot="centos-10", + event="pr-42", + identifier=None, + package="packit", + template=None, + expected="copr:pr-42:centos-10", + ), + id="default template", + ), + pytest.param( + StatusNameTestcase( + job_name="copr", + chroot="centos-10", + event="pr-42", + identifier=None, + package="packit", + template="packit:{job_name}:{event}:{chroot}", + expected="packit:copr:pr-42:centos-10", + ), + id="custom template", + ), + pytest.param( + StatusNameTestcase( + job_name="copr", + chroot="fedora-40", + event="stable", + identifier="custom-build", + package="special-package", + template="packit:rpm-build:{package}:{identifier}:{chroot}", + expected="packit:rpm-build:special-package:custom-build:fedora-40", + ), + id="custom template #2", + ), + ], +) +def test_get_check_cls(testcase): + assert ( + BaseBuildJobHelper.get_check_cls( + job_name=testcase.job_name, + chroot=testcase.chroot, + project_event_identifier=testcase.event, + identifier=testcase.identifier, + package=testcase.package, + template=testcase.template, + ) + == testcase.expected + ) + + +@pytest.mark.parametrize( + "testcase", + [ + pytest.param( + StatusNameTestcase( + chroot="centos-10", + event="stable", + identifier="release-build", + expected="rpm-build:stable:centos-10:release-build", + ), + id="default template", + ), + pytest.param( + StatusNameTestcase( + chroot="fedora-40-x86_64", + event="pr-42069", + identifier="release-build", + template="copr-build:pr:{chroot}:{identifier}", + expected="copr-build:pr:fedora-40-x86_64:release-build", + ), + id="custom template", + ), + ], +) +def test_get_copr_build_check_cls(testcase): + assert ( + CoprBuildJobHelper.get_build_check_cls( + chroot=testcase.chroot, + project_event_identifier=testcase.event, + identifier=testcase.identifier, + template=testcase.template, + ) + == testcase.expected + ) + + +@pytest.mark.parametrize( + "testcase", + [ + pytest.param( + StatusNameTestcase( + chroot="centos-10", + event="stable", + identifier="release-test", + expected="testing-farm:stable:centos-10:release-test", + ), + id="default template", + ), + pytest.param( + StatusNameTestcase( + chroot="fedora-40-x86_64", + event="pr-42069", + identifier="revdep-on-release-build", + template="tests:pr:{chroot}:{identifier}", + expected="tests:pr:fedora-40-x86_64:revdep-on-release-build", + ), + id="custom template", + ), + ], +) +def test_get_copr_test_check_cls(testcase): + assert ( + TestingFarmJobHelper.get_test_check_cls( + chroot=testcase.chroot, + project_event_identifier=testcase.event, + identifier=testcase.identifier, + template=testcase.template, + ) + == testcase.expected + ) From a4012cbda22a92202e3b121f890b28e0ffa5385e Mon Sep 17 00:00:00 2001 From: Matej Focko Date: Mon, 15 Apr 2024 16:50:39 +0200 Subject: [PATCH 5/6] feat(reporting): pass package name to the status name template Related to packit/packit-service#2266 Signed-off-by: Matej Focko --- .../worker/helpers/build/build_helper.py | 4 ++-- packit_service/worker/helpers/testing_farm.py | 2 +- tests/unit/test_allowlist.py | 14 ++++++++++++-- tests/unit/test_testing_farm.py | 3 +++ 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/packit_service/worker/helpers/build/build_helper.py b/packit_service/worker/helpers/build/build_helper.py index e2d09e2d5..f70659fc7 100644 --- a/packit_service/worker/helpers/build/build_helper.py +++ b/packit_service/worker/helpers/build/build_helper.py @@ -461,7 +461,7 @@ def get_build_check(self, chroot: str = None) -> str: chroot, self.project_event_identifier_for_status, self.job_build_or_job_config.identifier, - package=None, # [TODO] + package=self.get_package_name(), template=self.job_build_or_job_config.status_name_template, ) @@ -476,7 +476,7 @@ def test_check_names_for_test_job(self, test_job_config: JobConfig) -> List[str] target, self.project_event_identifier_for_status, test_job_config.identifier, - package=None, # [TODO] + package=self.get_package_name(), template=test_job_config.status_name_template, ) for target in self.tests_targets_for_test_job(test_job_config) diff --git a/packit_service/worker/helpers/testing_farm.py b/packit_service/worker/helpers/testing_farm.py index eff49bbad..a9adcfe95 100644 --- a/packit_service/worker/helpers/testing_farm.py +++ b/packit_service/worker/helpers/testing_farm.py @@ -1286,7 +1286,7 @@ def get_test_check(self, chroot: str = None) -> str: chroot, self.project_event_identifier_for_status, self.job_config.identifier, - package=None, # [TODO] + package=self.get_package_name(), template=self.job_config.status_name_template, ) diff --git a/tests/unit/test_allowlist.py b/tests/unit/test_allowlist.py index 1dc2159c1..2481924ca 100644 --- a/tests/unit/test_allowlist.py +++ b/tests/unit/test_allowlist.py @@ -620,7 +620,12 @@ def test_check_and_report( ) ] flexmock(PullRequestGithubEvent).should_receive("get_packages_config").and_return( - flexmock(jobs=job_configs, get_package_config_for=lambda job_config: flexmock()) + flexmock( + jobs=job_configs, + get_package_config_for=lambda job_config: flexmock( + packages={"package": {}} + ), + ) ) _, _ = add_pull_request_event_with_empty_sha @@ -805,7 +810,12 @@ def test_check_and_report_actor_pull_request( ) ] flexmock(PullRequestGithubEvent).should_receive("get_packages_config").and_return( - flexmock(jobs=job_configs, get_package_config_for=lambda job_config: flexmock()) + flexmock( + jobs=job_configs, + get_package_config_for=lambda job_config: flexmock( + packages={"package": {}} + ), + ) ) _, _ = add_pull_request_event_with_empty_sha diff --git a/tests/unit/test_testing_farm.py b/tests/unit/test_testing_farm.py index cd27cefb0..52f890162 100644 --- a/tests/unit/test_testing_farm.py +++ b/tests/unit/test_testing_farm.py @@ -102,6 +102,9 @@ def test_testing_farm_response( }, ), ], + packages={ + "package": {}, + }, ) flexmock(PackageConfigGetter).should_receive( "get_package_config_from_repo" From 026b2a38006ed8214b226ae0c274fc662b825844 Mon Sep 17 00:00:00 2001 From: Matej Focko Date: Mon, 29 Apr 2024 13:03:38 +0200 Subject: [PATCH 6/6] fix(reporting): wrap template in a try-catch When invalid template is given, fallback to the default status name we use and report the failure as a warning to logs. Signed-off-by: Matej Focko --- .../worker/helpers/build/build_helper.py | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/packit_service/worker/helpers/build/build_helper.py b/packit_service/worker/helpers/build/build_helper.py index f70659fc7..91b779721 100644 --- a/packit_service/worker/helpers/build/build_helper.py +++ b/packit_service/worker/helpers/build/build_helper.py @@ -391,14 +391,21 @@ def get_check_cls( if project_event_identifier: project_event_identifier = project_event_identifier.replace(":", "-") - if template is not None: - return template.format( - job_name=job_name, - chroot=chroot, - event=project_event_identifier, - identifier=identifier, - package=package, + try: + if template is not None: + return template.format( + job_name=job_name, + chroot=chroot, + event=project_event_identifier, + identifier=identifier, + package=package, + ) + except Exception as e: + logger.warning( + "Failed to use the template for status check, falling back to default: %s", + e, ) + pass chroot_str = f":{chroot}" if chroot else "" # replace ':' in the project event identifier