-
Notifications
You must be signed in to change notification settings - Fork 538
ci: add workflow to listen to comments for benchmark requests #5556
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
ci: add workflow to listen to comments for benchmark requests #5556
Conversation
Code ReviewP0 - Security Issue: Hardcoded repository should be The workflow forwards to repository: westonpace/lance-bench # Should this be lancedb/lance-bench?Please confirm the intended target repository. If this is intentional for testing purposes, it should be updated to the organization repo before merge. No other issues identified. The workflow logic is straightforward and follows common patterns for cross-repo dispatch. |
Code ReviewThis PR adds a GitHub Actions workflow to forward benchmark requests from PR comments to the P1: Security Concern - Missing Permission CheckThe workflow currently forwards any comment matching Recommendation: Consider adding a permission check in this workflow before dispatching. This would:
Example check: - name: Check permissions
uses: actions/github-script@v7
with:
script: |
const { data: permission } = await github.rest.repos.getCollaboratorPermissionLevel({
owner: context.repo.owner,
repo: context.repo.repo,
username: context.payload.comment.user.login
});
const isPrAuthor = context.payload.issue.user.login === context.payload.comment.user.login;
const hasWriteAccess = ['admin', 'write'].includes(permission.permission);
if (\!isPrAuthor && \!hasWriteAccess) {
core.setFailed('User is not authorized to trigger benchmarks');
}If the permission check is intentionally handled in Minor Notes (not blocking)
Overall, the implementation is clean and serves its purpose. The only significant concern is the missing authorization check. |
Yes, currently this check is done in the lance-bench repository. I have a script there to post changes on the PR and want to re-use that script to post a "you are not authorized to request this" message. Also, I think the ownership of authorization should belong to lance-bench. This isn't a security / abuse concern for lance as it is not the repository that is doing the bulk of the work. |
|
Example: #5532 |
…format#5556) I am working on a historical database of benchmark results at https://github.com/lancedb/lance-bench I would like to be able to evaluate a PR to see if it improves performance or significantly regresses performance. To do this I am following a pattern used in the Arrow project (and others). By commenting `@bench-bot benchmark` a user (only users with write permission or PR authors) can trigger the lance-bench repo to run the lance benchmarks on the PR. It will then compare the results with its historical record and decide if the PR's results are an improvement or regression. It will then post that results diff back to the PR as a comment. This PR adds the hook to listen for comments requesting benchmarks. These comments are then forwarded to the lance-bench repository for further analysis. For more details see https://github.com/lancedb/lance-bench/blob/main/.github/workflows/comment-monitor.yml
…format#5556) I am working on a historical database of benchmark results at https://github.com/lancedb/lance-bench I would like to be able to evaluate a PR to see if it improves performance or significantly regresses performance. To do this I am following a pattern used in the Arrow project (and others). By commenting `@bench-bot benchmark` a user (only users with write permission or PR authors) can trigger the lance-bench repo to run the lance benchmarks on the PR. It will then compare the results with its historical record and decide if the PR's results are an improvement or regression. It will then post that results diff back to the PR as a comment. This PR adds the hook to listen for comments requesting benchmarks. These comments are then forwarded to the lance-bench repository for further analysis. For more details see https://github.com/lancedb/lance-bench/blob/main/.github/workflows/comment-monitor.yml
I am working on a historical database of benchmark results at https://github.com/lancedb/lance-bench
I would like to be able to evaluate a PR to see if it improves performance or significantly regresses performance. To do this I am following a pattern used in the Arrow project (and others). By commenting
@bench-bot benchmarka user (only users with write permission or PR authors) can trigger the lance-bench repo to run the lance benchmarks on the PR. It will then compare the results with its historical record and decide if the PR's results are an improvement or regression. It will then post that results diff back to the PR as a comment.This PR adds the hook to listen for comments requesting benchmarks. These comments are then forwarded to the lance-bench repository for further analysis. For more details see https://github.com/lancedb/lance-bench/blob/main/.github/workflows/comment-monitor.yml