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

Use Readable.from for streams #21417

Merged
merged 4 commits into from
Nov 21, 2023
Merged

Use Readable.from for streams #21417

merged 4 commits into from
Nov 21, 2023

Conversation

benjdlambert
Copy link
Member

@benjdlambert benjdlambert commented Nov 20, 2023 •
edited

Fixes #17072

Same fix as #18002

@benjdlambert
chore: use Readable.from instead to convert the stream
f386bef 
Signed-off-by: blam <ben@blam.sh>
@benjdlambert
chore: add changeset
1ad8906 
Signed-off-by: blam <ben@blam.sh>
@benjdlambert benjdlambert requested review from a team as code owners November 20, 2023 13:07
@benjdlambert benjdlambert mentioned this pull request Nov 20, 2023
Closed
2 tasks
@backstage-goalie
Copy link
Contributor

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage/backend-common packages/backend-common patch v0.19.9

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 20, 2023 •
edited

Uffizzi Cluster pr-21417 was deleted.

@benjdlambert
Copy link
Member Author

benjdlambert commented Nov 20, 2023 •
edited

Hmm interestingly this fails in the tests but not at runtime where this bug is present, looks like this might need some more attention as what the stream actually is at runtime, as it doesn't look like it's a proper stream instance, which could actually be the bug.

EDIT: Think that this was just bad test data calling it with non-iterables, as ReadableStreams are iterable.

benjdlambert
benjdlambert commented Nov 21, 2023
View reviewed changes
packages/backend-common/src/reading/ReadUrlResponseFactory.test.ts
let readable: NodeJS.ReadableStream;

beforeEach(() => {
readable = new Stream({
encoding: 'utf-8',
}) as unknown as NodeJS.ReadableStream;
Copy link
Member Author

Choose a reason for hiding this comment

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

Stream is not a NodeJS.ReadableStream implementation.

Rugvip
Rugvip approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

✨

@benjdlambert
chore: fix gerrit reader
b623c9f 
Signed-off-by: blam <ben@blam.sh>
@benjdlambert benjdlambert merged commit f123f01 into master Nov 21, 2023
30 checks passed
@benjdlambert benjdlambert deleted the blam/fix-stream-not-readable branch November 21, 2023 15:07
@github-actions GitHub Actions
Copy link
Contributor

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.21.0 release, scheduled for Tue, 19 Dec 2023.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freben freben freben approved these changes

@Rugvip Rugvip Rugvip approved these changes

@awanlin awanlin awanlin approved these changes

@tudi2d tudi2d Awaiting requested review from tudi2d tudi2d is a code owner automatically assigned from backstage/reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🐛 Bug Report: fetch:plain:file fails with stream is not readable
4 participants
@benjdlambert @freben @Rugvip @awanlin