-
Notifications
You must be signed in to change notification settings - Fork 103
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
chore: update proto version and proto-gen #879
Conversation
Codecov Report
@@ Coverage Diff @@
## master #879 +/- ##
==========================================
+ Coverage 73.84% 74.06% +0.21%
==========================================
Files 173 171 -2
Lines 22239 22094 -145
==========================================
- Hits 16423 16363 -60
+ Misses 4673 4626 -47
+ Partials 1143 1105 -38
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -1399,7 +1399,7 @@ func (x *fastReflection_EventStoreRawData) ProtoMethods() *protoiface.Methods { | |||
// Code generated by protoc-gen-go. DO NOT EDIT. | |||
// versions: | |||
// protoc-gen-go v1.27.0 | |||
// protoc v3.19.1 |
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.
Would be nice if there was a version here rather than (unknown)
but this is consistent with the sdk.
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.
im not a docker expert, but im wondering if maybe protoc isn't installed in the container? https://grpc.io/docs/protoc-installation/
that protoc version in the -diff is the one i had installed
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.
Hmm... protoc is installed as it was on my system. I'm using a later version.
$ protoc --version
libprotoc 3.19.4
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.
I think this is a minor detail and the sdk is using the same setup and generates (unkown)
as well. Would be nice to get this merged so that we are all generating the same output. Updating the tendermint container is also out of scope for this pull request and not something we can do.
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.
if protoc wouldn't be installed then we wouldn't be able to generate anything, right?
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.
left on qustion
@@ -340,7 +340,7 @@ devdoc-update: | |||
### Protobuf ### | |||
############################################################################### | |||
|
|||
containerProtoVer=v0.2 |
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.
so we are not creating our own container?
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 could create our own container but I think it makes sense to use the tendermint container for now.
Description
Update proto version and re-enable docker proto-gen.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change