Support Forgejo events handling by Stevejobs - #3154
Support Forgejo events handling by Stevejobs#3154centosinfra-prod-github-app[bot] merged 2 commits into
Stevejobs#3154Conversation
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 49s |
2ee1236 to
16b9f77
Compare
|
🤖 Finished Review · ✅ Success · Started 7:22 AM UTC · Completed 7:42 AM UTC Commit: |
ReviewFindingsLow
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Labels: PR adds Forgejo event handling to SteveJobs and allowlist, directly implementing Forgejo dist-git forge support. |
16b9f77 to
e837e13
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 01s |
|
[The following is OUTDATED, based on fullsend's first review] To address fullsend's review:
|
|
🤖 Finished Review · ✅ Success · Started 12:35 PM UTC · Completed 1:12 PM UTC Commit: |
|
To address fullsend's (second) review:
|
e837e13 to
f715816
Compare
|
🤖 Review · ❌ Terminated · Started 1:58 PM UTC · Ended 2:16 PM UTC Commit: |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 04s |
|
🤖 Finished Review · ✅ Success · Started 1:58 PM UTC · Completed 2:16 PM UTC Commit: |
|
For reviewers taking a look at fullsends's latest review:
|
The logic in `SteveJobs` has been modified to support Forgejo events processing.
f715816 to
1b073cd
Compare
|
🤖 Review · ❌ Terminated · Started 6:54 AM UTC · Ended 7:34 AM UTC Commit: |
|
✔️ pre-commit SUCCESS in 1m 49s |
|
recheck |
There was a problem hiding this comment.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
packit_service/worker/allowlist.py:508: [low] fail-open
All six Forgejo event types bypass allowlist authorization. This follows the existing Pagure pattern and is acceptable for dist-git, but no structural mechanism prevents upstream Forgejo events from bypassing authorization when upstream support is added.
Suggested fix: Before adding upstream Forgejo support, implement a guard (e.g., checking project_url against configured dist-git domains) to route upstream events through _check_pr_event/_check_issue_comment_event.
packit_service/worker/jobs.py:967: [low] fail-open
check_explicit_matching() assumes all Forgejo events come from dist-git. Upstream Forgejo PR comments could trigger dist-git-specific job types (koji_build, bodhi_update) if upstream support is added without refining this logic.
Suggested fix: Add a dist-git vs upstream guard when upstream Forgejo support is implemented.
packit_service/worker/jobs.py:359: [low] scope-gap
Reaction guard in process() excludes only pagure.pr.Comment from add_reaction(). Forgejo events will trigger reaction addition. Likely correct (Forgejo supports reactions) but ogr Forgejo implementation should be confirmed.
packit_service/worker/jobs.py(file-level): Line 374 · [low] scope-gap
Forgejo PR events added to Fedora CI processing guard require db_project_object. Verified correct via AddPullRequestEventToDb inheritance chain.
packit_service/worker/jobs.py:965: [low] scope-alignment
XXX comment appropriately flags the dist-git assumption for forgejo.pr.Comment in koji-build/bodhi-update re-trigger matching. Well-aligned with issue #2862 scope.
packit_service/worker/jobs.py:378: [low] ordering convention
Fedora CI isinstance check interleaves Forgejo and Pagure events by subtype rather than grouping by forge as done elsewhere.
Suggested fix: Group by forge: (forgejo.pr.Action, forgejo.pr.Comment, pagure.pr.Action, pagure.pr.Comment, ...)
packit_service/worker/jobs.py:352: [low] ordering convention
Help comment isinstance check groups by event subtype with forgejo first alphabetically. Internally consistent but departs from forge-grouping used elsewhere in the file.
|
🤖 Finished Review · ✅ Success · Started 6:54 AM UTC · Completed 7:33 AM UTC Commit: |
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 10s |
|
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 1m 54s |
ca574c7
into
packit:main
The logic in
SteveJobshas been modified to support proper Forgejo events processing. Forgejo events have also been added in theAllowlist.check_and_report()method as unchecked events.Related to #2862