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

build(deps): fix npm (grpc) build on NodeJS v20.4.0 #2563

Merged
merged 1 commit into from
Oct 17, 2023
Merged

build(deps): fix npm (grpc) build on NodeJS v20.4.0 #2563

merged 1 commit into from
Oct 17, 2023

Conversation

petermetz
Copy link
Contributor

@petermetz petermetz commented Jul 22, 2023 •
edited
Loading

  1. Eliminates all uses of the old grpc dependency from the codebase.
    1.1. Upgraded all outdated fabirc-network, fabric-client and fabric-ca-client
    dependencies to the latest stable 2.2.x version which is 2.2.18 at the time.
    1.2. Also outrighted swapped grpc declarations with @grpc/grpc-js.
    The rest of the diff is due to the incompatibility of the two mentioned.

Fixes #2562
Fixes #1507

Signed-off-by: Peter Somogyvari [email protected]

@petermetz petermetz force-pushed the petermetz/issue2562 branch 2 times, most recently from 6db9daf to 46c8e9d Compare July 22, 2023 07:30
@sandeepnRES sandeepnRES force-pushed the petermetz/issue2562 branch from 46c8e9d to 4973ca9 Compare August 9, 2023 07:08
@sandeepnRES
Copy link
Contributor

Hi @petermetz, I've corrected fabric-ca-client (along with other fabric packges) version in weaver packages, also upgraded grpc-js version in other places

@petermetz
Copy link
Contributor Author

Hi @petermetz, I've corrected fabric-ca-client (along with other fabric packges) version in weaver packages, also upgraded grpc-js version in other places

@sandeepnRES The protos-js folder's package.json still had a "grpc": "1.24.11", in it. I'm removing it.

@petermetz petermetz mentioned this pull request Sep 11, 2023
Closed
@petermetz petermetz marked this pull request as ready for review September 11, 2023 21:26
@jagpreetsinghsasan
Copy link
Contributor

jagpreetsinghsasan commented Sep 12, 2023 •
edited
Loading

@petermetz the below test case fails
packages/cactus-plugin-ledger-connector-fabric-socketio/src/test/typescript/integration/fabric-socketio-connector.test.ts (as startFabricSocketIOConnector function is cleaned in the PR and returns void)
image

stating
image

@petermetz petermetz force-pushed the petermetz/issue2562 branch 2 times, most recently from 252909d to 563d73f Compare October 5, 2023 03:54
@petermetz
Copy link
Contributor Author

packages/cactus-plugin-ledger-connector-fabric-socketio/src/test/typescript/integration/fabric-socketio-connector.test.ts

@jagpreetsinghsasan Thanks for catching that! I've removed the failing code for now just to get it moving along.

@takeutak Apologies but I had to make some further modifications so please take a look at the new diff if you have the chance!

@petermetz petermetz force-pushed the petermetz/issue2562 branch from 563d73f to c75fc56 Compare October 12, 2023 02:01
VRamakrishna
VRamakrishna requested changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@VRamakrishna VRamakrishna left a comment

Choose a reason for hiding this comment

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

LGTM. Just had a suggestion below for a refactoring instead of a deletion. See if that works. Otherwise, if you don't want to spend the time to refactor code right now, I'll approve.

@petermetz
Copy link
Contributor Author

LGTM. Just had a suggestion below for a refactoring instead of a deletion. See if that works. Otherwise, if you don't want to spend the time to refactor code right now, I'll approve.

@VRamakrishna Thank you, this helps a lot! What you suggest makes a lot of sense to me and I agree it should be done. With that said, this pull request has been pending for a while and due to it being a fix for critical severity vulnerabilities I would suggest to merge it now and send a separate pull request later with the refactor as you described (I'd go with re-using the code not copying it unless it somehow introduces a circular dependency among the packages that we cannot get out of).
In terms of time: I will have some in the second half of next month (e.g. 4 weeks from now) but until then I have to ration my time unfortunately.

@petermetz
build(deps): fix npm (grpc) build on NodeJS v20.4.0
a11825c 
1. Eliminates all uses of the old `grpc` dependency from the codebase.
1.1. Upgraded all outdated fabirc-network, fabric-client and fabric-ca-client
dependencies to the latest stable 2.2.x version which is 2.2.19 at the time.
1.2. Also outright swapped `grpc` declarations with `@grpc/grpc-js`.
The rest of the diff is due to the incompatibility of the two mentioned.

Fixes hyperledger-cacti#2562
Fixes hyperledger-cacti#1507

Signed-off-by: Sandeep Nishad <[email protected]>
Signed-off-by: Peter Somogyvari <[email protected]>
@petermetz petermetz force-pushed the petermetz/issue2562 branch from c75fc56 to a11825c Compare October 17, 2023 18:54
@petermetz petermetz merged commit 71734b0 into hyperledger-cacti:main Oct 17, 2023
@petermetz petermetz deleted the petermetz/issue2562 branch October 17, 2023 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VRamakrishna VRamakrishna VRamakrishna approved these changes

@takeutak takeutak takeutak approved these changes

@izuru0 izuru0 Awaiting requested review from izuru0 izuru0 is a code owner

@jagpreetsinghsasan jagpreetsinghsasan Awaiting requested review from jagpreetsinghsasan jagpreetsinghsasan is a code owner

@sandeepnRES sandeepnRES Awaiting requested review from sandeepnRES sandeepnRES is a code owner

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

Successfully merging this pull request may close these issues.

build(deps): fix npm (grpc) build on NodeJS v20.4.0 build(deps): upgrade dependencies, especially grpc
5 participants
@petermetz @sandeepnRES @jagpreetsinghsasan @VRamakrishna @takeutak