Skip to content

Conversation

@big-r81
Copy link
Contributor

@big-r81 big-r81 commented Sep 22, 2023

Add additional branch checks before merging a PR.

1. Require a linear history for better redability.
2. Disable "merge commits".
3. Require at least one approval.

Edited:

As a first smaller change, add at least one approval before merging a PR.

@big-r81 big-r81 force-pushed the big-r81-ext-branch-protect branch from d8eed8a to 4b1e3ba Compare October 3, 2023 12:13
@big-r81 big-r81 marked this pull request as ready for review October 3, 2023 12:13
Copy link
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

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

+1 from me. Let's try it and see how it goes. We can always tweak them as we go along. Most of these are already good practices we follow anyway, so it would be an extra check to prevent accidental merges and such.

For visibility let's mention it in dev slack channel to see if there are other preferences.

@rnewson
Copy link
Member

rnewson commented Oct 3, 2023

I dislike the inability to do a merge commit. For larger pull requests I deliberately split them into multiple commits that are easy to review (at the time but, importantly, retrospectively). CouchDB may not be usable at each step, only before or after the set. Flattening these commits to form a fake linear history will lose that context, will yield broken builds at some of those commits, and the history does not actually reflect the history that the project took.

@big-r81
Copy link
Contributor Author

big-r81 commented Oct 4, 2023

Okay, than I would like to know, how others review commits.

For my side, I use the GH UI and check all changes (from all commits) as a whole and if it's a larger change, then I used the checkmark that I viewed the file. If no changes are made again (in the meantime) I would give my approval/request/comment ...

I think it's totally okay to give seperate commits during development, but if it should land in main, then it should be squashed to exactly one "atomic" commit. One feature/fix one commit. If something is wrong with that, it can be easily reverted or fixed.

This way would solve two of your hints. CouchDB is always usable after each commit and there are no "sub-feature/bug-commits" on the history. If a "sub-feature/bug" is important for history, then it should become an own commit.

@big-r81 big-r81 force-pushed the big-r81-ext-branch-protect branch from 4b1e3ba to 9975131 Compare October 4, 2023 12:17
@big-r81 big-r81 mentioned this pull request Oct 30, 2023
5 tasks
@big-r81 big-r81 force-pushed the big-r81-ext-branch-protect branch 2 times, most recently from 76f8a9c to d0db876 Compare February 13, 2024 18:37
@big-r81 big-r81 changed the title Extending branch protection rules of .asf.yaml Add at least one approval before merging a PR to .asf.yaml Feb 13, 2024
@big-r81
Copy link
Contributor Author

big-r81 commented Feb 13, 2024

Hey, I updated the PR and removed the disabling of merge requests and the force of a linear history.
The only additional check is, that we need at least one approval to be allowed to merge the PR.

Nick @nickva, I had your approval before, but would like to get a new one (from others too). Maybe @rnewson has additional comments?

@big-r81 big-r81 changed the title Add at least one approval before merging a PR to .asf.yaml Add at least one approval before merging a PR Feb 13, 2024
Require at least one approval before merging a PR.
@big-r81 big-r81 force-pushed the big-r81-ext-branch-protect branch from d0db876 to ba8c534 Compare February 14, 2024 08:05
@pgj
Copy link
Contributor

pgj commented Feb 14, 2024

Thanks @big-r81 for working on this!

@big-r81 big-r81 merged commit 43ab37b into main Feb 14, 2024
@big-r81 big-r81 deleted the big-r81-ext-branch-protect branch February 14, 2024 17:07
jiahuili430 pushed a commit to cloudant/couchdb that referenced this pull request Mar 19, 2024
Require at least one approval before merging a PR.
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.

4 participants