add fabric-protos-go-apiv2 #66
Conversation
pfi79
force-pushed
the
add-fabric-protos-go-apiv2
branch
2 times, most recently
from
cd0f161
to
d535363
Compare
|
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. |
So for those who still need the old version, we should make a separate branch called “v1”, and main update to v2? |
|
It seems you don't actually need a new branch. You can depend based on tag, provided the pre-v2 release is tagged. Or you can use pseudo-versions. It just makes it harder to pick pre-v2 versions without an actual Git tag. The second branch just comes in if you need to release a new pre-v2 version after you have already made the main branch the v2 stream. You need somewhere to deliver the change. Only once this is necessary, I would create a new branch based on the last commit before the v2 change, deliver pre-v2 changes to that branch, then just tag the new pre-v2 version commit so make it accessible as a specific Go version. |
…oogle.golang.org/protobuf Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
pfi79
force-pushed
the
add-fabric-protos-go-apiv2
branch
from
d535363
to
637026e
Compare
|
@denyeart can you see pr, please? |
| fieldName := prop.OrigName | ||
| fieldValue := mVal.FieldByName(prop.Name) | ||
| fieldTypeStruct, ok := mVal.Type().FieldByName(prop.Name) | ||
| for i := 0; i < t.NumField(); i++ { |
There was a problem hiding this comment.
Why the different approach here?
There was a problem hiding this comment.
I'm sorry, what do you see as a different approach?
Functions were used here that are no longer present in the new protobuf. I was just trying to replicate the logic by other means. Although in my opinion, this code fragment is very overcomplicated.
I think it can be written in a simpler way. But not in this pr.
There was a problem hiding this comment.
proto.GetProperties is missing from the new protobuf
I got inside her and tried to replicate her logic here
There was a problem hiding this comment.
When I rewrote it, I got it wrong 3 or 4 times. And the tests didn't pass. This is the result of long debugging.
But if there is something wrong here, tell me what. I'll rewrite it.
There was a problem hiding this comment.
I meant why did you have to utilize a new approach versus the prior code. I understand now. I've also tested the fabric consumers such as configtxgen -inspect with the new PR, and it works fine, so I'll go ahead and merge.
@bestbeforetoday Does this really require a v2.0.0 release tag? I didn't see any signature changes. Plus, the current version is v0.x which implies that this module is not yet stable. We kept it at v0.x because this module has not been used in a fabric admin SDK yet (as we expected), so the signatures could still potentially change once we do that integration. I'm thinking of tagging as v0.3.0. |
|
Right now you definitely cannot tag it as v2.x since the module path remains as The key issue with compatibility is likely to be the switch from Given that it's currently at v0.x, you could certainly argue that there is no compatibility guarantee and, particularly if the public API is unchanged, just stick at a v0.x release. A middle ground could be to declare a v1.x release, which would indicate a breaking change from v0.x without the module path change. I guess the choice is likely to be guided by the risk of breaking any users of this module (who also use fabric-protos) moving up a minor release version without realising the implications. |
|
Thanks @bestbeforetoday , I've gone ahead and released as v0.3.0, remaining at v0.x.x will allow us to make small changes if needed for any important consumers such as a fabric SDK or CLI consumer. @pfi79 Go ahead and update your fabric PR to use v0.3.0. |
and change github.com/golang/protobuf on google.golang.org/protobuf
it's an alternative to pr
you can choose one of them or propose a 3rd alternative
hyperledger/fabric#3650