-
Notifications
You must be signed in to change notification settings - Fork 153
Delete old /test help messages after a successful invocation #525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for k8s-prow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Some tests were described as "<COMMENT-TEXT> results in <BOT-ACTION>" and some were described as "<COMMENT-TEXT> in <CONTEXT> [has no effect]", but some of the latter erroneously had the word "results" in them as though that was part of the comment text.
ed19a16
to
0301d43
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: danwinship The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I'll let GH copilot review this as a resource but so far I do not have great experience with the result so feel free to just ignore whatever it says unless its a clear slam dunk. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements functionality to automatically delete old help messages from the /test
and /retest
commands after a successful test invocation, preventing spam in pull requests when users initially misuse the commands and then trigger tests correctly.
Key changes:
- Added
ShouldRespondByPruningHelp
andIsHelpComment
functions to identify when to prune help comments - Modified
handleGenericComment
to accept a comment pruner and call it when appropriate - Updated test infrastructure to validate comment pruning behavior
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
pkg/plugins/trigger/trigger.go | Modified to get comment pruner and pass it to handleGenericComment |
pkg/plugins/trigger/generic-comment.go | Added commentPruner interface and logic to prune help comments on successful triggers |
pkg/plugins/trigger/generic-comment_test.go | Added fake comment pruner and test validation for pruning behavior |
pkg/pjutil/help.go | Added functions to identify help comments and determine when to prune them |
pkg/pjutil/help_test.go | Added comprehensive tests for help comment identification and pruning logic |
Comments suppressed due to low confidence (1)
pkg/pjutil/help.go:1
- Incorrect variable used in optional test commands section. Should use
optionalTestCommands
instead ofrequiredTestCommands
.
/*
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Ship it! 😎 (I'm not sure what the "Comments suppressed due to low confidence" is even referring to? It seems like it hallucinated a non-existent incorrect comment and then explained how to fix it and then realized that it couldn't find the comment it was trying to explain how to fix?) |
If a user misuses
/test
or/retest
and gets a 50-page-long help message from prow, and then triggers a test correctly afterward, delete the help message so as to de-spam the PR.Fixes #374
I don't know this codebase at all and probably did stuff wrong. Also, I'm not sure how to actually test it.
(I assume I need to add a unit test topkg/plugins/trigger/
too, but I was having a hard time figuring out if I could actually figure out which test cases should result in comment deletion correctly just from the info already in the testcase struct, so I gave up on it for now since you might request large rewrites anyway...)/assign @petr-muller
since you 👍 'd the idea on #374...