feat(cli): prompt user when landing PR with several commits by aduh95 · Pull Request #572 · nodejs/node-core-utils · GitHub
Skip to content

feat(cli): prompt user when landing PR with several commits#572

Merged
targos merged 2 commits intonodejs:mainfrom
aduh95:one-commit-max
Oct 28, 2021
Merged

feat(cli): prompt user when landing PR with several commits#572
targos merged 2 commits intonodejs:mainfrom
aduh95:one-commit-max

Conversation

@aduh95
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 commented Oct 14, 2021

The idea would be to run CQ with this flag by default, potentially we could add additional labels such as commit-queue-fixupAll and commit-queue-more-than-one-commit to let the CQ handle more complex PRs.

Refs: nodejs/node#40436

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 14, 2021 •
edited
Loading

@aduh95 aduh95 changed the title enh(cli): add --oneCommitMax option feat(cli): add --oneCommitMax option Oct 14, 2021
@aduh95 aduh95 changed the title feat(cli): add --oneCommitMax option feat(cli): prompt user when landing PR with several commits Oct 14, 2021
@richardlau
Copy link
Copy Markdown
Member

richardlau commented Oct 14, 2021 •
edited
Loading

The current commit-queue passes in --yes so will automatically answer "yes" to the prompt.
https://github.com/nodejs/node/blob/b80b85e130a2a04a69c8fc846e27236a86107a80/tools/actions/commit-queue.sh#L73

@aduh95
Copy link
Copy Markdown
Contributor Author

aduh95 commented Oct 14, 2021

The current commit-queue passes in --yes so will automatically answer "yes" to the prompt. https://github.com/nodejs/node/blob/b80b85e130a2a04a69c8fc846e27236a86107a80/tools/actions/commit-queue.sh#L73

Not sure that's true, I think --yes means "answer whatever is the default answer"

node-core-utils/lib/cli.js

Lines 71 to 73 in 3e8b07d

if (this.assumeYes) {
return defaultAnswer;
}

@richardlau
Copy link
Copy Markdown
Member

richardlau commented Oct 14, 2021

Not sure that's true, I think --yes means "answer whatever is the default answer"

node-core-utils/lib/cli.js

Lines 71 to 73 in 3e8b07d

if (this.assumeYes) {
return defaultAnswer;
}

In which case this is very misleading

node-core-utils/components/git/land.js

Lines 53 to 58 in 62b3fef

yes: {
type: 'boolean',
default: false,
describe: 'Assume "yes" as answer to all prompts and run ' +
'non-interactively. If an undesirable situation occurs, such as a pull ' +
'request or commit check fails, then git node land will abort.'

@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
Copy Markdown
Contributor Author

aduh95 commented Oct 23, 2021

@aduh95 aduh95 mentioned this pull request Oct 23, 2021
Closed
aduh95 added a commit to aduh95/node that referenced this pull request 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 pull request 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 pull request Oct 23, 2021
Merged
mhdawson
mhdawson approved these changes Oct 25, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

targos
targos approved these changes Oct 26, 2021
View reviewed changes
Comment thread lib/landing_session.js Outdated
@aduh95 @targos
fixup! feat(cli): prompt user when landing PR with several commits
6b173fb 
Co-authored-by: Michaël Zasso <targos@protonmail.com>
@targos targos merged commit 89925c3 into nodejs:main Oct 28, 2021
@aduh95 aduh95 deleted the one-commit-max branch October 29, 2021 07:59
aduh95 added a commit to nodejs/node-auto-test that referenced this pull request Oct 30, 2021
@aduh95
tools: abort CQ session when landing several commits
ce4b0ec 
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 nodejs/node-auto-test that referenced this pull request 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 pull request 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
aduh95 added a commit to nodejs/node that referenced this pull request 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 to nodejs/node that referenced this pull request 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 to nodejs/node that referenced this pull request 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@targos targos targos approved these changes

@codebytere codebytere codebytere approved these changes

+2 more reviewers

@Trott Trott Trott left review comments

@mhdawson mhdawson mhdawson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@aduh95 @richardlau @Trott @codebytere @targos @mhdawson