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

Commit queue and unsquashed commits #40436

Closed
mscdex opened this issue Oct 13, 2021 · 6 comments · Fixed by #40577
Closed

Commit queue and unsquashed commits #40436

mscdex opened this issue Oct 13, 2021 · 6 comments · Fixed by #40577
Labels
meta Issues and PRs related to the general management of the project.

Comments

@mscdex
Copy link
Contributor

mscdex commented Oct 13, 2021

I've seen this occur a few times in recent times where PRs are landed with the "commit queue" feature without the commits being squashed first.

What if the github bot required some kind of explicit "approval" or something similar before a PR with the commit queue label applied with multiple commits in it could be landed? That way it wouldn't be as easy as simply adding the label to push a bunch of unsquashed commits.

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Oct 13, 2021
@targos
Copy link
Member

targos commented Oct 13, 2021

A few examples: #40344, #40348, #40128

@Flarna
Copy link
Member

Flarna commented Oct 13, 2021

It would be nice to tell bot to squash commits even the extra commits are no fixup commits.

@mscdex
Copy link
Contributor Author

mscdex commented Oct 13, 2021

I think the problems with having the bot squash commits would at least include:

  • Not knowing what should go in the squashed commit message.
  • Knowing which (or if all) commits really should be squashed. There are some cases (although not as common relatively speaking) where you really do want to keep the commits currently in the PR.

@mhdawson
Copy link
Member

@mscdex, I think we could require manual landing in the case where you wanted to keep multiple commits

In terms of what should go into the squashed commit message I think I'd keep it to the message in the original commit as that is most often what would make sense.

Having said that, the original suggestion of an approval makes sense to me.

Maybe as a quick fix, just preventing the queue from landing a PR with multiple commits would be a good first step.

@tniessen
Copy link
Member

It would be nice to tell bot to squash commits even the extra commits are no fixup commits.

To add to this, our guides say that

All commits should be self-contained, meaning every commit should pass all tests.

and this is not true for some of the unsquashed commits landed through the commit queue.

aduh95 added a commit to aduh95/node-core-utils that referenced this issue Oct 23, 2021
And add an option to abort the session if trying to land more than one
commit.

Refs: nodejs/node#40436
@aduh95
Copy link
Contributor

aduh95 commented Oct 23, 2021

nodejs/node-core-utils#572 is ready for reviews, should improve the situation. The plan would be to make the CQ fail by default when trying to land several commits.
We could decide to add commit-queue-fixupAll and commit-queue-land-multiple-commits to allow to land PR with multiple commits, or decide that this is better to manual landing.

aduh95 added a commit to aduh95/node that referenced this issue Oct 23, 2021
Most PRs are meant to be squashed in one commit when landing. If the
collaborator hasn't been using `fixup!` commit, the CQ will land the PR
as several commits. This change makes the CQ abort by default
when attempting to land several commits, unless there's another
label added to the PR to force squashing or landing as several commits.

Fixes: nodejs#40436
Refs: nodejs/node-core-utils#572
aduh95 added a commit to aduh95/node that referenced this issue Oct 23, 2021
Most PRs are meant to be squashed in one commit when landing. If the
collaborator hasn't been using `fixup!` commits, the CQ lands the PR
as several commits. This change makes the CQ abort by default
when attempting to land several commits, unless there's another
label added to the PR to force squashing or landing as several commits.

Fixes: nodejs#40436
Refs: nodejs/node-core-utils#572
targos pushed a commit to nodejs/node-core-utils that referenced this issue Oct 28, 2021
And add an option to abort the session if trying to land more than one
commit.

Refs: nodejs/node#40436
aduh95 added a commit to nodejs/node-auto-test that referenced this issue Oct 30, 2021
Most PRs are meant to be squashed in one commit when landing. If the
collaborator hasn't been using `fixup!` commits, the CQ lands the PR
as several commits. This change makes the CQ abort by default
when attempting to land several commits, unless there's another
label added to the PR to force squashing or landing as several commits.

Fixes: nodejs/node#40436
Refs: nodejs/node-core-utils#572

PR-URL: nodejs/node#40577
aduh95 added a commit to aduh95/node that referenced this issue Oct 30, 2021
Most PRs are meant to be squashed in one commit when landing. If the
collaborator hasn't been using `fixup!` commits, the CQ lands the PR
as several commits. This change makes the CQ abort by default
when attempting to land several commits, unless there's another
label added to the PR to force squashing or landing as several commits.

Fixes: nodejs#40436
Refs: nodejs/node-core-utils#572

PR-URL: nodejs#40577
@Mesteery Mesteery reopened this Oct 30, 2021
aduh95 added a commit that referenced this issue Nov 1, 2021
Most PRs are meant to be squashed in one commit when landing. If the
collaborator hasn't been using `fixup!` commits, the CQ lands the PR
as several commits. This change makes the CQ abort by default
when attempting to land several commits, unless there's another
label added to the PR to force squashing or landing as several commits.

Fixes: #40436
Refs: nodejs/node-core-utils#572

PR-URL: #40577
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this issue Nov 6, 2021
Most PRs are meant to be squashed in one commit when landing. If the
collaborator hasn't been using `fixup!` commits, the CQ lands the PR
as several commits. This change makes the CQ abort by default
when attempting to land several commits, unless there's another
label added to the PR to force squashing or landing as several commits.

Fixes: #40436
Refs: nodejs/node-core-utils#572

PR-URL: #40577
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
BethGriggs pushed a commit that referenced this issue Nov 25, 2021
Most PRs are meant to be squashed in one commit when landing. If the
collaborator hasn't been using `fixup!` commits, the CQ lands the PR
as several commits. This change makes the CQ abort by default
when attempting to land several commits, unless there's another
label added to the PR to force squashing or landing as several commits.

Fixes: #40436
Refs: nodejs/node-core-utils#572

PR-URL: #40577
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
johnfrench3 pushed a commit to johnfrench3/core-utils-node that referenced this issue Nov 2, 2022
And add an option to abort the session if trying to land more than one
commit.

Refs: nodejs/node#40436
renawolford6 added a commit to renawolford6/node-dev-build-core-utils that referenced this issue Nov 10, 2022
And add an option to abort the session if trying to land more than one
commit.

Refs: nodejs/node#40436
Developerarif2 pushed a commit to Developerarif2/node-core-utils that referenced this issue Jan 27, 2023
And add an option to abort the session if trying to land more than one
commit.

Refs: nodejs/node#40436
gerkai added a commit to gerkai/node-core-utils-project-build that referenced this issue Jan 27, 2023
And add an option to abort the session if trying to land more than one
commit.

Refs: nodejs/node#40436
shovon58 pushed a commit to shovon58/node-core-utils that referenced this issue Jun 9, 2023
And add an option to abort the session if trying to land more than one
commit.

Refs: nodejs/node#40436
patrickm68 added a commit to patrickm68/NodeJS-core-utils that referenced this issue Sep 14, 2023
And add an option to abort the session if trying to land more than one
commit.

Refs: nodejs/node#40436
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants