add github.com/hyperledger/fabric-protos-go-apiv2 #47
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
|
@ale-linux please allow the pipelines to run. |
This was referenced Sep 5, 2024
|
Based on experience of the v2 implementations for Go chaincode, my preference would be for the approach in this PR where the v2 code replaces the previous code in the root of the repository. It really does require a v2.0.0 release/tag to then be created at this point to differentiate from the older (pre-v2) code. If any future maintenance updates are required to the pre-v2 code, they would be made in a new branch (populated with the last pre-v2 commit from the main branch), and tagged there. |
C0rWin
reviewed
Sep 8, 2024
There was a problem hiding this comment.
the change is rather straight forward, and looks good, still I think the second opinion by @ale-linux or @yacovm would be great.
denyeart
approved these changes
Sep 12, 2024
denyeart
reviewed
Sep 13, 2024
There was a problem hiding this comment.
@pfi79 In the other repositories, the direct dependency on github.com/golang/protobuf was changed to google.golang.org/protobuf. Is there a reason that is not done in this PR?
@ale-linux Which approach would you prefer?
|
thanks for the patch. I see that we upgrade multiple packages in one single PR - let's split them for now and prioritise the protobuf upgrade. How about we just merge this for now? @pfi79 |
|
@pfi79 Additionally, instead of a gomega downgrade here, can we upgrade gomega in fabric? |
|
@pfi79 if you are okay with merging just that for now - pls go ahead and force-push. I'll re-run the CI and merge |
For some reason, I don't know yet why. upgrading gomega version causes raft integration tests to fail. This requires separate investigation. |
Protobuf new and old can coexist. For example, in fabric old protobuf is still used in smartbft and raft. idemix is not one module, but several. It has several go.mod files. I do not fully understand this whole repository and therefore I am afraid to change protobuf to the new one. |
The thing is that I wanted to change fabric-protos-go-apiv2 first. But then fabric will pull in the new version of gomega from idemix. But for some reason, with the new version of gomega, raft integration tests in fabric fail. I can make 2 pr: first gomega, then fabric-protos-go-apiv2 |
ale-linux
approved these changes
Sep 13, 2024
|
sounds good - let's go with this then |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
and downgrade gomega for fabric
it's an alternative to pr
you can choose one of them or propose a 3rd alternative
hyperledger/fabric#3650