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

Requiring status checks before merging #1783

Open
mahrud opened this issue Jan 3, 2021 · 9 comments
Open

Requiring status checks before merging #1783

mahrud opened this issue Jan 3, 2021 · 9 comments

Comments

@mahrud
Copy link
Member

mahrud commented Jan 3, 2021

Github allows for restricting merges to certain branches in various ways:

  • allowing only admins to merge
  • requiring a certain number of approvals before allowing a merge
  • requiring status checks to pass before allowing a merge

This is all documented here: https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/defining-the-mergeability-of-pull-requests

We should institute similar restrictions to prevent accidentally merging PR's with failing tests into incorrect branches, as happened today with #1778.

To make matters worse, after that PR was merged into master, master was merged into release-1.17, then release-1.17 was merged into development and pre-master. This is excessive and confusing, and has lead to all four active branches to be in a failing state!

Moreover, now that there's a fix (#1782), even fixing it will leads to a spaghetti of merges between these 4 branches, because we have to do 4 merges to fix all the branches! I think we should also reduce the number of parallel active branches to only master and development. As I explained in #1691 (comment):

If we use tags instead of branches for each release, including release candidates, then I don't see a reason to keep the release-* branches: whenever you have a commit that's reasonably ready, tag it release-1.17-rc1, when new issues are found and fixed, tag release-1.17-rc2, until it's ready to be tagged release-1.17.

@mahrud mahrud added the build issue platform specific issues involving compiling M2, generating examples, or running tests label Jan 6, 2021
@DanGrayson
Copy link
Member

Yes, I keep mistakenly merging into "master", as it is natural for users to direct pull requests there. It would be great if I could do that only after being presented with a vivid warning, for any branch that is not "development".

@mahrud
Copy link
Member Author

mahrud commented Jan 17, 2021

Sure. If we follow some basic etiquette, we can also avoid such objective messiness:
image

I provided the link to the documentation above.

@DanGrayson DanGrayson self-assigned this Jan 17, 2021
@mahrud
Copy link
Member Author

mahrud commented Jan 22, 2021

Checks can now be triggered for any branch from this page:
image

We can remove pre-master now.

@DanGrayson
Copy link
Member

I've been waiting for that magic button to suddenly appear for a week now. Thanks!

@mahrud
Copy link
Member Author

mahrud commented Jan 22, 2021

You hadn't merged into master till recently, I believe.

@DanGrayson
Copy link
Member

I think it's because I never happened to click on one of these two "workflows":

Screen Shot 2021-01-22 at 1 34 08 PM

@DanGrayson
Copy link
Member

I've just figured out a potentially better way: click a button to submit a pull request from development to master. As in #1862

@mahrud
Copy link
Member Author

mahrud commented Jul 26, 2022

@DanGrayson @mikestillman you may have seen the following tip from GitHub:
image

@mahrud
Copy link
Member Author

mahrud commented Jul 21, 2024

Updating for new features: GitHub's branch protection settings have quite a few options that would work well for us:

  • require reviews,
  • require status checks (e.g. green checks),
  • require a conversation, etc.

In addition, GitHub CODEOWNERS files tell it who "owns" a piece of code and should submit a review before it is merged. I'm not sure if we necessarily need this, but it's an option.

cc: @antonleykin @mikestillman @d-torrance

@mahrud mahrud added under discussion and removed build issue platform specific issues involving compiling M2, generating examples, or running tests labels Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants