-
Notifications
You must be signed in to change notification settings - Fork 288
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(cactus-example-tcs-huawei): remove deprecated sample app #3158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@outSH LGTM and thank you for the PR!
I just wanted to give some more context around this:
We are in a pickle because we've been waiting for some months now for the updates to come in on these packages but have run out of time to wait and so in the meantime we are just removing this code but that does not imply in any way that we are not happy to put it back as soon as it's ready for prime time.
I wanted to make sure that this is loud and clear to all community members, contributors, etc. because without this context it might come off as the maintainers just unilaterally removing the hard work of others which is not the case of course!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@outSH Just one more suggestion: I'd also put the clarification that you explained in the GitHub issue itself directly to the commit message so that it's guaranteed to be available as information to someone who is just browsing the commit log and trying to figure out the details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
680c170
to
be999ec
Compare
@petermetz Done, I've added the following to the commit:
|
@outSH Looks good to me, thank you for adding the extra clarifications! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks
- `cactus-example-tcs-huawei` is using `cactus-plugin-ledger-connector-go-ethereum-socketio` and `cactus-plugin-ledger-connector-tcs-huawei-socketio` which will be removed as well. Ths sample app can't exist on it's own. - This is not permanent - we'd love to bring the package back once the necessary refactors are done! - See issue hyperledger-cacti#3155 on github for more details on context and reasoning of this decision. Closes: hyperledger-cacti#3157 Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
be999ec
to
1083a71
Compare
cactus-example-tcs-huawei
is usingcactus-plugin-ledger-connector-go-ethereum-socketio
andcactus-plugin-ledger-connector-tcs-huawei-socketio
which will be removed as well. Ths sample app can't exist on it's own.Closes #3157
Part of #3155
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.