diff --git a/packit_service/utils.py b/packit_service/utils.py index 096962541..2cb55989c 100644 --- a/packit_service/utils.py +++ b/packit_service/utils.py @@ -6,8 +6,9 @@ from io import StringIO from logging import StreamHandler from re import search -from typing import List, Tuple +from typing import List, Tuple, Optional +from ogr.abstract import PullRequest from packit.config import JobConfig, PackageConfig from packit.schema import JobConfigSchema, PackageConfigSchema from packit.utils import PackitFormatter @@ -222,3 +223,33 @@ def get_koji_task_id_and_url_from_stdout(stdout: str): task_url = task_url_match.group(0) return task_id, task_url + + +def pr_labels_match_configuration( + pull_request: Optional[PullRequest], + configured_labels_present: list[str], + configured_labels_absent: list[str], +) -> bool: + """ + Do the PR labels match the configuration of the labels? + """ + if not pull_request: + logger.debug("No PR to check the labels on.") + return True + + logger.info( + f"About to check whether PR labels in PR {pull_request.id} " + f"match to the labels configuration " + f"(label.present: {configured_labels_present}, label.absent: {configured_labels_absent})" + ) + + pr_labels = pull_request.labels + logger.info(f"Labels on PR: {pr_labels}") + + return ( + not configured_labels_present + or any(label in pr_labels for label in configured_labels_present) + ) and ( + not configured_labels_absent + or all(label not in pr_labels for label in configured_labels_absent) + ) diff --git a/packit_service/worker/checker/distgit.py b/packit_service/worker/checker/distgit.py index e2e623097..57238bc43 100644 --- a/packit_service/worker/checker/distgit.py +++ b/packit_service/worker/checker/distgit.py @@ -7,6 +7,7 @@ from ogr.abstract import AccessLevel from packit.config.aliases import get_branches from packit_service.constants import MSG_GET_IN_TOUCH, KojiAllowedAccountsAlias +from packit_service.utils import pr_labels_match_configuration from packit_service.worker.checker.abstract import Checker, ActorChecker from packit_service.worker.events import ( PushPagureEvent, @@ -24,6 +25,21 @@ logger = logging.getLogger(__name__) +class LabelsOnDistgitPR(Checker, GetPagurePullRequestMixin): + def pre_check(self) -> bool: + if self.data.event_type not in (PushPagureEvent.__name__,) or not ( + self.job_config.require.label.present + or self.job_config.require.label.absent + ): + return True + + return pr_labels_match_configuration( + self.pull_request, + self.job_config.require.label.present, + self.job_config.require.label.absent, + ) + + class PermissionOnDistgit(Checker, GetPagurePullRequestMixin): def contains_specfile_change(self): """ diff --git a/packit_service/worker/events/comment.py b/packit_service/worker/events/comment.py index f3ee4c8b2..94c540b7e 100644 --- a/packit_service/worker/events/comment.py +++ b/packit_service/worker/events/comment.py @@ -7,8 +7,7 @@ from logging import getLogger from typing import Dict, Optional, Set -from ogr.abstract import Comment, PullRequest, Issue - +from ogr.abstract import Comment, Issue from packit_service.models import TestingFarmResult, BuildStatus from packit_service.service.db_project_events import ( AddIssueEventToDb, @@ -70,7 +69,6 @@ def __init__( self._comment_object = comment_object self._build_targets_override = build_targets_override self._tests_targets_override = tests_targets_override - self._pull_request_object = None @property def commit_sha(self) -> str: # type:ignore @@ -79,12 +77,6 @@ def commit_sha(self) -> str: # type:ignore self._commit_sha = self.project.get_pr(pr_id=self.pr_id).head_commit return self._commit_sha - @property - def pull_request_object(self) -> PullRequest: - if not self._pull_request_object: - self._pull_request_object = self.project.get_pr(self.pr_id) - return self._pull_request_object - @property def comment_object(self) -> Optional[Comment]: if not self._comment_object: @@ -121,7 +113,6 @@ def get_dict(self, default_dict: Optional[Dict] = None) -> dict: result["commit_sha"] = self.commit_sha result.pop("_build_targets_override") result.pop("_tests_targets_override") - result.pop("_pull_request_object") return result diff --git a/packit_service/worker/events/event.py b/packit_service/worker/events/event.py index 61ba65ff2..f9f201239 100644 --- a/packit_service/worker/events/event.py +++ b/packit_service/worker/events/event.py @@ -9,7 +9,7 @@ from logging import getLogger from typing import Dict, Optional, Type, Union, Set, List -from ogr.abstract import GitProject +from ogr.abstract import GitProject, PullRequest from ogr.parsing import RepoUrl from packit.config import JobConfigTriggerType, PackageConfig @@ -461,6 +461,7 @@ def __init__( self._pr_id = pr_id self.fail_when_config_file_missing = False self.actor = actor + self._pull_request_object = None @property def project(self): @@ -491,6 +492,12 @@ def get_db_project_event(self) -> Optional[ProjectEventModel]: def pr_id(self) -> Optional[int]: return self._pr_id + @property + def pull_request_object(self) -> Optional[PullRequest]: + if not self._pull_request_object and self.pr_id: + self._pull_request_object = self.project.get_pr(self.pr_id) + return self._pull_request_object + def get_project(self) -> Optional[GitProject]: if not (self.project_url or self.db_project_object): return None @@ -552,6 +559,11 @@ def get_all_build_targets_by_status( statuses_to_filter_with=statuses_to_filter_with, ) + def get_dict(self, default_dict: Optional[Dict] = None) -> dict: + result = super().get_dict() + result.pop("_pull_request_object") + return result + class AbstractResultEvent(AbstractForgeIndependentEvent): """ diff --git a/packit_service/worker/handlers/distgit.py b/packit_service/worker/handlers/distgit.py index 53215e252..a24c0f1cf 100644 --- a/packit_service/worker/handlers/distgit.py +++ b/packit_service/worker/handlers/distgit.py @@ -60,6 +60,7 @@ ValidInformationForPullFromUpstream, HasIssueCommenterRetriggeringPermissions, IsUpstreamTagMatchingConfig, + LabelsOnDistgitPR, ) from packit_service.worker.events import ( PushPagureEvent, @@ -678,7 +679,11 @@ def __init__( @staticmethod def get_checkers() -> Tuple[Type[Checker], ...]: - return (PermissionOnDistgit, HasIssueCommenterRetriggeringPermissions) + return ( + LabelsOnDistgitPR, + PermissionOnDistgit, + HasIssueCommenterRetriggeringPermissions, + ) def _get_or_create_koji_group_model(self) -> KojiBuildGroupModel: if self._koji_group_model_id is not None: diff --git a/packit_service/worker/jobs.py b/packit_service/worker/jobs.py index ebeb2ce05..21a2048fc 100644 --- a/packit_service/worker/jobs.py +++ b/packit_service/worker/jobs.py @@ -24,7 +24,11 @@ COMMENT_REACTION, PACKIT_VERIFY_FAS_COMMAND, ) -from packit_service.utils import get_packit_commands_from_comment, elapsed_seconds +from packit_service.utils import ( + get_packit_commands_from_comment, + elapsed_seconds, + pr_labels_match_configuration, +) from packit_service.worker.allowlist import Allowlist from packit_service.worker.events import ( Event, @@ -39,7 +43,10 @@ AbstractIssueCommentEvent, AbstractPRCommentEvent, ) -from packit_service.worker.events.event import AbstractResultEvent +from packit_service.worker.events.event import ( + AbstractResultEvent, + AbstractForgeIndependentEvent, +) from packit_service.worker.handlers import ( CoprBuildHandler, GithubAppInstallationHandler, @@ -666,6 +673,16 @@ def get_jobs_matching_event(self) -> List[JobConfig]: for event_type in MANUAL_OR_RESULT_EVENTS ) ) + and ( + job.trigger != JobConfigTriggerType.pull_request + or not (job.require.label.present or job.require.label.absent) + or not isinstance(self.event, AbstractForgeIndependentEvent) + or pr_labels_match_configuration( + pull_request=self.event.pull_request_object, + configured_labels_absent=job.require.label.absent, + configured_labels_present=job.require.label.present, + ) + ) ): jobs_matching_trigger.append(job) diff --git a/tests/unit/test_babysit_vm_image.py b/tests/unit/test_babysit_vm_image.py index 71ffff1b4..067fee223 100644 --- a/tests/unit/test_babysit_vm_image.py +++ b/tests/unit/test_babysit_vm_image.py @@ -7,6 +7,7 @@ from flexmock import flexmock from flexmock import Mock from packit.config.job_config import JobConfigTriggerType, JobType +from packit.config.requirements import RequirementsConfig, LabelRequirementsConfig from packit_service.config import ServiceConfig from packit_service.models import ( VMImageBuildTargetModel, @@ -117,6 +118,12 @@ def test_update_vm_image_build(stop_babysitting, build_status, vm_image_builder_ trigger=JobConfigTriggerType.pull_request, type=JobType.vm_image_build, manual_trigger=False, + require=RequirementsConfig( + LabelRequirementsConfig( + absent=[], + present=[], + ) + ), ) ], ) diff --git a/tests/unit/test_checkers.py b/tests/unit/test_checkers.py index 9cf128909..4f3dce1ce 100644 --- a/tests/unit/test_checkers.py +++ b/tests/unit/test_checkers.py @@ -6,7 +6,7 @@ from flexmock import flexmock from ogr import PagureService -from ogr.abstract import AccessLevel +from ogr.abstract import AccessLevel, PRStatus from ogr.services.pagure import PagureProject from packit.config import ( CommonPackageConfig, @@ -18,6 +18,7 @@ ) from packit.config.commands import TestCommandConfig +from packit.config.requirements import RequirementsConfig, LabelRequirementsConfig from packit_service.config import ServiceConfig from packit_service.models import CoprBuildTargetModel from packit_service.worker.checker.copr import ( @@ -27,6 +28,7 @@ from packit_service.worker.checker.distgit import ( IsUpstreamTagMatchingConfig, PermissionOnDistgit, + LabelsOnDistgitPR, ) from packit_service.worker.checker.koji import ( IsJobConfigTriggerMatching as IsJobConfigTriggerMatchingKoji, @@ -992,3 +994,61 @@ def test_koji_check_allowed_accounts( package_config, job_config, distgit_push_event.get_dict() ) assert checker.check_allowed_accounts(allowed_pr_authors, account) == should_pass + + +@pytest.mark.parametrize( + "pr_labels,labels_present,labels_absent,should_pass", + ( + ([], [], [], True), + (["allowed-1"], [], ["skip-ci"], True), + (["allowed-1"], ["allowed-1"], ["skip-ci"], True), + (["allowed-1"], ["allowed-1"], ["skip-ci"], True), + (["allowed-1", "skip-ci"], ["allowed-1"], ["skip-ci"], False), + ), +) +def test_labels_on_distgit_pr( + distgit_push_event, + pr_labels, + labels_present, + labels_absent, + should_pass, +): + jobs = [ + JobConfig( + type=JobType.koji_build, + trigger=JobConfigTriggerType.commit, + packages={ + "package": CommonPackageConfig( + dist_git_branches=["f36"], + require=RequirementsConfig( + LabelRequirementsConfig( + absent=labels_absent, + present=labels_present, + ) + ), + ) + }, + ), + ] + + package_config = PackageConfig( + jobs=jobs, + packages={"package": CommonPackageConfig()}, + ) + job_config = jobs[0] + + flexmock(PagureProject).should_receive("get_pr_list").and_return( + [ + flexmock( + id=5, + head_commit="ad0c308af91da45cf40b253cd82f07f63ea9cbbf", + status=PRStatus.open, + labels=pr_labels, + ) + ] + ) + + checker = LabelsOnDistgitPR( + package_config, job_config, distgit_push_event.get_dict() + ) + assert checker.pre_check() == should_pass diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index ae905a68b..735a9e012 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -1,7 +1,9 @@ # Copyright Contributors to the Packit project. # SPDX-License-Identifier: MIT +import pytest +from flexmock import flexmock -from packit_service.utils import only_once +from packit_service.utils import only_once, pr_labels_match_configuration def test_only_once(): @@ -74,3 +76,87 @@ def f(one, two, three="something"): assert counter == 1 f("b", "b", three="different") assert counter == 1 + + +@pytest.mark.parametrize( + "absent,present,pr_labels,should_pass", + [ + pytest.param( + [], + ["my-label"], + [], + False, + ), + pytest.param( + [], + ["my-label"], + ["my-label"], + True, + ), + pytest.param( + ["skip-ci"], + ["my-label"], + ["my-label"], + True, + ), + pytest.param( + ["skip-ci"], + ["my-label"], + ["my-label", "skip-ci"], + False, + ), + pytest.param( + ["skip-ci"], + ["my-label"], + ["skip-ci"], + False, + ), + pytest.param( + ["skip-ci"], + [], + ["skip-ci"], + False, + ), + pytest.param( + ["skip-ci"], + [], + [], + True, + ), + pytest.param( + ["skip-ci"], + ["first", "second"], + ["second"], + True, + ), + pytest.param( + ["skip-ci"], + ["first", "second"], + ["third"], + False, + ), + pytest.param( + ["skip-ci", "block-ci"], + ["first", "second"], + ["block-ci"], + False, + ), + pytest.param( + [], + [], + [], + True, + ), + pytest.param( + [], + [], + ["some-label"], + True, + ), + ], +) +def test_pr_labels_match(absent, present, pr_labels, should_pass): + assert ( + pr_labels_match_configuration(flexmock(labels=pr_labels, id=1), present, absent) + == should_pass + )