-
Notifications
You must be signed in to change notification settings - Fork 77
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
Add github-action PR open/closer #1866
Conversation
388cb24
to
3558ef1
Compare
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.
Left a few comments. Thanks a lot for implementing this!
src/handlers/bot_pull_requests.rs
Outdated
|
||
// If it's not the github-actions bot, we don't expect this handler to be needed. Skip the | ||
// event. | ||
if event.sender.login != "github-actions" { |
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.
I'm not sure if this will work, an user with such login doesn't seem to exist? https://github.com/github-actions
I tried to run this on rust-lang/miri#4070:
It looks like the login is app/github-actions
. Ideally we should also check if it's a bot.
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.
Hm, I'm not sure if we get is_bot = true in the sender
field, I'd have to go look at events we've seen. I've updated to app/github-actions, and we can revisit if this doesn't work in practice IMO.
src/handlers/bot_pull_requests.rs
Outdated
// Sanity check that our logic above doesn't cause us to act on PRs in a loop, by | ||
// tracking a window of PRs we've acted on. We can probably drop this if we don't see problems | ||
// in the first few days/weeks of deployment. | ||
{ |
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.
I think/hope that this logic should be unnecessary due to the difference between the opened/reopened events.
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.
Yeah, I missed that those are different - I think I'll go ahead and delete the extra logic since it's not trivial then.
3558ef1
to
68280d8
Compare
I tested this locally on my bors-kindergarten repo using |
68280d8
to
e8d017a
Compare
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.
Thanks!
This isn't tested, but my hope is that it works ~first try or after small tweaks. I intentionally made it relatively safe (I hope) in terms of not falling into retry loops calling itself, but open to either dropping that or discussing alternatives.
cc @RalfJung
Closes #1864
Sample PR: rust-lang/miri#4081