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

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

Merged
merged 2 commits into from
Oct 28, 2021

Conversation

aduh95
Copy link
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

codecov bot commented Oct 14, 2021

Codecov Report

Merging #572 (6b173fb) into main (35d281b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #572   +/-   ##
=======================================
  Coverage   82.57%   82.57%           
=======================================
  Files          35       35           
  Lines        1750     1750           
=======================================
  Hits         1445     1445           
  Misses        305      305           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35d281b...6b173fb. Read the comment docs.

@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
Member

richardlau 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

@aduh95
Copy link
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"

if (this.assumeYes) {
return defaultAnswer;
}

@richardlau
Copy link
Member

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

if (this.assumeYes) {
return defaultAnswer;
}

In which case this is very misleading

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.'

And add an option to abort the session if trying to land more than one
commit.

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

aduh95 commented Oct 23, 2021

Just tried with:

  • git node land 39392 --yes --autorebase --oneCommitMax (failing as expected)1
  • git node land 39392 --yes --autorebase (passing as expected)1
  • git node land 40475 --yes --autorebase --oneCommitMax (passing as expected) 2
  • git node land 40475 --yes --autorebase (passing as expected)2
  • git node land 40570 --yes --autorebase --oneCommitMax (passing as expected) 3
  • git node land 40570 --yes --autorebase (passing as expected) 3

Footnotes

  1. https://github.com/nodejs/node/pull/39392 is a PR containing several commits. 2

  2. https://github.com/nodejs/node/pull/40475 is a PR containing only one commit. 2

  3. https://github.com/nodejs/node/pull/40570 is a PR containing only fixup! commits (I had to make it fast-tracked to work around the 48-hour rule). 2

aduh95 added a commit to aduh95/node that referenced this pull request 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 pull request 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
Copy link
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

lib/landing_session.js Outdated Show resolved Hide resolved
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
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
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
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
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
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants