-
Notifications
You must be signed in to change notification settings - Fork 307
chore: upgrade protobuf to v2 and fabric protos in copm #4085
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
base: main
Are you sure you want to change the base?
Conversation
VRamakrishna
left a comment
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. But let's treat this as a work in progress and make a refactor plan. We will need to refactor the code base to eliminate redundant proto definitions, which are currently copied over from weaver/common/protos to various packages touched in this PR. @LordKubaya Can you open an issue to track this?
| "codegen": "yarn run --top-level run-s 'codegen:*'", | ||
| "codegen:proto": "yarn run proto:openapi; yarn run proto:connectrpc", | ||
| "codegen:proto": "yarn run proto:openapi; yarn clone-fabric-protos; buf generate --verbose", | ||
| "clone-fabric-protos": "rm -rf src/main/protos/fabric-protos && git clone https://github.com/hyperledger/fabric-protos.git src/main/protos/fabric-protos", |
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.
We were using release-2.1 branch of fabric-protos repo (so as to not cause any build breaks from new changes). But if latest release is compatible, then we can use that, but I'll prefer to use some branch/tag of fabric-protos and not main branch.
sandeepnRES
left a comment
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.
PR looks good to me, just one comment I feel is important, its a small change. but you will have to test compatibility
Signed-off-by: Carlos Amaro <[email protected]>
8b87c12 to
31f1eae
Compare
Changed to release 2.1, there are no problems. |
|
|
@outSH @RafaelAPB @AndreAugusto11 Can any of you review and approve so Carlos can merge this soon and move to the next task? Thanks! |
Pull Request Requirements
upstream/mainbranch 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.-sflag when usinggit commitcommand. 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.