Macaulay2 / M2

The primary source code repository for Macaulay2, a system for computing in commutative algebra, algebraic geometry and related fields.
https://macaulay2.com
341 stars 228 forks source link

Requiring status checks before merging #1783

Open mahrud opened 3 years ago

mahrud commented 3 years ago

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

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 https://github.com/Macaulay2/M2/issues/1691#issuecomment-747184042:

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.

DanGrayson commented 3 years ago

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 commented 3 years ago

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

I provided the link to the documentation above.

mahrud commented 3 years ago

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

We can remove pre-master now.

DanGrayson commented 3 years ago

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

mahrud commented 3 years ago

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

DanGrayson commented 3 years ago

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 commented 3 years ago

I've just figured out a potentially better way: click a button to submit a pull request from development to master. As in https://github.com/Macaulay2/M2/pull/1862

mahrud commented 2 years ago

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

mahrud commented 1 month ago

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

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