chore: return early from detect for edited comments#2674
chore: return early from detect for edited comments#2674chmouel merged 2 commits intotektoncd:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors GitHub event handling by moving the "created" action validation to the detection phase and removing it from the payload processing logic. Feedback indicates that the new early returns in detect.go render subsequent action checks redundant, which should be removed to improve clarity. Furthermore, rather than deleting test cases for unsupported actions, they should be updated to assert the expected failure reasons and maintain test coverage.
| if event.GetAction() == "created" && | ||
| event.GetIssue().IsPullRequest() && | ||
| event.GetIssue().GetState() == "open" { |
There was a problem hiding this comment.
The check event.GetAction() == "created" is redundant here because the function already returns early if the action is not "created" on lines 79-81.
if event.GetIssue().IsPullRequest() &&
event.GetIssue().GetState() == "open" {| return "", fmt.Sprintf("commit_comment: unsupported action \"%s\"", event.GetAction()) | ||
| } | ||
|
|
||
| if event.GetAction() == "created" { |
There was a problem hiding this comment.
The check if event.GetAction() == "created" is redundant because of the early return on lines 108-110. This if block and its corresponding closing brace (on line 123) should be removed, and the inner logic unindented to improve code clarity and maintainability.
| isGH: true, | ||
| processReq: true, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Several test cases that verified the behavior of non-"created" actions (such as "deleted" or "something") were removed in this PR. Instead of deleting them, these tests should be updated to assert that processReq is now false and that the correct wantReason is returned. This ensures that the new early return logic in the Detect function is properly covered by the test suite and prevents future regressions.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2674 +/- ##
=======================================
Coverage 58.86% 58.86%
=======================================
Files 206 206
Lines 20329 20330 +1
=======================================
+ Hits 11967 11968 +1
Misses 7589 7589
Partials 773 773 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
when edited comment event is recieved, we used to return in ParsePayload which is quite late because we don't wanna entertain that event for issue_comment and commit_comment so this commit makes it return early in detect func. Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
zakisk
force-pushed
the
return-early-in-detect-for-edited-event
branch
from
9c1a3a1 to
f852d66
Compare
|
i'll be honest with you i don't understand, what is getting fix here? |
|
is it this bug? #2652 |
🤖 AI Analysis - pr-complexity-ratingAs a code review triage assistant, I have analyzed the provided context. Please note that since the specific file diff was not provided, this assessment is based on the PR metadata and branch context. 📊 PR Review Complexity
Overall difficulty: Easy SummaryThis PR implements an "early return" pattern within the detection logic for edited events. Reviewers should verify that the early exit condition does not inadvertently skip necessary downstream processing or side effects required for pipeline execution. Suggested reviewers focus
Generated by Pipelines-as-Code LLM Analysis |
🤖 AI Analysis - pr-complexity-ratingTo provide an accurate assessment, I have analyzed the provided metadata. Note: As the specific file diff was not included in the payload, this assessment is based on the PR title ("return-early-in-detect-for-edited-event") and the nature of the merge commit. 📊 PR Review Complexity
Overall difficulty: Easy SummaryThis PR implements an early return in the event detection logic for the Suggested reviewers focus
Generated by Pipelines-as-Code LLM Analysis |
may be implicitly 🙂 . but I didn't see that issue rather when I saw that edited comments are rejected from parse_payload.go I thought ParsePayload func is not the place to filter the event rather its detect.go for every provider which stops events early without making noise. and I think the reason of noise is the same in #2652 |
3 participants
when edited comment event is recieved, we used to return in ParsePayload which is quite late because we don't wanna entertain that event for issue_comment and commit_comment so this commit makes it return early in detect func.
📝 Description of the Change
🔗 Linked GitHub Issue
Fixes #
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
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: Claude noreply@anthropic.com
✅ 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.