[Integration][GitLab] - Improve on GitOps push events #1028
Conversation
| f"Found file {file} in spec_path {spec_path} pattern, processing its entity diff" | ||
| ) | ||
|
|
||
| if file_action == FileAction.DELETED: |
There was a problem hiding this comment.
match, case here will be prettier imo
| 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", []) | ||
| ] | ||
|
|
There was a problem hiding this comment.
maybe worth using the same verb names as in the push hook.
instead of diverging between removed and deleted to use the same removed key that way in the commit.get( you could use the consts that you've created
| 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.
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.
I would rather move this outside of this method, to where we filter the each type of changed files.
There was a problem hiding this comment.
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.
So far I don't think it's spammy, we only log the relevant info that needs to be displayed. And this will be crucial for debugging
| 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.
should be debug I think 🤔
There was a problem hiding this comment.
and should be moved outside of the method
| for file in files: | ||
| try: | ||
| if does_pattern_apply(spec_path, file): | ||
|
|
…0524-git-lab-gitops-push-event-improvement
…tps://github.com/port-labs/ocean into PORT-10524-git-lab-gitops-push-event-improvement
Description
What - Improved on the way the integration handles GitOps push events by using only files that have been changed in the push even rather than fetching the entire repository tree
Why - Some customers were not receiving the push events in their GitLab GitOps
How -
Type of change
Please leave one option from the following and delete the rest:
All tests should be run against the port production environment(using a testing org).
Core testing checklist
Integration testing checklist
examplesfolder in the integration directory.Preflight checklist
Screenshots
showing successful ingestion
entity diffs operations
API Documentation
Provide links to the API documentation used for this integration.