Review comments and code cleanup

.gitignore

@@ -10,3 +10,4 @@ secrets
 .mypy_cache/
 /build/
 __pycache__/
+files/python3-ogr.rpm

files/docker/Dockerfile

@@ -4,10 +4,10 @@ FROM quay.io/packit/base:fedora
 
 ARG SOURCE_BRANCH
 RUN  if [[ -z $SOURCE_BRANCH ]]; then \
-echo -e "\nMissing SOURCE_BRANCH build argument! Please add \
-\"--build-arg SOURCE_BRANCH=<val>\" to the build command to specify it!\n\
-This is the branch used when installing other Packit projects (e.g. ogr, packit).\n" && exit 1;\
-fi
+    echo -e "\nMissing SOURCE_BRANCH build argument! Please add \
+    \"--build-arg SOURCE_BRANCH=<val>\" to the build command to specify it!\n\
+    This is the branch used when installing other Packit projects (e.g. ogr, packit).\n" && exit 1;\
+    fi
 
 ENV USER=packit \
     HOME=/home/packit
@@ -23,6 +23,7 @@ COPY setup.py setup.cfg ./
 COPY .git ./.git
 COPY packit_service/ ./packit_service/
 
+
 RUN git rev-parse HEAD >/.packit-service.git.commit.hash \
     && git show --quiet --format=%B HEAD >/.packit-service.git.commit.message \
     && ansible-playbook -vv -c local -i localhost, files/recipe.yaml
@@ -32,6 +33,7 @@ RUN git rev-parse HEAD >/.packit-service.git.commit.hash \
 COPY alembic.ini ./
 COPY alembic/ ./alembic/
 
+
 EXPOSE 8443
 
-CMD ["/usr/bin/run_httpd.sh"]
+CMD ["/usr/bin/run_httpd.sh"]

files/install-deps-worker.yaml

@@ -53,10 +53,10 @@
           - git+https://github.com/packit/sandcastle.git@{{ source_branch }}
           - eventlet
           - celery
-#    - name: Install guppy3 for memory profiling in non production environments
-#      ansible.builtin.pip:
-#        name:
-#          - guppy3
+      #    - name: Install guppy3 for memory profiling in non production environments
+      #      ansible.builtin.pip:
+      #        name:
+      #          - guppy3
       when: deployment != 'prod'
     - name: Check if all pip packages have all dependencies installed
       command: pip check

files/install-deps.yaml

@@ -48,4 +48,4 @@
           - python3-ogr
           - python3-specfile
           - packit
-        state: latest
+        state: latest

files/run_httpd.sh

@@ -2,8 +2,7 @@
 
 set -eux
 
-# if all containers started at the same time, pg is definitely not ready to serve
-# so let's try this for a few times
+export PACKIT_SERVICE_CONFIG="${HOME}/.config/packit-service.yaml"
 ATTEMPTS=7
 n=0
 while [[ $n -lt $ATTEMPTS ]]; do
@@ -18,11 +17,6 @@ if [[ $n -eq $ATTEMPTS ]]; then
     echo "Migration failed after $ATTEMPTS attempts. Exiting."
     exit 1
 fi
-
-export PACKIT_SERVICE_CONFIG="${HOME}/.config/packit-service.yaml"
-SERVER_NAME=$(sed -nr 's/^server_name: ([^:]+)(:([0-9]+))?$/\1/p' "$PACKIT_SERVICE_CONFIG")
-HTTPS_PORT=$(sed -nr 's/^server_name: ([^:]+)(:([0-9]+))?$/\3/p' "$PACKIT_SERVICE_CONFIG")
-
 # See "mod_wsgi-express-3 start-server --help" for details on
 # these options, and the configuration documentation of mod_wsgi:
 # https://modwsgi.readthedocs.io/en/master/configuration.html
@@ -32,4 +26,3 @@ exec gunicorn \
   --certfile /secrets/fullchain.pem \
   --keyfile /secrets/privkey.pem \
   packit_service.service.app:application
