Conversation
Summary of ChangesHello @mynk8, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the service's compatibility by adding full support for Forgejo, a self-hosted Git service. It enables the system to receive, parse, and react to various Forgejo events, such as pull request actions, issue comments, and code pushes, and to report statuses back to Forgejo repositories. This integration broadens the range of Git platforms the service can interact with. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary of ChangesHello @mynk8, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the platform's compatibility by adding full event support for Forgejo, a free, open-source, community-driven forge. It introduces the necessary infrastructure to listen for, parse, and react to various events from Forgejo repositories, including pull request activities, issue comments, and code pushes. This integration allows the system to seamlessly interact with projects hosted on Forgejo, mirroring the functionality already available for other Git platforms. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for Forgejo events, including a new webhook endpoint, event classes, and parsing logic. The implementation is comprehensive, covering PR actions, comments, and pushes. I've identified a couple of critical issues that will cause runtime errors, related to undefined variables in the new webhook endpoint. Additionally, there are opportunities to improve performance by passing more data during event instantiation to avoid later API calls, and to enhance robustness by using a more reliable method for distinguishing Forgejo events. I've also included some minor suggestions for code style and consistency.
|
Would like to ask, Is there an alternative to mocking the Project object from ServiceConfig? |
|
Build failed. ❌ pre-commit FAILURE in 1m 52s |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Forgejo events, including pull requests, issue comments, and pushes. It adds new event classes, a webhook endpoint, and parsers for Forgejo payloads. The implementation is comprehensive, with new logic for handling Forgejo's specific event structure and new tests to cover the functionality. My review focuses on a few critical issues, such as missing variable definitions that would cause runtime errors, and some suggestions for improving the robustness and maintainability of the event parsing logic.
|
Build failed. ❌ pre-commit FAILURE in 1m 52s |
There was a problem hiding this comment.
Code Review
This pull request introduces support for Forgejo webhooks, including events for pull requests, issues, and pushes. The implementation includes new event classes, a webhook endpoint, and parsing logic. The changes are well-structured and include comprehensive tests.
I've found a critical security vulnerability where the Forgejo webhook signature is not being validated, and another high-severity issue with incomplete validation logic. I've also pointed out a medium-severity issue regarding redundant code in the event parser.
My review comments provide suggestions to fix these issues. Once addressed, this will be a great addition.
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 55s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 58s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 55s |
|
This is probably just a nitpick, but this docstring should probably be edited to mention Forgejo webhooks, not just GitHub and GitLab. I would just add Forgejo inside the brackets so that it says: packit-service/packit_service/worker/parser.py Lines 97 to 101 in ee1c350 |
mfocko
left a comment
There was a problem hiding this comment.
Apart from the commit history and some minor issues, looks OK to me :)
| tag_name: str = "", | ||
| comment_object: Optional[Comment] = None, | ||
| dist_git_project_url=None, | ||
| commit_sha: Optional[str] = None, |
There was a problem hiding this comment.
How come this got in:
include unit tests for forgejo event; fixes
😄
| class ForgejoEvent(ForgeIndependent): | ||
| def __init__(self, project_url: str, pr_id: Optional[int] = None, **kwargs): | ||
| super().__init__(pr_id=pr_id) | ||
| super().__init__(pr_id=pr_id, **kwargs) |
There was a problem hiding this comment.
Why? Just an FYI, this can cause issues.
There was a problem hiding this comment.
that was so that I could pass actor and other parameters into the event, not anymore. corrected it.
packit_service/events/forgejo/pr.py
Outdated
| self.commit_sha_before = commit_sha_before | ||
| self.actor = actor | ||
| self.identifier = str(pr_id) | ||
| self._pr_id = pr_id | ||
| self.git_ref = None # use pr_id for checkout |
There was a problem hiding this comment.
Are you sure about removing all of these? You left the arguments in the constructor, but you do not use them.
| "description", "visibility", "followers_count", | ||
| "following_count", "starred_repos_count", "username" | ||
| } | ||
| return any(field in user_obj for field in forgejo_fields) |
There was a problem hiding this comment.
Since you're using any, I assume those are the Forgejo-unique fields?
| logger.warning("No event to process!") | ||
| return None | ||
|
|
||
| # Check if this is a Forgejo event and prioritize Forgejo parsers |
There was a problem hiding this comment.
I don't like this… This would make sense, if majority of the events would come from Forgejo instances; this way you are basically putting Forgejo at the top1, so it gets prioritised overall…
The best way would be for us to collect metrics on what we parse and sort it from most frequent to least frequent events… I'll probably open up a follow-up issue for this, it could definitely help with SLO1 and responsiveness issues we've been having recently.
Cc @packit/the-packit-team
Related to #2854
Footnotes
-
at least there's that one helper
_is_forgejo_event(), but when you get bunch of events, it will add up… ↩
There was a problem hiding this comment.
Yes, I am also confused by the need to prioritise Forgejo parsers in particular.
There was a problem hiding this comment.
The structure of webhook payload was similar to that of github, some events would mistakenly get passed as github objects, in tests as well. I think the logic is wrong to fix that and maybe determine the type of forge from the HTTP headers at service level instead? but that would break tests and I think not the best way out.
There was a problem hiding this comment.
I see. If we don't find a better way to solve this, there should probably be a comment added to explain why the Forgejo parsers are being prioritized so that it's clear in the future. @mfocko, what do you think?
There was a problem hiding this comment.
Looking at it again, I think it would be better to detect that the event in a Forgejo event in the Github parsers and handle it there. The code would be cleaner without having to prioritize Forgejo events. I'll take further look into this.
There was a problem hiding this comment.
It's possible we will be using Fedora messaging bus (instead of a webhook) to receive Forgejo events. If so, then parsing of Forgejo events will be done a bit different (similar to Pagure events). It should be possible to identify Forgejo events based on the content of event.topic inside the parsers (in case Fedora messaging bus ends up being used). Before making changes to this, I'll wait on further information on what ends up being used to receive the events.
betulependule
left a comment
There was a problem hiding this comment.
Overall, it looks good to me so far. I didn't find any other issues other than what @mfocko has pointed out already. Just regarding the tests, it would be good for consistency to add ForgejoService in this conftest.py file.
packit-service/tests/conftest.py
Lines 28 to 46 in aa1ffde
|
Build failed. ❌ pre-commit FAILURE in 1m 12s |
|
Build failed. ❌ pre-commit FAILURE in 1m 11s |
|
my bad. rebasing... |
|
Build failed. ❌ pre-commit FAILURE in 1m 10s |
|
Build failed. ❌ pre-commit FAILURE in 1m 11s |
|
Build failed. ❌ pre-commit FAILURE in 1m 09s |
|
Build failed. ❌ pre-commit FAILURE in 1m 08s |
|
pre-commit tests failing due to: and tests due to problem communicating with pagure. for |
|
recheck |
|
Build failed. ✔️ pre-commit SUCCESS in 1m 50s |
|
recheck |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 49s |
|
Build failed. ✔️ pre-commit SUCCESS in 1m 46s |
|
recheck |
|
wait, didn't notice till now but some tests are randomly failing due to Pagure returning a 503 |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 47s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 49s |
|
Hi @mynk8, thank you for all of the work. May ask you whether this is something you plan to continue working on? If not, do we have your permission to finish this PR? This functionality would be great to have. |
I'd like to help with the PR, but I've been out of touch with the codebase for a while. Please feel free to finish it if that's more convenient. Thanks! |
Thank you for responding. In that case we will continue work on this soon. If you want, you are welcome to drop in at any time before it is finished. Thanks again. |
7d5e9dc to
3a9b15c
Compare
|
❌ pre-commit FAILURE in 1m 50s |
Update packit_service/worker/reporting/reporters/base.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Update packit_service/service/api/webhooks.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Update packit_service/worker/parser.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Update packit_service/events/forgejo/pr.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> pre-commit and mypy fixes test(data): add Forgejo webhooks for tests include unit tests for forgejo event; fixes add check for prioritising Forgejo before parsing events; fixes pre-commit remove redundant entries in parser list; fix bug Apply suggestion from @gemini-code-assist[bot] Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> implement validate_token for ForgejoWebhooks; correct the pr.Action impl for Forgejo; fix a problem in parser that could have caused keynotfound errors. implement validate_token for ForgejoWebhooks; correct the pr.Action impl for Forgejo; fix a problem in parser that could have caused keynotfound errors. Implement Forgejo event handling in Packit Service.
3a9b15c to
7182065
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 49s |
TODO:
Merge before/after
RELEASE NOTES BEGIN
pr.Action,pr.Comment,issue.Comment,push.Commitin `events.RELEASE NOTES END