-
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
Don't ping everyone on rebase mistakes #1672
base: master
Are you sure you want to change the base?
Conversation
src/handlers/mentions.rs
Outdated
.into_iter() | ||
.any(|commit| commit.commit.author.name == "bors") | ||
{ | ||
return Ok(Some(MentionsInput::HasBorsCommit)); |
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 this will cause us to warn on clippy and Miri subtree bumps since those also use bors.
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, seems like this isn't the most unreasonable behavior for that case though. The PR author could of course ping manually, but I also don't think that it's even necessary in case of a subtree bump
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.
It does seem just as relevant to ping on a subtree bump if it touches code outside the subtree (as is not uncommon, I believe, though maybe not particularly desirable).
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.
Oh, I did not know that was possible. How does that happen?
I'll update in that case
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.
rust-lang/rust#101907 (comment) would also help here -- it would mean we could distinguish between Miri-bors commits and rustc-bors commits.
src/handlers/mentions.rs
Outdated
None => write!(result, "Some changes occurred in {to_mention}").unwrap(), | ||
match input { | ||
MentionsInput::HasBorsCommit => { | ||
result = config.bors_commit_message.to_string(); |
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 this doesn't provide a way to opt-out and get the real message (e.g., if you are pulling in miri or clippy, and making changes as part of the merge to other files).
I think one option is to escape all of the mentions and still post the message - that gives the author the ability to re-ping the right folks. Though only if they're a member of the org for teams.
I think a better option is to detect the bors merge commits and then set the PR as draft and bail out. (Probably leaving a comment is still reasonable). That means that the author can just close the PR if they don't want to ping folks or fix it and then mark as ready (which should already retrigger the logic if I'm remembering right).
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 seems quite annoying for the cases in which this is intentional - the current version will warn but at least it doesn't require the PR author to take any additional steps if that's what they intended to do
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.
The additional step is clicking in the UI on ready for review - that feels pretty lightweight to me? It's also not strictly required - e.g., bors will listen to you in draft state iirc.
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.
Hmm, looking at the github API, I don't think we can mark someone else's pull request as being a draft. Escaping the pings is possible though, I'll update
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.
It seems like the conversion is possible, just needs graphql rather than the rest API.
https://docs.github.com/en/graphql/reference/mutations#convertpullrequesttodraft
9f8c61b
to
24668d2
Compare
24668d2
to
379832d
Compare
@@ -109,6 +132,11 @@ pub(super) async fn handle_input( | |||
None => write!(result, "Some changes occurred in {to_mention}").unwrap(), | |||
} | |||
if !cc.is_empty() { | |||
let cc: Vec<String> = if input.has_bors_commit { | |||
cc.iter().map(|s| format!("`{s}`")).collect() |
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.
On Zulip there was a proposal to also mark the PR as draft, to prevent it from being merge.
Ideally we'd want a scheme where the pings definitely happen when the PR gets approved or lands, but I am not sure if that is possible.
EDIT: Ah I saw that was already discussed and doesn't seem possible. Bummer.
let has_bors_commit = event.action == IssuesAction::Opened | ||
&& config.bors_commit_message.is_some() | ||
&& event | ||
.issue | ||
.commits(&ctx.github) | ||
.await | ||
.map_err(|e| log::error!("failed to fetch commits: {:?}", e)) | ||
.unwrap_or_default() | ||
.into_iter() | ||
.any(|commit| commit.commit.author.name == "bors"); |
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.
Does this even work for problematic rebases? As someone pointed out on Zulip, https://github.com/rust-lang/rust/pull/103918/commits does not have any bors-authored commits.
assert!(config.paths.len() == 1); | ||
assert_eq!( | ||
config.bors_commit_message.as_deref(), | ||
Some("has bors commit") |
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.
How could this middle test possibly fail?
This test should be over the overall output, not the config system
See Zulip discussion here. Instead of pinging everyone, we'll send a helpful message with advice on how to correctly rebase.