-

files/scripts/webhook.py

@@ -11,10 +11,11 @@
 from typing import Union
 
 import click
-import ogr
 import requests
 from github.PullRequest import PullRequest
 
+import ogr
+
 
 class PRWebhookPayload:
     """

packit_service/config.py

@@ -42,8 +42,7 @@ class ProjectToSync(NamedTuple):
 
     def __repr__(self):
         return (
-            f"ProjectToSync(forge={self.forge}, repo_namespace={
-                self.repo_namespace}, "
+            f"ProjectToSync(forge={self.forge}, repo_namespace={self.repo_namespace}, "
             f"repo_name={self.repo_name}, branch={self.branch}, "
             f"dg_repo_name={self.dg_repo_name}, dg_branch={self.dg_branch})"
         )
@@ -91,25 +90,21 @@ def __init__(
         validate_webhooks: bool = True,
         admins: Optional[list] = None,
         fas_password: Optional[str] = "",
-        enabled_private_namespaces: Optional[Union[set[str],
-                                                   list[str]]] = None,
+        enabled_private_namespaces: Optional[Union[set[str], list[str]]] = None,
         gitlab_token_secret: str = "",
         gitlab_mr_targets_handled: Optional[list[MRTarget]] = None,
         projects_to_sync: Optional[list[ProjectToSync]] = None,
-        enabled_projects_for_internal_tf: Optional[Union[set[str],
-                                                         list[str]]] = None,
+        enabled_projects_for_internal_tf: Optional[Union[set[str], list[str]]] = None,
         dashboard_url: str = "",
         koji_logs_url: str = "https://kojipkgs.fedoraproject.org",
         koji_web_url: str = "https://koji.fedoraproject.org",
-        enabled_projects_for_srpm_in_copr: Optional[Union[set[str],
-                                                          list[str]]] = None,
+        enabled_projects_for_srpm_in_copr: Optional[Union[set[str], list[str]]] = None,
         comment_command_prefix: str = "/packit",
         redhat_api_refresh_token: Optional[str] = None,
         package_config_path_override: Optional[str] = None,
         command_handler_storage_class: Optional[str] = None,
         appcode: Optional[str] = None,
-        enabled_projects_for_fedora_ci: Optional[Union[set[str],
-                                                       list[str]]] = None,
+        enabled_projects_for_fedora_ci: Optional[Union[set[str], list[str]]] = None,
         forgejo_instance_url: Optional[str] = None,
         forgejo_api_key: Optional[str] = None,
         forgejo_namespace: Optional[str] = None,
@@ -159,8 +154,7 @@ def __init__(
 
         # e.g.:
         #  - https://src.fedoraproject.org/rpms/packit
-        self.enabled_projects_for_fedora_ci: set[str] = set(
-            enabled_projects_for_fedora_ci or [])
+        self.enabled_projects_for_fedora_ci: set[str] = set(enabled_projects_for_fedora_ci or [])
 
         self.projects_to_sync = projects_to_sync or []
 
@@ -204,29 +198,23 @@ def hide(token: str) -> str:
             f"webhook_secret='{hide(self.webhook_secret)}', "
             f"testing_farm_secret='{hide(self.testing_farm_secret)}', "
             f"testing_farm_api_url='{self.testing_farm_api_url}', "
-            f"internal_testing_farm_secret='{
-                hide(self.internal_testing_farm_secret)}', "
+            f"internal_testing_farm_secret='{hide(self.internal_testing_farm_secret)}', "
             f"validate_webhooks='{self.validate_webhooks}', "
             f"admins='{self.admins}', "
             f"fas_password='{hide(self.fas_password)}', "
             f"gitlab_token_secret='{hide(self.gitlab_token_secret)}',"
             f"gitlab_mr_targets_handled='{self.gitlab_mr_targets_handled}', "
             f"enabled_private_namespaces='{self.enabled_private_namespaces}', "
-            f"enabled_projects_for_internal_tf='{
-                self.enabled_projects_for_internal_tf}', "
+            f"enabled_projects_for_internal_tf='{self.enabled_projects_for_internal_tf}', "
             f"server_name='{self.server_name}', "
             f"dashboard_url='{self.dashboard_url}', "
             f"koji_logs_url='{self.koji_logs_url}', "
             f"koji_web_url='{self.koji_web_url}', "
-            f"enabled_projects_for_srpm_in_copr= '{
-                self.enabled_projects_for_srpm_in_copr}', "
+            f"enabled_projects_for_srpm_in_copr= '{self.enabled_projects_for_srpm_in_copr}', "
             f"comment_command_prefix='{self.comment_command_prefix}', "
-            f"redhat_api_refresh_token='{
-                hide(self.redhat_api_refresh_token)}', "
-            f"package_config_path_override='{
-                self.package_config_path_override}', "
-            f"enabled_projects_for_fedora_ci='{
-                self.enabled_projects_for_fedora_ci}')"
+            f"redhat_api_refresh_token='{hide(self.redhat_api_refresh_token)}', "
+            f"package_config_path_override='{self.package_config_path_override}', "
+            f"enabled_projects_for_fedora_ci='{self.enabled_projects_for_fedora_ci}')"
             f"forgejo_instance_url='{self.forgejo_instance_url}', "
             f"forgejo_api_key='{hide(self.forgejo_api_key)}', "
         )
@@ -284,11 +272,9 @@ def get_service_config(cls) -> "ServiceConfig":
                     loaded_config = safe_load(file_stream)
             except Exception as ex:
                 logger.error(f"Cannot load service config '{config_file}'.")
-                raise PackitException(
-                    f"Cannot load service config: {ex}.") from ex
+                raise PackitException(f"Cannot load service config: {ex}.") from ex
 
-            cls.service_config = ServiceConfig.get_from_dict(
-                raw_dict=loaded_config)
+            cls.service_config = ServiceConfig.get_from_dict(raw_dict=loaded_config)
         return cls.service_config
 
     def get_project_to_sync(self, dg_repo_name, dg_branch) -> Optional[ProjectToSync]:

packit_service/events/__init__.py

@@ -10,6 +10,7 @@
     github,
     gitlab,
     koji,
+    forgejo,
     openscanhub,
     pagure,
     testing_farm,
@@ -22,6 +23,7 @@
     github.__name__,
     gitlab.__name__,
     koji.__name__,
+    forgejo.__name__,
     openscanhub.__name__,
     pagure.__name__,
     copr.__name__,

packit_service/events/abstract/base.py

@@ -4,9 +4,9 @@
 from logging import getLogger
 from typing import Optional, Union
 
-from ogr.abstract import GitProject, PullRequest
 from packit.config import PackageConfig
 
+from ogr.abstract import GitProject, PullRequest
 from packit_service.config import ServiceConfig
 from packit_service.models import (
     AbstractProjectObjectDbType,

packit_service/events/abstract/comment.py

@@ -51,7 +51,7 @@ def comment_object(self) -> Optional[Comment]:
 
     def get_dict(self, default_dict: Optional[dict] = None) -> dict:
         result = super().get_dict()
-        result.pop("_comment_object")
+        result.pop("_comment_object", None)  # Safe pop in case it was already excluded
         return result
 
 

packit_service/events/copr.py

@@ -7,7 +7,6 @@
 
 from ogr.abstract import GitProject
 from ogr.services.pagure import PagureProject
-
 from packit_service.constants import COPR_SRPM_CHROOT
 from packit_service.models import (
     AbstractProjectObjectDbType,

packit_service/events/event.py

@@ -11,9 +11,9 @@
 from logging import getLogger
 from typing import Optional, Union
 
-from ogr.abstract import GitProject
 from packit.config import JobConfigTriggerType, PackageConfig
 
+from ogr.abstract import GitProject
 from packit_service.models import (
     AbstractProjectObjectDbType,
     AnityaProjectModel,
@@ -93,6 +93,8 @@ def make_serializable(d: dict, skip: list) -> dict:
         This method will copy everything from dict except the specified
         non serializable keys.
         """
