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

stream: stricter isReadableNodeStream #40941

Merged
merged 3 commits into from
Nov 26, 2021
Merged

stream: stricter isReadableNodeStream #40941

merged 3 commits into from
Nov 26, 2021

Conversation

ronag
Copy link
Member

@ronag ronag commented Nov 23, 2021

Fixes: #40938

@ronag ronag added stream Issues and PRs related to the stream subsystem. v17.x labels Nov 23, 2021
@ronag ronag requested a review from mcollina November 23, 2021 17:11
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2021
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 23, 2021
@ronag ronag removed the needs-ci PRs that need a full CI run. label Nov 23, 2021
mcollina
mcollina approved these changes Nov 23, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41059/

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41061/

mcollina
mcollina approved these changes Nov 23, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 23, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41068/

@github-actions github-actions bot mentioned this pull request Nov 25, 2021
Open
38 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41095/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41103/

@ronag
Copy link
Member Author

ronag commented Nov 25, 2021

@Trott CI doesn't seem to be able to succeed. Not just this PR. Can we land without node-test-commit-linux-containered?

@github-actions github-actions bot mentioned this pull request Nov 26, 2021
Open
33 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41125/

@Trott
Copy link
Member

Trott commented Nov 26, 2021 •
edited
Loading

@Trott CI doesn't seem to be able to succeed. Not just this PR. Can we land without node-test-commit-linux-containered?

I believe the issue is fixed (or at least mitigated somewhat?) by #40934 which was fast-tracked and landed. So it you rebase against master and re-run CI, it should pass.

@Trott
Copy link
Member

Trott commented Nov 26, 2021

I believe the issue is fixed (or at least mitigated somewhat?) by #40934 which was fast-tracked and landed. So it you rebase against master and re-run CI, it should pass.

Oh, wait, for Jenkins, I don't think you need to rebase. I think a Rebuild rather than a Resume Build will rebase. I'll kick that job off now to see what happens.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41126/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41130/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41138/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/41140/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 26, 2021 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/41141/ ✅

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 26, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 26, 2021
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/40941
✔  Done loading data for nodejs/node/pull/40941
----------------------------------- PR info ------------------------------------
Title      stream: stricter isReadableNodeStream (#40941)
Author     Robert Nagy  (@ronag)
Branch     ronag:is-readable-strict -> nodejs:master
Labels     stream, author ready, v17.x
Commits    3
 - stream: stricter isReadableNodeStream
 - fixup
 - fixup: lint
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/40941
Fixes: https://github.com/nodejs/node/issues/40938
Reviewed-By: Matteo Collina 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/40941
Fixes: https://github.com/nodejs/node/issues/40938
Reviewed-By: Matteo Collina 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 23 Nov 2021 17:11:36 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/40941#pullrequestreview-814001663
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/40941#pullrequestreview-814084727
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2021-11-26T16:27:16Z: https://ci.nodejs.org/job/node-test-pull-request/41141/
- Querying data for job/node-test-pull-request/41141/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 40941
From https://github.com/nodejs/node
 * branch                  refs/pull/40941/merge -> FETCH_HEAD
✔  Fetched commits as 43e127801425..133cc2c55270
--------------------------------------------------------------------------------
[master d8e39bc26c] stream: stricter isReadableNodeStream
 Author: Robert Nagy 
 Date: Tue Nov 23 18:11:00 2021 +0100
 3 files changed, 22 insertions(+), 2 deletions(-)
[master a9a8e95738] fixup
 Author: Robert Nagy 
 Date: Tue Nov 23 18:20:46 2021 +0100
 1 file changed, 1 insertion(+), 3 deletions(-)
[master 58fe8c2cba] fixup: lint
 Author: Robert Nagy 
 Date: Tue Nov 23 18:22:56 2021 +0100
 1 file changed, 13 insertions(+), 10 deletions(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)

Executing: git node land --amend --yes

⚠  Found Fixes: #40938, skipping..

--------------------------------- New Message ----------------------------------

stream: stricter isReadableNodeStream


Fixes: #40938


PR-URL: #40941

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: James M Snell jasnell@gmail.com


[detached HEAD 3df604992c] stream: stricter isReadableNodeStream

Author: Robert Nagy ronagy@icloud.com

Date: Tue Nov 23 18:11:00 2021 +0100

3 files changed, 22 insertions(+), 2 deletions(-)

Rebasing (3/6)

Rebasing (4/6)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup


PR-URL: #40941

Fixes: #40938

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: James M Snell jasnell@gmail.com


[detached HEAD b12ac74f60] fixup

Author: Robert Nagy ronagy@icloud.com

Date: Tue Nov 23 18:20:46 2021 +0100

1 file changed, 1 insertion(+), 3 deletions(-)

Rebasing (5/6)

Rebasing (6/6)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup: lint


PR-URL: #40941

Fixes: #40938

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: James M Snell jasnell@gmail.com


[detached HEAD ba72c05fb0] fixup: lint

Author: Robert Nagy ronagy@icloud.com

Date: Tue Nov 23 18:22:56 2021 +0100

1 file changed, 13 insertions(+), 10 deletions(-)


Successfully rebased and updated refs/heads/master.


ℹ  Use --fixupAll option, squash the PR manually or land the PR from the command line.
https://github.com/nodejs/node/actions/runs/1508434363

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Nov 26, 2021
@Trott Trott added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 26, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 26, 2021
@nodejs-github-bot nodejs-github-bot merged commit 49b8c4f into nodejs:master Nov 26, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 49b8c4f

@github-actions github-actions bot mentioned this pull request Nov 27, 2021
Open
27 tasks
@ronag ronag mentioned this pull request Nov 27, 2021
Merged
This was referenced Nov 28, 2021
Open
Open
targos pushed a commit that referenced this pull request Nov 29, 2021
@ronag @targos
stream: stricter isReadableNodeStream
70bd90e 
Fixes: #40938

PR-URL: #40941
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams
Copy link
Contributor

@ronag is this only for 17.x?

@F3n67u F3n67u mentioned this pull request Jun 30, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

stream: Node v17 regression in finished with OutgoingMessage
7 participants
@ronag @nodejs-github-bot @Trott @danielleadams @mcollina @jasnell @targos