-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Integration][GitLab] - Improve on GitOps push events #1028
Changes from 5 commits
69f29f8
ad772f1
4796f31
64f5710
40b09e7
de16d81
6f80375
8e7457c
1cc82dd
8683ab5
2c14532
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
import typing | ||
from typing import Any | ||
from enum import StrEnum | ||
|
||
from loguru import logger | ||
from gitlab.v4.objects import Project | ||
|
||
from gitlab_integration.core.utils import generate_ref | ||
from gitlab_integration.core.utils import generate_ref, does_pattern_apply | ||
from gitlab_integration.events.hooks.base import ProjectHandler | ||
from gitlab_integration.git_integration import GitlabPortAppConfig | ||
from gitlab_integration.utils import ObjectKind | ||
|
@@ -14,36 +15,84 @@ | |
from port_ocean.context.ocean import ocean | ||
|
||
|
||
class FileAction(StrEnum): | ||
DELETED = "deleted" | ||
ADDED = "added" | ||
MODIFIED = "modified" | ||
|
||
|
||
class PushHook(ProjectHandler): | ||
events = ["Push Hook"] | ||
system_events = ["push"] | ||
|
||
async def _on_hook(self, body: dict[str, Any], gitlab_project: Project) -> None: | ||
before, after, ref = body.get("before"), body.get("after"), body.get("ref") | ||
commit_before, commit_after, ref = ( | ||
body.get("before"), | ||
body.get("after"), | ||
body.get("ref"), | ||
) | ||
|
||
if before is None or after is None or ref is None: | ||
if commit_before is None or commit_after is None or ref is None: | ||
raise ValueError( | ||
"Invalid push hook. Missing one or more of the required fields (before, after, ref)" | ||
) | ||
|
||
added_files = [ | ||
added_file | ||
for commit in body.get("commits", []) | ||
for added_file in commit.get("added", []) | ||
] | ||
modified_files = [ | ||
modified_file | ||
for commit in body.get("commits", []) | ||
for modified_file in commit.get("modified", []) | ||
] | ||
|
||
removed_files = [ | ||
removed_file | ||
for commit in body.get("commits", []) | ||
for removed_file in commit.get("removed", []) | ||
] | ||
|
||
config: GitlabPortAppConfig = typing.cast( | ||
GitlabPortAppConfig, event.port_app_config | ||
) | ||
|
||
branch = config.branch or gitlab_project.default_branch | ||
|
||
if generate_ref(branch) == ref: | ||
entities_before, entities_after = ( | ||
await self.gitlab_service.get_entities_diff( | ||
gitlab_project, config.spec_path, before, after, branch | ||
) | ||
) | ||
spec_path = config.spec_path | ||
if not isinstance(spec_path, list): | ||
spec_path = [spec_path] | ||
|
||
# update the entities diff found in the `config.spec_path` file the user configured | ||
await ocean.update_diff( | ||
{"before": entities_before, "after": entities_after}, | ||
UserAgentType.gitops, | ||
await self._process_files( | ||
gitlab_project, | ||
removed_files, | ||
spec_path, | ||
commit_before, | ||
"", | ||
branch, | ||
FileAction.DELETED, | ||
) | ||
await self._process_files( | ||
gitlab_project, | ||
added_files, | ||
spec_path, | ||
"", | ||
commit_after, | ||
branch, | ||
FileAction.ADDED, | ||
) | ||
await self._process_files( | ||
gitlab_project, | ||
modified_files, | ||
spec_path, | ||
commit_before, | ||
commit_after, | ||
branch, | ||
FileAction.MODIFIED, | ||
) | ||
|
||
# update information regarding the project as well | ||
logger.info( | ||
f"Updating project information after push hook for project {gitlab_project.path_with_namespace}" | ||
|
@@ -52,8 +101,78 @@ async def _on_hook(self, body: dict[str, Any], gitlab_project: Project) -> None: | |
gitlab_project | ||
) | ||
await ocean.register_raw(ObjectKind.PROJECT, [enriched_project]) | ||
|
||
else: | ||
logger.debug( | ||
f"Skipping push hook for project {gitlab_project.path_with_namespace} because the ref {ref} " | ||
f"does not match the branch {branch}" | ||
) | ||
|
||
async def _process_files( | ||
self, | ||
gitlab_project: Project, | ||
files: list[str], | ||
spec_path: list[str], | ||
commit_before: str, | ||
commit_after: str, | ||
branch: str, | ||
file_action: FileAction, | ||
) -> None: | ||
if not files: | ||
return | ||
logger.info( | ||
f"Processing {file_action} files {files} for project {gitlab_project.path_with_namespace}" | ||
) | ||
|
||
for file in files: | ||
try: | ||
if does_pattern_apply(spec_path, file): | ||
logger.info( | ||
f"Found file {file} in spec_path {spec_path} pattern, processing its entity diff" | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why would we want to pass all the files that have been added / removed / modified to this function and to this log. it can be very spammy and irrelevant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed it from the loop. i see we are already logging the file info in the method where we get the entities from the commit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. redundant |
||
if file_action == FileAction.DELETED: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. match, case here will be prettier imo |
||
entities_before = ( | ||
await self.gitlab_service._get_entities_by_commit( | ||
gitlab_project, file, commit_before, branch | ||
) | ||
) | ||
await ocean.update_diff( | ||
{"before": entities_before, "after": []}, | ||
UserAgentType.gitops, | ||
) | ||
|
||
elif file_action == FileAction.ADDED: | ||
entities_after = ( | ||
await self.gitlab_service._get_entities_by_commit( | ||
gitlab_project, file, commit_after, branch | ||
) | ||
) | ||
await ocean.update_diff( | ||
{"before": [], "after": entities_after}, | ||
UserAgentType.gitops, | ||
) | ||
|
||
elif file_action == FileAction.MODIFIED: | ||
entities_before = ( | ||
await self.gitlab_service._get_entities_by_commit( | ||
gitlab_project, file, commit_before, branch | ||
) | ||
) | ||
entities_after = ( | ||
await self.gitlab_service._get_entities_by_commit( | ||
gitlab_project, file, commit_after, branch | ||
) | ||
) | ||
await ocean.update_diff( | ||
{"before": entities_before, "after": entities_after}, | ||
UserAgentType.gitops, | ||
) | ||
else: | ||
logger.info( | ||
f"Skipping file {file} as it does not match the spec_path pattern {spec_path}" | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be debug I think 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and should be moved outside of the method |
||
except Exception as e: | ||
logger.error( | ||
f"Error processing file {file} in action {file_action}: {str(e)}" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe worth using the same verb names as in the push hook.
instead of diverging between
removed
anddeleted
to use the sameremoved
key that way in thecommit.get(
you could use the consts that you've created