+        logger.debug(f"Making serializable: {d.items()}")
+        logger.debug(f"Skipping attributes: {skip}")
         return {k: copy.deepcopy(v) for k, v in d.items() if k not in skip}
 
     def store_packages_config(self):
@@ -129,7 +131,9 @@ def get_non_serializable_attributes(self):
     def get_dict(self, default_dict: Optional[dict] = None) -> dict:
         d = default_dict or self.__dict__
         # whole dict has to be JSON serializable because of redis
-        d = self.make_serializable(d, self.get_non_serializable_attributes())
+        skip_attrs = self.get_non_serializable_attributes()
+        logger.debug(f"Event class: {self.__class__.__name__}, Skip attrs: {skip_attrs}")
+        d = self.make_serializable(d, skip_attrs)
         d["event_type"] = self.event_type()
 
         # we are trying to be lazy => don't touch database if it is not needed

packit_service/events/event_data.py

@@ -8,7 +8,6 @@
 
 from ogr.abstract import GitProject
 from ogr.parsing import RepoUrl
-
 from packit_service.config import ServiceConfig
 from packit_service.models import (
     AbstractProjectObjectDbType,

packit_service/events/forgejo/__init__.py

@@ -1,8 +1,6 @@
-from . import abstract, push, issue,pr
+# Copyright Contributors to the Packit project.
+# SPDX-License-Identifier: MIT
 
-__all__ = [
-    abstract.__name__,
-    push.__name__,
-    issue.__name__,
-    pr.__name__
-]
+from . import abstract, issue, pr, push
+
+__all__ = [abstract.__name__, push.__name__, issue.__name__, pr.__name__]

packit_service/events/forgejo/abstract.py

@@ -3,7 +3,6 @@
 
 from typing import Optional
 
-from ogr.abstract import GitProject
 from ..abstract.base import ForgeIndependent
 
 
@@ -15,4 +14,4 @@ def __init__(self, project_url: str, pr_id: Optional[int] = None, **kwargs):
         self.git_ref: Optional[str] = None
         self.identifier: Optional[str] = (
             None  # will be shown to users -- e.g. in logs or in the copr-project name
-        )
+        )

