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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

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.

@aduh95 aduh95 mentioned this issue Oct 14, 2021
Merged
@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
@aduh95
feat(cli): prompt user when landing PR with several commits
cbcf4b0 
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
@aduh95
tools: abort CQ session when landing several commits
1d1ba6f 
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
@aduh95
tools: abort CQ session when landing several commits
fdd0ac9 
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
@aduh95 aduh95 mentioned this issue Oct 23, 2021
Merged
targos pushed a commit to nodejs/node-core-utils that referenced this issue Oct 28, 2021
@aduh95
feat(cli): prompt user when landing PR with several commits (#572)
89925c3 
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
@aduh95
tools: abort CQ session when landing several commits
cad7fb9 
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
@aduh95
tools: abort CQ session when landing several commits
f7a3c49 
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
@aduh95
tools: abort CQ session when landing several commits
9bd6d52 
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 <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Nov 6, 2021
@aduh95 @targos
tools: abort CQ session when landing several commits
0787c78 
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 <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this issue Nov 25, 2021
@aduh95 @BethGriggs
tools: abort CQ session when landing several commits
c12ce89 
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 <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
johnfrench3 pushed a commit to johnfrench3/core-utils-node that referenced this issue Nov 2, 2022
feat(cli): prompt user when landing PR with several commits (#572)
9f47215 
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
@renawolford6
feat(cli): prompt user when landing PR with several commits (#572)
2514fb3 
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
@logictitans
feat(cli): prompt user when landing PR with several commits (#572)
84bed38 
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
@gerkai
feat(cli): prompt user when landing PR with several commits (#572)
9f000a7 
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
@patrickm68
feat(cli): prompt user when landing PR with several commits (#572)
2fe27fe 
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
Assignees
No one assigned
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

tools: abort CQ session when landing several commits aduh95/node
7 participants
@mscdex @targos @tniessen @mhdawson @aduh95 @Flarna @Mesteery