Skip to content
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

handlers: add no merge policy notifications #1642

Merged
merged 1 commit into from Oct 4, 2022

Conversation

davidtwco
Copy link
Member

Inspired by discussion on Zulip.

Add a handler for issue events that checks whether a merge commit has been added to the pull request and informs the user of the project's no merge policy.

cc @Mark-Simulacrum

@davidtwco
Copy link
Member Author

This passes a cargo check but I haven't yet had the time to set up a proper environment to check it actually works, so opening as a draft.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we'd get a Github-native warning by adding "require linear history" to the branch protections on rust-lang/rust or whatever other repositories. Maybe worth experimenting with? We obviously don't actually have a linear history, so we'd need to bypass that in bors, but maybe then PRs would get warnings of some kind?

(I think this is probably still worth doing, and we might want a status check of some kind that prevents bors merges too...)

src/github.rs Outdated Show resolved Hide resolved
src/github.rs Outdated Show resolved Hide resolved
src/handlers/no_merges.rs Outdated Show resolved Hide resolved
@davidtwco
Copy link
Member Author

I wonder if we'd get a Github-native warning by adding "require linear history" to the branch protections on rust-lang/rust or whatever other repositories. Maybe worth experimenting with?

I've received feedback that GitHub will offer a "fix this conflict" button of some sort that adds a merge commit, which is immediately incorrect, so we could reduce the incidences of this happening by preventing GitHub from offering that option.

Add a handler for issue events that checks whether a merge commit has
been added to the pull request and informs the user of the project's no
merge policy.

Signed-off-by: David Wood <david.wood@huawei.com>
@Mark-Simulacrum
Copy link
Member

@davidtwco this is still marked as a draft, is that state accurate? Or are we good to go?

@davidtwco
Copy link
Member Author

@davidtwco this is still marked as a draft, is that state accurate? Or are we good to go?

I haven't taken the time to set up a development environment for this locally and actually check it does what I expect it to (just because that seems like something that I'll need to block out some time to do).

@Mark-Simulacrum
Copy link
Member

Oh. Well, feel free to if you want, but we can also merge this and then enable it (perhaps not on rust-lang/rust) and test it that way. Just let me know if you'd prefer that; it's pretty typical of triagebot PRs.

@davidtwco
Copy link
Member Author

Oh. Well, feel free to if you want, but we can also merge this and then enable it (perhaps not on rust-lang/rust) and test it that way. Just let me know if you'd prefer that; it's pretty typical of triagebot PRs.

Sorry for the delay here, we can test it after merging I think.

@Mark-Simulacrum Mark-Simulacrum marked this pull request as ready for review October 4, 2022 13:52
@Mark-Simulacrum Mark-Simulacrum merged commit a368b61 into rust-lang:master Oct 4, 2022
@davidtwco davidtwco deleted the no-merge-comments branch October 4, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants