GitLab events / parsing refactoring (#2590)

GitLab events / parsing refactoring

TODO

 Write new tests or update the old ones to cover new functionality.
 Update doc-strings where appropriate.
 Open up follow-up issues.



 We create instances of abstract classes in tests… I have implemented .event_type() for those with an assertion to check that the tests are actually running…

Comments / questions from the commits

 Manual caching of the db_project_object, db_project_event and project (currently unimplemented method in superclasses, property that caches to an attribute)
Comment from the review:
Are these supposed to be abstract methods?
Also, if they're abstract (and cached, based on the commits from blame),
why don't we just use cachedproperty after abstractmethod? Having these
return ‹None› by default »can« result in unimplemented override somewhere
down the chain.


 Leaving as is for now, both project_object and project_event are constructed at the same time via one helper function, therefore the caching can't be simply wrapped with a cached_property
 Ideally drop explicit getters and implement cached abstract properties (even though I recall some issues with type checking that, or it's not very bulletproof…)


 Event.pre_check returns True by default…

 after a discussion with @lbarcziova just adding a note why we have chose the specific default there
 log a warning
 default to False



Reconsider structuring

 forge.push.Push (respectively gitlab.push.TagPush) → forge.push.Commit or forge.push.Tag
 anitya.base → anitya.events (not used directly though, events are exposed via __init__.py as it doesn't make much sense to structure them further)
 forge.pr.Synchronize doesn't make sense, it is “some kind of an action” on the PR/MR, but Synchronize has been inspired by the GitHub itself (and it's an example of an action on the PR), but I don't have a better name… maybe even forge.pr.Change / forge.pr.Action?
 Enumerations: there are some enums in events.enums for PR/MRs, comments and FedMsg…


 Combination of both… as for the PR and comment actions they are shared by GitHub and Pagure, therefore I kept those shared actions in the events.enums, while moving the GitLab enum that's also shared by GitLab-only events to events.gitlab.enums and as for the OpenScanHub, the least disruptive change (since the enum is tied only to the task) to events.openscanhub.task.Status


 Follow OpenScanHub implementation by Maja: nests the enumerations into the event type itself, which is a possible way, but it looks a bit weird used in code


 Have a separate module (e.g. openscanhub.enum) for all of them consistently


 Going from the current change:
-        if self.event.status == OpenScanHubTaskFinishedEvent.Status.success:
+        if self.event.status == openscanhub.task.Finished.Status.success:
I'd probably prefer openscanhub.task.Status.success 👀





Fixes
Supersedes #2586 (cause GitHub is a 🫓)
Merge before/after

Reviewed-by: Laura Barcziová
Reviewed-by: Maja Massarini
Reviewed-by: Matej Focko

.github/CODEOWNERS

@@ -13,6 +13,7 @@
 # the repo. Unless a later match takes precedence
 *	@packit/the-packit-team
 
-packit_service/worker/allowlist.py		@mfocko
-packit_service/worker/helpers/testing_farm.py	@mfocko
-packit_service/worker/reporting/*		@mfocko
+packit_service/worker/allowlist.py              @mfocko
+packit_service/worker/events/*                  @mfocko
+packit_service/worker/helpers/testing_farm.py   @mfocko
+packit_service/worker/reporting/*               @mfocko

alembic/versions/c6250555a36c_.py

@@ -30,7 +30,7 @@
 
 from alembic import op
 from packit_service.constants import ALLOWLIST_CONSTANTS
-from packit_service.worker.events import InstallationEvent
+from packit_service.events import github
 
 # revision identifiers, used by Alembic.
 revision = "c6250555a36c"
@@ -224,7 +224,7 @@ def db(cls) -> PersistentDict:
 
 class RedisInstallation(RedisModel):
     table_name = "github_installation"
-    event_data: InstallationEvent
+    event_data: github.installation.Installation
 
 
 class RedisBuild(RedisModel):

packit_service/events/__init__.py

@@ -0,0 +1,32 @@
+# Copyright Contributors to the Packit project.
+# SPDX-License-Identifier: MIT
+
+from . import (
+    abstract,
+    anitya,
+    copr,
+    enums,
+    event,
+    github,
+    gitlab,
+    koji,
+    openscanhub,
+    pagure,
+    testing_farm,
+    vm_image,
+)
+
+__all__ = [
+    abstract.__name__,
+    anitya.__name__,
+    github.__name__,
+    gitlab.__name__,
+    koji.__name__,
+    openscanhub.__name__,
+    pagure.__name__,
+    copr.__name__,
+    enums.__name__,
+    event.__name__,
+    testing_farm.__name__,
+    vm_image.__name__,
+]

packit_service/events/abstract/__init__.py

@@ -0,0 +1,9 @@
+# Copyright Contributors to the Packit project.
+# SPDX-License-Identifier: MIT
+
+from . import base, comment
+
+__all__ = [
+    base.__name__,
+    comment.__name__,
+]

packit_service/events/abstract/base.py

@@ -0,0 +1,166 @@
+# Copyright Contributors to the Packit project.
+# SPDX-License-Identifier: MIT
+
+from logging import getLogger
+from typing import Optional, Union
+
+from ogr.abstract import GitProject, PullRequest
+from packit.config import PackageConfig
+
+from packit_service.config import PackageConfigGetter, ServiceConfig
+from packit_service.models import (
+    AbstractProjectObjectDbType,
+    CoprBuildTargetModel,
+    ProjectEventModel,
+    TFTTestRunTargetModel,
+    filter_most_recent_target_names_by_status,
+)
+
+from ..event import Event
+
+logger = getLogger(__name__)
+
+
+class ForgeIndependent(Event):
+    commit_sha: Optional[str]
+    project_url: str
+
+    def __init__(
+        self,
+        created_at: Optional[Union[int, float, str]] = None,
+        project_url=None,
+        pr_id: Optional[int] = None,
+        actor: Optional[str] = None,
+    ):
+        super().__init__(created_at)
+        self.project_url = project_url
+        self._pr_id = pr_id
+        self.fail_when_config_file_missing = False
+        self.actor = actor
+        self._pull_request_object = None
+
+    @property
+    def project(self):
+        if not self._project:
+            self._project = self.get_project()
+        return self._project
+
+    @property
+    def base_project(self):
+        if not self._base_project:
+            self._base_project = self.get_base_project()
+        return self._base_project
+
+    @property
+    def packages_config(self):
+        if not self._package_config_searched and not self._package_config:
+            self._package_config = self.get_packages_config()
+            self._package_config_searched = True
+        return self._package_config
+
+    def get_db_project_object(self) -> Optional[AbstractProjectObjectDbType]:
+        raise NotImplementedError()
+
+    def get_db_project_event(self) -> Optional[ProjectEventModel]:
+        raise NotImplementedError()
+
+    @property
+    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
+
+        return ServiceConfig.get_service_config().get_project(
+            url=self.project_url or self.db_project_object.project.project_url,
+        )
+
+    def get_base_project(self) -> Optional[GitProject]:
+        """Reimplement in the PR events."""
+        return None
+
+    def get_packages_config(self) -> Optional[PackageConfig]:
+        logger.debug(
+            f"Getting packages_config:\n"
+            f"\tproject: {self.project}\n"
+            f"\tbase_project: {self.base_project}\n"
+            f"\treference: {self.commit_sha}\n"
+            f"\tpr_id: {self.pr_id}",
+        )
+
+        return PackageConfigGetter.get_package_config_from_repo(
+            base_project=self.base_project,
+            project=self.project,
+            reference=self.commit_sha,
+            pr_id=self.pr_id,
+            fail_when_missing=self.fail_when_config_file_missing,
+        )
+
+    def get_all_tf_targets_by_status(
+        self,
+        statuses_to_filter_with: list[str],
+    ) -> Optional[set[tuple[str, str]]]:
+        if self.commit_sha is None:
+            return None
+
+        logger.debug(
+            f"Getting Testing Farm targets for commit sha {self.commit_sha} "
+            f"and statuses {statuses_to_filter_with}",
+        )
+        found_targets = filter_most_recent_target_names_by_status(
+            models=TFTTestRunTargetModel.get_all_by_commit_target(
+                commit_sha=self.commit_sha,
+            ),
+            statuses_to_filter_with=statuses_to_filter_with,
+        )
+        logger.debug(
+            f"Testing Farm found targets {found_targets}",
+        )
+        return found_targets
+
+    def get_all_build_targets_by_status(
+        self,
+        statuses_to_filter_with: list[str],
+    ) -> Optional[set[tuple[str, str]]]:
+        if self.commit_sha is None or self.project.repo is None:
+            return None
+
+        logger.debug(
+            f"Getting COPR build targets for commit sha {self.commit_sha} "
+            f"and statuses {statuses_to_filter_with}",
+        )
+        found_targets = filter_most_recent_target_names_by_status(
+            models=CoprBuildTargetModel.get_all_by_commit(commit_sha=self.commit_sha),
+            statuses_to_filter_with=statuses_to_filter_with,
+        )
+        logger.debug(
+            f"Builds found targets {found_targets}",
+        )
+        return found_targets
+
+    def get_non_serializable_attributes(self):
+        return [
+            *super().get_non_serializable_attributes(),
+            "fail_when_config_file_missing",
+            "_pull_request_object",
+        ]
+
+
+class Result(ForgeIndependent):
+    """
+    This class is used only as an Abstract for result events to
+    allow Steve properly filter jobs with manual trigger.
+    """
+
+    def get_packages_config(self) -> Optional[PackageConfig]:
+        if self.db_project_event and (db_config := self.db_project_event.packages_config):
+            logger.debug("Getting packages config from DB.")
+            return PackageConfig.get_from_dict_without_setting_defaults(db_config)
+        return super().get_packages_config()

packit_service/worker/events/comment.py → packit_service/events/abstract/comment.py

@@ -5,11 +5,13 @@
 abstract-comment event classes.
 """
 
+import os
 import re
 from logging import getLogger
 from typing import Optional, Union
 
-from ogr.abstract import Comment, Issue
+from ogr.abstract import Comment
+from ogr.abstract import Issue as OgrIssue
 
 from packit_service.models import (
     BuildStatus,
@@ -22,12 +24,13 @@
     AddIssueEventToDb,
     AddPullRequestEventToDb,
 )
-from packit_service.worker.events.event import AbstractForgeIndependentEvent
+
+from .base import ForgeIndependent
 
 logger = getLogger(__name__)
 
 
-class AbstractCommentEvent(AbstractForgeIndependentEvent):
+class CommentEvent(ForgeIndependent):
     def __init__(
         self,
         project_url: str,
@@ -53,7 +56,7 @@ def get_dict(self, default_dict: Optional[dict] = None) -> dict:
         return result
 
 
-class AbstractPRCommentEvent(AddPullRequestEventToDb, AbstractCommentEvent):
+class PullRequest(AddPullRequestEventToDb, CommentEvent):
     def __init__(
         self,
         pr_id: int,
@@ -79,6 +82,11 @@ def __init__(
         self._build_targets_override = build_targets_override
         self._tests_targets_override = tests_targets_override
 
+    @classmethod
+    def event_type(cls) -> str:
+        assert os.environ.get("PYTEST_VERSION"), "Should be initialized only during tests"
+        return "test.abstract.comment.PullRequest"
+
     @property
     def commit_sha(self) -> str:  # type:ignore
         # mypy does not like properties
@@ -132,7 +140,7 @@ def get_dict(self, default_dict: Optional[dict] = None) -> dict:
         return result
 
 
-class AbstractIssueCommentEvent(AddIssueEventToDb, AbstractCommentEvent):
+class Issue(AddIssueEventToDb, CommentEvent):
     def __init__(
         self,
         issue_id: int,
@@ -162,7 +170,12 @@ def __init__(
         self._tag_name = tag_name
         self._commit_sha: Optional[str] = None
         self._comment_object = comment_object
-        self._issue_object: Optional[Issue] = None
+        self._issue_object: Optional[OgrIssue] = None
+
+    @classmethod
+    def event_type(cls) -> str:
+        assert os.environ.get("PYTEST_VERSION"), "Should be initialized only during tests"
+        return "test.abstract.comment.Issue"
 
     @property
     def tag_name(self):
@@ -180,7 +193,7 @@ def commit_sha(self) -> Optional[str]:  # type:ignore
         return self._commit_sha
 
     @property
-    def issue_object(self) -> Optional[Issue]:
+    def issue_object(self) -> Optional[OgrIssue]:
         if not self._issue_object:
             self._issue_object = self.project.get_issue(self.issue_id)
         return self._issue_object
@@ -200,7 +213,7 @@ def get_dict(self, default_dict: Optional[dict] = None) -> dict:
         return result
 
 
-class CommitCommentEvent(AbstractCommentEvent):
+class Commit(CommentEvent):
     _trigger: Union[GitBranchModel, ProjectReleaseModel] = None
     _event: ProjectEventModel = None
 
@@ -226,6 +239,11 @@ def __init__(
         self._tag_name: Optional[str] = None
         self._branch: Optional[str] = None
 
+    @classmethod
+    def event_type(cls) -> str:
+        assert os.environ.get("PYTEST_VERSION"), "Should be initialized only during tests"
+        return "test.abstract.comment.Commit"
+
     @property
     def identifier(self) -> Optional[str]:
         return self.tag_name or self.branch

packit_service/events/anitya/__init__.py

@@ -0,0 +1,11 @@
+# Copyright Contributors to the Packit project.
+# SPDX-License-Identifier: MIT
+
+from . import abstract
+from .update import NewHotness, VersionUpdate
+
+__all__ = [
+    abstract.__name__,
+    NewHotness.__name__,
+    VersionUpdate.__name__,
+]