feat(github): enrich comment event body for CEL evaluation#2355
feat(github): enrich comment event body for CEL evaluation#2355theakshaypant wants to merge 1 commit intotektoncd:mainfrom
Conversation
Summary of ChangesHello @theakshaypant, 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 introduces a significant enhancement for GitHub App users by enriching the event payload for issue comment webhooks. Previously, CEL expressions evaluating conditions on commands like 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
|
🔍 PR Lint Feedback
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by enriching comment event bodies with pull request data for CEL evaluations on GitHub. The implementation is solid, and the accompanying documentation update is clear.
I've provided a couple of suggestions to improve the efficiency and clarity of the enrichment logic by avoiding a JSON marshal/unmarshal cycle. This involves using an anonymous struct for enrichment and updating the corresponding test to reflect this change. These are medium-severity suggestions aimed at improving performance and maintainability.
| eventMap, ok := tt.event.Event.(map[string]any) | ||
| assert.Assert(t, ok, "event should be a map after enrichment") | ||
|
|
||
| prData, ok := eventMap["pull_request"] | ||
| assert.Assert(t, ok, "event should contain pull_request field") | ||
| assert.Assert(t, prData != nil, "pull_request data should not be nil") | ||
|
|
||
| // Verify some PR fields are present | ||
| prMap, ok := prData.(map[string]any) | ||
| if ok { | ||
| assert.Assert(t, prMap["number"] != nil, "PR should have number field") | ||
| } |
There was a problem hiding this comment.
This change is necessary to align with the suggested refactoring in pkg/provider/github/parse_payload.go. With the suggested change to use an anonymous struct for enrichment, the type of tt.event.Event will no longer be map[string]any. The test needs to be updated to handle the enriched struct. You can marshal the event to JSON and then unmarshal it into a map to inspect its contents.
// Marshal the enriched event to check its contents as it's now a struct
enrichedBytes, err := json.Marshal(tt.event.Event)
assert.NilError(t, err)
var eventMap map[string]any
err = json.Unmarshal(enrichedBytes, &eventMap)
assert.NilError(t, err)
prData, ok := eventMap["pull_request"]
assert.Assert(t, ok, "event should contain pull_request field")
assert.Assert(t, prData != nil, "pull_request data should not be nil")
// Verify some PR fields are present
prMap, ok := prData.(map[string]any)
if ok {
assert.Assert(t, prMap["number"] != nil, "PR should have number field")
}|
@theakshaypant this is kind of changing schema of the event payload so if someone is looking at GitHub event schema they will find it different and will conflict and we specify in our docs that |
690a448 to
59c8943
Compare
Thanks for the info. Have it in a draft state as this is not ready yet. |
@chmouel WDYT? I have restricted this change to github only (and not gitlab) as github issue_comment "pull_request": {
"url": "https://api.github.com/repos/theakshaypant/akshay-pac-test-repo/pulls/62",
"html_url": "https://github.com/theakshaypant/akshay-pac-test-repo/pull/62",
"diff_url": "https://github.com/theakshaypant/akshay-pac-test-repo/pull/62.diff",
"patch_url": "https://github.com/theakshaypant/akshay-pac-test-repo/pull/62.patch",
"merged_at": null
},Did find some use cases where the issue_comment body is used for fetching the PR number but there is already a IMO overwriting the body.pull_request for issue_comment with PR details has far more usage compared to not allowing altering the event payload. Keeping this conversation restricted to github for now given the paylaod structure is completely different for gitlab and the same overwriting would not work there. |
|
|
||
| {{< hint info >}} | ||
| When matching the body payload in a Pull Request, the GitOps comments such as | ||
| **GitHub App only**: When using GitOps comments such as `/test` or `/retest` on a Pull Request, |
There was a problem hiding this comment.
Docs are hard 🥲
Will clean them up.
There was a problem hiding this comment.
Merged both the hints into a single one as it was earlier
When processing comment events on GitHub, we now enrich the event body with the complete pull request data. This allows CEL expressions to access all pull request details when evaluating conditions on comment webhooks (/test, /retest, /ok-to-test). Jira: https://issues.redhat.com/browse/SRVKP-8617 Signed-off-by: Akshay Pant <akshay.akshaypant@gmail.com>
59c8943 to
ac3faaa
Compare
|
Closing PR for now since there are some reservations about this implementation for the issue. Will reopen if necessary |
📝 Description of the Change
When processing comment events on GitHub, we now enrich the event body with the complete pull request data. This allows CEL expressions to access all pull request details when evaluating conditions on comment webhooks (/test, /retest, /ok-to-test).
👨🏻 Linked Jira
https://issues.redhat.com/browse/SRVKP-8617
🔗 Linked GitHub Issue
N/A
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.