Merge pull request #1459 from nikromen/fix-rebuild

Fix rebuild-failed

Previously /packit rebuild-failed searched in DB for models with matching commit sha and project name. Searching by project name is redundant and it seems like project names in copr and repo's project names can differ (somehow) so result of searching is sometimes empty list.
Related to #1375

RELEASE NOTES BEGIN

RELEASE NOTES END

Reviewed-by: Matej Focko <None>
Reviewed-by: Tomas Tomecek <[email protected]>

packit_service/models.py

@@ -1012,6 +1012,15 @@ def get_all_by(
             )
             return query.all()
 
+    @classmethod
+    def get_all_by_commit(
+        cls, commit_sha: str
+    ) -> Optional[Iterable["CoprBuildTargetModel"]]:
+        """Returns all builds that match a given commit sha"""
+        with get_sa_session() as session:
+            query = session.query(CoprBuildTargetModel).filter_by(commit_sha=commit_sha)
+            return query.all()
+
     @classmethod
     def create(
         cls,

packit_service/worker/events/comment.py

@@ -91,7 +91,9 @@ def comment_object(self) -> Optional[Comment]:
     def build_targets_override(self) -> Optional[Set[str]]:
         if not self._build_targets_override and "rebuild-failed" in self.comment:
             self._build_targets_override = (
-                super().get_all_build_targets_by_status([PG_BUILD_STATUS_FAILURE])
+                super().get_all_build_targets_by_status(
+                    statuses_to_filter_with=[PG_BUILD_STATUS_FAILURE]
+                )
                 or None
             )
         return self._build_targets_override
@@ -101,7 +103,10 @@ def tests_targets_override(self) -> Optional[Set[str]]:
         if not self._tests_targets_override and "retest-failed" in self.comment:
             self._tests_targets_override = (
                 super().get_all_tf_targets_by_status(
-                    [TestingFarmResult.failed, TestingFarmResult.error]
+                    statuses_to_filter_with=[
+                        TestingFarmResult.failed,
+                        TestingFarmResult.error,
+                    ]
                 )
                 or None
             )

packit_service/worker/events/event.py

@@ -490,8 +490,7 @@ def get_all_tf_targets_by_status(
             return None
 
         logger.debug(
-            f"Getting failed Testing Farm targets for repo: {self.project.repo} and "
-            f"commit sha: {self.commit_sha}"
+            f"Getting failed Testing Farm targets for commit sha: {self.commit_sha}"
         )
         return self._filter_failed_models_targets(
             models=TFTTestRunTargetModel.get_all_by_commit_target(
@@ -503,19 +502,14 @@ def get_all_tf_targets_by_status(
     def get_all_build_targets_by_status(
         self, statuses_to_filter_with: List[str]
     ) -> Optional[Set[str]]:
-        # TODO: get rid of project.repo which is mandatory in `CoprBuildTargetModel.get_all_by`
-        # in this case relevant for us is only commit_sha
         if self.commit_sha is None or self.project.repo is None:
             return None
 
         logger.debug(
-            f"Getting failed COPR build targets for repo: {self.project.repo} and "
-            f"commit sha: {self.commit_sha}"
+            f"Getting failed COPR build targets for commit sha: {self.commit_sha}"
         )
         return self._filter_failed_models_targets(
-            models=CoprBuildTargetModel.get_all_by(
-                project_name=self.project.repo, commit_sha=self.commit_sha
-            ),
+            models=CoprBuildTargetModel.get_all_by_commit(commit_sha=self.commit_sha),
             statuses_to_filter_with=statuses_to_filter_with,
         )
 

tests/integration/test_pr_comment.py

@@ -1244,12 +1244,23 @@ def test_rebuild_failed(
     flexmock(copr_build).should_receive("get_valid_build_targets").and_return(set())
 
     model = flexmock(
-        CoprBuildTargetModel, status=PG_BUILD_STATUS_FAILURE, target="target"
+        CoprBuildTargetModel, status=PG_BUILD_STATUS_FAILURE, target="some_target"
     )
-    flexmock(model).should_receive("get_all_by").and_return(flexmock())
+    flexmock(model).should_receive("get_all_by_commit").with_args(
+        commit_sha="12345"
+    ).and_return(model)
     flexmock(AbstractForgeIndependentEvent).should_receive(
         "get_all_build_targets_by_status"
-    ).and_return({"target"})
+    ).with_args(statuses_to_filter_with=[PG_BUILD_STATUS_FAILURE]).and_return(
+        {"some_target"}
+    )
+    flexmock(AbstractForgeIndependentEvent).should_receive(
+        "_filter_failed_models_targets"
+    ).with_args(
+        models=[model], statuses_to_filter_with=[PG_BUILD_STATUS_FAILURE]
+    ).and_return(
+        {"some_target"}
+    )
 
     flexmock(CoprBuildJobHelper).should_receive("report_status_to_build").with_args(
         description=TASK_ACCEPTED,
@@ -1263,7 +1274,7 @@ def test_rebuild_failed(
     event_dict, job, job_config, package_config = get_parameters_from_results(
         processing_results
     )
-    assert event_dict["build_targets_override"] == ["target"]
+    assert event_dict["build_targets_override"] == ["some_target"]
     assert json.dumps(event_dict)
 
     results = run_copr_build_handler(
@@ -1319,12 +1330,26 @@ def test_retest_failed(
     )
 
     model = flexmock(
-        TFTTestRunTargetModel, status=TestingFarmResult.failed, target="tf_target-arch"
+        TFTTestRunTargetModel, status=TestingFarmResult.failed, target="some_tf_target"
     )
-    flexmock(model).should_receive("get_all_by_commit_target").and_return(flexmock())
+    flexmock(model).should_receive("get_all_by_commit_target").with_args(
+        commit_sha="12345"
+    ).and_return(model)
     flexmock(AbstractForgeIndependentEvent).should_receive(
         "get_all_tf_targets_by_status"
-    ).and_return({"tf_target-arch"})
+    ).with_args(
+        statuses_to_filter_with=[TestingFarmResult.failed, TestingFarmResult.error]
+    ).and_return(
+        {"some_tf_target"}
+    )
+    flexmock(AbstractForgeIndependentEvent).should_receive(
+        "_filter_failed_models_targets"
+    ).with_args(
+        models=[model],
+        statuses_to_filter_with=[TestingFarmResult.failed, TestingFarmResult.error],
+    ).and_return(
+        {"some_target"}
+    )
 
     flexmock(Pushgateway).should_receive("push").twice().and_return()
     flexmock(CoprBuildJobHelper).should_receive("report_status_to_tests").with_args(
@@ -1337,7 +1362,7 @@ def test_retest_failed(
     event_dict, job, job_config, package_config = get_parameters_from_results(
         processing_results
     )
-    assert event_dict["tests_targets_override"] == ["tf_target-arch"]
+    assert event_dict["tests_targets_override"] == ["some_tf_target"]
     assert json.dumps(event_dict)
 
     run_testing_farm_handler(

tests_openshift/database/test_models.py

@@ -352,6 +352,20 @@ def test_copr_get_all_by_owner_project_commit_target(
     )
 
 
+def test_copr_get_all_by_commit(clean_before_and_after, multiple_copr_builds):
+    builds_list = list(
+        CoprBuildTargetModel.get_all_by_commit(commit_sha=SampleValues.ref)
+    )
+    assert len(builds_list) == 3
+    # they should have the same project_name
+    assert (
+        builds_list[0].project_name
+        == builds_list[1].project_name
+        == builds_list[2].project_name
+        == SampleValues.project
+    )
+
+
 def test_multiple_pr_models(clean_before_and_after):
     pr1 = PullRequestModel.get_or_create(
         pr_id=1,