packit_service/events/forgejo/issue.py

@@ -1,3 +1,6 @@
+# Copyright Contributors to the Packit project.
+# SPDX-License-Identifier: MIT
+
 # SPDX-License-Identifier: MIT
 
 from typing import Optional
@@ -25,6 +28,7 @@ def __init__(
         base_ref: Optional[str] = "main",
         comment_object: Optional[OgrComment] = None,
         dist_git_project_url=None,
+        commit_sha: Optional[str] = None,
     ) -> None:
         super().__init__(
             issue_id=issue_id,
@@ -55,26 +59,17 @@ def tag_name(self):
         """
         return self._tag_name
 
-    @property
-    def commit_sha(self) -> Optional[str]:
-        """
-        For Forgejo issue comments, return the commit_sha passed in constructor
-        without making API calls to avoid authentication issues.
-        """
-        return self._commit_sha
-
     def get_dict(self, default_dict: Optional[dict] = None) -> dict:
         """
         Override get_dict to avoid accessing properties that make API calls.
         """
         # Get the basic dict from CommentEvent, not from Issue to avoid tag_name access
         from ..abstract.comment import CommentEvent
+
         result = CommentEvent.get_dict(self, default_dict=default_dict)
-        
+
         # Add the specific fields we need without triggering API calls
         result["action"] = self.action.value
         result["issue_id"] = self.issue_id
-        result["tag_name"] = self._tag_name  # Use the private attribute directly
-        result["commit_sha"] = self._commit_sha  # Use the private attribute directly
-
+
         return result