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

Prefer "Squash and merge" while merging Pull Requests? #28

Closed
trivikr opened this issue Jan 17, 2020 · 2 comments
Closed

Prefer "Squash and merge" while merging Pull Requests? #28

trivikr opened this issue Jan 17, 2020 · 2 comments

Comments

@trivikr
Copy link
Member

trivikr commented Jan 17, 2020

I noticed that this project uses "Create a merge commit" while merging PRs.
This creates a merge commit, and all commits from PR (including experimental ones) are merged to master.

Commit history: https://github.com/aws/aws-sdk-js-crypto-helpers/commits/master

Merge commit and base commits for #26
Screen Shot 2020-01-17 at 3 03 48 PM

Mixed commits between #19 and #25
Screen Shot 2020-01-17 at 3 04 08 PM

Suggestion: Do the following in settings https://github.com/aws/aws-sdk-js-crypto-helpers/settings

  • Disable "Allow merge commits" and "Allow rebase merging"
  • Only enable "Allow squash merging"

Squash and merge will create one commit per PR, keeping the history clean.
Squash merging is used in https://github.com/aws/aws-sdk-js-v3/commits/master

@trivikr
Copy link
Member Author

trivikr commented Jan 20, 2020

Some discussions from Amazon Chime:

In aws/aws-sdk-js-v3, we squash almost all the times to keep the commit history clean.
If we need more info about specific commit, we visit PR associated with the commit and view its commits.

For example:

We use “rebase and merge” for rare PRs, where individual commits need to be preserved.
For example, PR which generates initial code for 17 clients was rebased and merged aws/aws-sdk-js-v3#746

We use commitlint in CI and lint-staged to test for format of commit message in each commit aws/aws-sdk-js-v3#239
For squashed commit msg, the maintainer ensures the PR is title is updated before merging.
We usually update the title as soon as context changes, and verify if title is incorrect before reviewing the PR

@seebees
Copy link
Contributor

seebees commented Jan 20, 2020

We are working on an auto-merge.
While most PR's should be squashed, not every one should.
Expressing this intent between the project and the requester is key.

At this time, I am using the Require branches to be up to date before merging setting in branch protection.
This should help with keeping the history linear if not perfectly clean.

@seebees seebees closed this as completed Jan 20, 2020
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

No branches or pull requests

2 participants