-
Notifications
You must be signed in to change notification settings - Fork 139
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
fix(generated-files-bot): Use sender
in addition to PR user
#1759
Conversation
Part of the fix for #1751
const prAuthor = context.payload.pull_request.user.login; | ||
const sender = context.payload.sender.login; | ||
|
||
// ignore PRs from a configurable list of authors | ||
if (config.ignoreAuthors?.includes(prAuthor)) { | ||
if (config.ignoreAuthors?.includes(sender)) { |
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.
We may want to check both - consider the case where owlbot opens a PR, then I update the branch from main. I think I would be the event sender
(of pull_request.synchronize
) but the PR may be legitimately updating a generated file.
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.
Good point - what are your thoughts on handling the use case where owl-bot opens a PR, then someone edits something that shouldn't be edited? Perhaps I should refactor to capture the files that changed between commits and create messages based on those files rather than all files in the PR itself
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.
That might work, but it's worth investigating what GitHub reports for files changed on a merge from main. The case I'm thinking of is a maintainer updates the branch from main and the files it pulls in might have updated a templated file (not sure what the data looks like). In that case, it might not be possible to distinguish whether or not to comment unless we special-case ignore the updating of a branch from the pull request base branch.
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.
Diving into this, there doesn't appear to be a clean way to get a clean diff between merges/commits. I can come back to work on this functionality if desired, while checking both the prAuthor
and sender
for now
sender
in addition to PR user
Part of the fix for #1751
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #1751 🦕
Related: googleapis/synthtool#1080