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(socket.io, socket.io-client): upgrade to common socket.io version #1696

Merged
merged 1 commit into from
Jan 14, 2022
Merged

build(socket.io, socket.io-client): upgrade to common socket.io version #1696

merged 1 commit into from
Jan 14, 2022

Conversation

outSH
Copy link
Contributor

@outSH outSH commented Dec 28, 2021

Some packages use socket.io and socket.io-client V2, while other use V4.
In order for these components to communicate, we must use common
socket.io version in all cactus packages (V4.1.3). This commit introduce
some other related changes, like merging socketio unit tests to common
setup (to assert changes are correct) and fixes some strict-flag
warnings and minor bugs. It also updates python packages requirements
and updates readme when it was necessary.

Closes: #1679
Signed-off-by: Michal Bajer michal.bajer@fujitsu.com

Additional changes in cactus-cmd-socketio-server:

Depends on #1670

@petermetz petermetz requested review from takeutak and removed request for jonathan-m-hamilton December 28, 2021 21:30
@petermetz petermetz added the dependencies Pull requests that update a dependency file label Dec 28, 2021
@petermetz
Copy link
Contributor

@outSH Thank you very much for addressing this so quickly, I'm very grateful for the quick turnaround!
Right now the PR has a single commit that includes the full diff of the PR that this one is dependent on, there's two options we can do to make it easy to review:

  1. Seperate out the commits so that the diffs can be reviewed separately
  2. Or just wait until build: include cmd-socketio-server and socketio validators in the monorepo setup #1670 gets merged and then after a rebase this will have the correct diff to be reviewed.

I'm fine either way. Regarless of which one you choose, this is looking great at first glance.

@outSH
Copy link
Contributor Author

outSH commented Dec 29, 2021

Depends on #1660

@outSH outSH marked this pull request as draft December 29, 2021 12:10
@outSH
Copy link
Contributor Author

outSH commented Dec 29, 2021

@petermetz
I've changed this PR to draft, after #1660 and #1670 are merged I will double check and do some test to ensure everything is fine, and we'll proceed :) In the future I'll probably just merge dependent commits to clarify the diffs

@petermetz
Copy link
Contributor

@outSH Understood, sounds like a plan! :-)

@github-actions github-actions bot removed the dependent label Jan 5, 2022
@github-actions
Copy link

github-actions bot commented Jan 5, 2022

This PR/issue depends on:

@outSH
Copy link
Contributor Author

outSH commented Jan 5, 2022

Dependent PR were merged, I've done tests after rebasing to main and all seems fine.

@petermetz @izuru0 Please review the changes :)

@outSH outSH marked this pull request as ready for review January 5, 2022 18:43
@outSH outSH requested a review from petermetz January 5, 2022 18:43
izuru0
izuru0 approved these changes Jan 6, 2022
View reviewed changes
Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

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

LGTM

@outSH outSH mentioned this pull request Jan 11, 2022
Merged
petermetz
petermetz approved these changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@outSH LGTM, thank you very much.

@petermetz petermetz removed the request for review from takeutak January 11, 2022 23:58
@outSH
build(socket.io, socket.io-client): upgrade to common socket
7e1bc8d 
Some packages use socket.io and socket.io-client V2, while other use V4.
In order for these components to communicate, we must use common
socket.io version in all cactus packages (V4.1.3). This commit introduce
some other related changes, like merging socketio unit tests to common
setup (to assert changes are correct) and fixes some strict-flag
warnings and minor bugs. It also updates python packages requirements
and updates readme when it was necessary.

Closes: hyperledger-cacti#1679
Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
@petermetz petermetz merged commit 22d474c into hyperledger-cacti:main Jan 14, 2022
@StepniowskiP StepniowskiP mentioned this pull request Jan 17, 2022
Merged
@outSH outSH deleted the upgrade_socketio_to_4_pr branch March 21, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petermetz petermetz petermetz approved these changes

@izuru0 izuru0 izuru0 approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

build(socket.io, socket.io-client) Upgrade to common socket.io version
3 participants
@outSH @petermetz @izuru0