diff --git a/packit_service/models.py b/packit_service/models.py index 6e5e42b41..ca2725820 100644 --- a/packit_service/models.py +++ b/packit_service/models.py @@ -7,6 +7,7 @@ import enum import logging +import re from collections import Counter from collections.abc import Generator, Iterable from contextlib import contextmanager @@ -42,6 +43,7 @@ null, ) from sqlalchemy.dialects.postgresql import array as psql_array +from sqlalchemy.dialects.postgresql import dialect as psql_dialect from sqlalchemy.ext.declarative import declarative_base from sqlalchemy.orm import ( Session as SQLASession, @@ -2484,7 +2486,7 @@ def get_all_projects(cls) -> set["GitProjectModel"]: return set(projects) @classmethod - def get_first_successful_by_sidetag( + def get_last_successful_by_sidetag( cls, sidetag: str, ) -> Optional["BodhiUpdateTargetModel"]: @@ -2495,9 +2497,28 @@ def get_first_successful_by_sidetag( BodhiUpdateTargetModel.status == "success", BodhiUpdateTargetModel.sidetag == sidetag, ) + .order_by(BodhiUpdateTargetModel.update_creation_time.desc()) .first() ) + @classmethod + def get_all_successful_or_in_progress_by_nvrs( + cls, + koji_nvrs: str, + ) -> set["BodhiUpdateTargetModel"]: + regexp = "|".join(re.escape(nvr) for nvr in set(koji_nvrs.split())) + with sa_session_transaction() as session: + return set( + session.query(BodhiUpdateTargetModel) + .filter( + BodhiUpdateTargetModel.status.in_(("queued", "retry", "success")), + BodhiUpdateTargetModel.koji_nvrs.regexp_match(regexp).compile( + dialect=psql_dialect(), + ), + ) + .all(), + ) + class BodhiUpdateGroupModel(ProjectAndEventsConnector, GroupModel, Base): __tablename__ = "bodhi_update_groups" diff --git a/packit_service/worker/handlers/bodhi.py b/packit_service/worker/handlers/bodhi.py index 9d505c1cf..8719d7eca 100644 --- a/packit_service/worker/handlers/bodhi.py +++ b/packit_service/worker/handlers/bodhi.py @@ -110,11 +110,33 @@ def run(self) -> TaskResults: existing_alias = None # get update alias from previous run(s) from the same sidetag (if any) if target_model.sidetag and ( - model := BodhiUpdateTargetModel.get_first_successful_by_sidetag( + existing_model := BodhiUpdateTargetModel.get_last_successful_by_sidetag( target_model.sidetag, ) ): - existing_alias = model.alias + existing_alias = existing_model.alias + if set(target_model.koji_nvrs.split()) == set( + existing_model.koji_nvrs.split(), + ): + logger.info("No changes, skipping Bodhi update edit") + target_model.set_status("skipped") + continue + + if not existing_alias: + # avoid creating another update containing the same build - Bodhi shouldn't + # allow it anyway but there is a race condition that makes it possible + existing_models = ( + BodhiUpdateTargetModel.get_all_successful_or_in_progress_by_nvrs( + target_model.koji_nvrs, + ) + ) + if existing_models - {target_model}: + logger.info( + "Bodhi update containing one or more builds from " + f"{{{target_model.koji_nvrs}}} already exists, skipping", + ) + target_model.set_status("skipped") + continue logger.debug( (f"Edit update {existing_alias} " if existing_alias else "Create update ") diff --git a/tests/integration/test_bodhi_update.py b/tests/integration/test_bodhi_update.py index 2b9b9e05d..29d1fbbf0 100644 --- a/tests/integration/test_bodhi_update.py +++ b/tests/integration/test_bodhi_update.py @@ -118,6 +118,9 @@ def test_bodhi_update_for_unknown_koji_build(koji_build_completed_old_format): ) flexmock(ProjectEventModel).should_receive("get_or_create") flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("packit-0.43.0-1.fc36").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="rawhide", koji_nvrs="packit-0.43.0-1.fc36", @@ -221,6 +224,9 @@ def test_bodhi_update_for_unknown_koji_build_failed(koji_build_completed_old_for ], ) flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("packit-0.43.0-1.fc36").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="rawhide", koji_nvrs="packit-0.43.0-1.fc36", @@ -328,6 +334,9 @@ def test_bodhi_update_for_unknown_koji_build_failed_issue_created( ], ) flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("packit-0.43.0-1.fc36").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="rawhide", koji_nvrs="packit-0.43.0-1.fc36", @@ -448,6 +457,9 @@ def test_bodhi_update_for_unknown_koji_build_failed_issue_comment( ], ) flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("packit-0.43.0-1.fc36").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="rawhide", koji_nvrs="packit-0.43.0-1.fc36", @@ -545,6 +557,9 @@ def test_bodhi_update_build_not_tagged_yet( ], ) flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("packit-0.43.0-1.fc36").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="rawhide", koji_nvrs="packit-0.43.0-1.fc36", @@ -656,6 +671,9 @@ def test_bodhi_update_for_unknown_koji_build_not_for_unfinished( ], ) flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("packit-0.43.0-1.fc36").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="rawhide", koji_nvrs="packit-0.43.0-1.fc36", @@ -742,6 +760,9 @@ def test_bodhi_update_for_known_koji_build(koji_build_completed_old_format): ], ) flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("packit-0.43.0-1.fc36").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="rawhide", koji_nvrs="packit-0.43.0-1.fc36", @@ -893,6 +914,9 @@ def test_bodhi_update_fedora_stable_by_default(koji_build_completed_f36): ], ) flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("python-ogr-0.34.0-1.fc36").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="f36", koji_nvrs="python-ogr-0.34.0-1.fc36", @@ -929,12 +953,30 @@ def test_bodhi_update_fedora_stable_by_default(koji_build_completed_f36): @pytest.mark.parametrize( - "missing_dependency, existing_update", - [(False, None), (False, "FEDORA-2024-abcdef1234"), (True, None)], + "missing_dependency, non_unique_builds, existing_update", + [ + (False, False, None), + ( + False, + False, + flexmock( + alias="FEDORA-2024-abcdef1234", + koji_nvrs="python-specfile-0.31.0-1.fc40 packit-0.99.0-1.fc40", + ), + ), + ( + False, + False, + flexmock(alias="FEDORA-2024-abcdef1234", koji_nvrs="packit-0.99.0-1.fc40"), + ), + (False, True, None), + (True, False, None), + ], ) def test_bodhi_update_from_sidetag( koji_build_tagged, missing_dependency, + non_unique_builds, existing_update, ): """(Sidetag scenario.)""" @@ -997,10 +1039,8 @@ def test_bodhi_update_from_sidetag( ).and_return(flexmock(target=dg_branch, get_project_event_model=lambda: None)) flexmock(BodhiUpdateTargetModel).should_receive( - "get_first_successful_by_sidetag", - ).with_args(sidetag_name).and_return( - flexmock(alias=existing_update) if existing_update else None, - ) + "get_last_successful_by_sidetag", + ).with_args(sidetag_name).and_return(existing_update) specfile_packit_yaml = ( "{'specfile_path': 'python-specfile.spec', 'synced_files': []," @@ -1096,30 +1136,44 @@ def _create_update(dist_git_branch, update_type, koji_builds, sidetag, alias): "packit-0.99.0-1.fc40", } assert sidetag == sidetag_name - assert alias == (existing_update if existing_update else None) + assert alias == (existing_update.alias if existing_update else None) return "alias", "url" flexmock(PackitAPI).should_receive("create_update").replace_with( _create_update, - ).times(0 if missing_dependency else 1) + ).times( + ( + 0 + if missing_dependency + or non_unique_builds + or existing_update + and " " in existing_update.koji_nvrs + else 1 + ), + ) flexmock(PipelineModel).should_receive("create").and_return(flexmock()) - group_model = flexmock( - grouped_targets=[ - flexmock( - target=dg_branch, - koji_nvrs="python-specfile-0.31.0-1.fc40 packit-0.99.0-1.fc40", - sidetag=sidetag_name, - set_status=lambda x: None, - set_data=lambda x: None, - set_web_url=lambda x: None, - set_alias=lambda x: None, - set_update_creation_time=lambda x: None, - ), - ], - ) + update_model = flexmock( + target=dg_branch, + koji_nvrs="python-specfile-0.31.0-1.fc40 packit-0.99.0-1.fc40", + sidetag=sidetag_name, + set_status=lambda x: None, + set_data=lambda x: None, + set_web_url=lambda x: None, + set_alias=lambda x: None, + set_update_creation_time=lambda x: None, + ) + group_model = flexmock(grouped_targets=[update_model]) flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args( + "python-specfile-0.31.0-1.fc40 packit-0.99.0-1.fc40", + ).and_return( + {flexmock(), update_model} if non_unique_builds else {update_model}, + ) + def _create(target, koji_nvrs, sidetag, status, bodhi_update_group): assert target == dg_branch assert set(koji_nvrs.split()) == { diff --git a/tests/integration/test_issue_comment.py b/tests/integration/test_issue_comment.py index aa52999af..32a70a45e 100644 --- a/tests/integration/test_issue_comment.py +++ b/tests/integration/test_issue_comment.py @@ -495,6 +495,9 @@ def test_issue_comment_retrigger_bodhi_update_handler( ], ) flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("python-teamcity-messages.fc38").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="f38", koji_nvrs="python-teamcity-messages.fc38", @@ -502,6 +505,9 @@ def test_issue_comment_retrigger_bodhi_update_handler( status="queued", bodhi_update_group=group_model, ).and_return().once() + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("python-teamcity-messages.fc37").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="f37", koji_nvrs="python-teamcity-messages.fc37", diff --git a/tests/integration/test_pr_comment.py b/tests/integration/test_pr_comment.py index 115c89ce0..b4fd5cbde 100644 --- a/tests/integration/test_pr_comment.py +++ b/tests/integration/test_pr_comment.py @@ -2526,6 +2526,9 @@ def test_bodhi_update_retrigger_via_dist_git_pr_comment(pagure_pr_comment_added) ], ) flexmock(BodhiUpdateGroupModel).should_receive("create").and_return(group_model) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).with_args("123").and_return(set()) flexmock(BodhiUpdateTargetModel).should_receive("create").with_args( target="the_distgit_branch", koji_nvrs="123", diff --git a/tests/unit/test_bodhi_update_error_msgs.py b/tests/unit/test_bodhi_update_error_msgs.py index 35f6eb5ca..b65030d8e 100644 --- a/tests/unit/test_bodhi_update_error_msgs.py +++ b/tests/unit/test_bodhi_update_error_msgs.py @@ -13,6 +13,7 @@ from packit.exceptions import PackitException from packit_service.config import ServiceConfig +from packit_service.models import BodhiUpdateTargetModel from packit_service.worker.celery_task import CeleryTask from packit_service.worker.events import ( PullRequestCommentPagureEvent, @@ -139,6 +140,9 @@ def test_pull_request_retrigger_bodhi_update_with_koji_data( task_id=123, ), ) + flexmock(BodhiUpdateTargetModel).should_receive( + "get_all_successful_or_in_progress_by_nvrs", + ).and_return(set()) flexmock(CeleryTask).should_receive("is_last_try").and_return(True) handler = RetriggerBodhiUpdateHandler(package_config, job_config, data, flexmock()) handler.run()