Replace gogo protobuf with google's protobuf v2 compiler #119
Conversation
This wasn't too bad, though the variable names disappeared from the go-parsed type signature of our RPC specs so I had to generate variable names manually /shrug
Google's protobuf compiler doesn't allow you to customize JSON formatting and forking it like we did with gogo's is no simple matter (it depends on a large number of internal packages...). I'm deleting it for now to unblock myself while I wait to discuss this with our HTTP API team tomorrow
Props to sed for existing
Go's protobuf-generated code embeds locks, so by copying the contents of the object we violate the lock's guarantees. We'll return the new payload and overwrite the old instead
The files changed name, whoops.
We'll want to use this in the CLI as well to allow users to update old histories
There's one option and its pretty much always supplied; this was an over-eager change.
Pulled these over form the SDK and cleaned up the naming. common.ThingPtr and common.ThingValue don't roll off the tongue as nicely as thing.Proto and thing.Value.
Filter out non-go files and make sure to rewrite uses of these enums
Tests are now run by default (as they should be)
There was a problem hiding this comment.
I would totally support the complete removal of mocks from this library, but not sure what others think
There was a problem hiding this comment.
FWIW I think it makes sense to delete them for the following reasons:
- https://github.com/golang/mock that is used to generate mocks is no longer maintained. https://github.com/uber-go/mock is the recommended replacement (fork).
- Apart from the Uber's fork, there is at least also https://github.com/vektra/mockery, which is also quite popular. So, there is no standard, people use various frameworks.
- It's trivial to generate mocks using one's favorite library.
- Good to have fewer dependencies.
There was a problem hiding this comment.
Someone should file a ticket for it so we can put it somewhere in the backlog
There was a problem hiding this comment.
This project doesn't have an issue tracker. What's the right place? Perhaps https://github.com/temporalio/sdk-go ?
There was a problem hiding this comment.
Looks like there is already an issue for this temporalio/sdk-go#61.
We're back to proto for codegen too
My previous approach failed to fix enums in nested failures. Rather than hardcode logic for handling failures specifically I took a step back and approached it another way entirely: tracking paths to fix for each proto message that is reachable from the History message. With this approach we can handle any future type that can contain itself without code changes
|
"Kicking the tires" and gogo protobufs are causing a compilation failure in my project. I'm using bazel, so there are a few "gotchas", but... it'd be nice if everything just used the v2 vanilla API. Thank you! |
That's the plan. This effort is to use the vanilla v2 API across all temporal Go repos :) |
|
Note: since the HTTP API isn't GA yet I don't plan to include shorthand JSON support in the first pass of all this work. I spoke with @yiminc yesterday about this and we can add that in after the initial release |
These are becoming necessary elsewhere
It's all proto's fault anyways
And we chatted again today. The shorthand JSON support will almost certainly be in the initial release, but I'll reimplement it in the server repo as @cretz mentioned it's the only place we need it |
.gitmodules
Outdated
| @@ -1,3 +1,4 @@ | |||
| [submodule "proto/api"] | |||
| path = proto/api | |||
| url = https://github.com/temporalio/api | |||
| url = https://github.com/tdeebswihart/temporal-api | |||
There was a problem hiding this comment.
Goes without saying I'm sure, but reminder to replace this when temporalio/api#317 merged
I've no idea why these two mocks keep swapping names. It's irritating
This is only used by the server so should stay there
There was a problem hiding this comment.
Looks good except for newly added dependency on strcase (https://github.com/temporalio/api-go/pull/119/files#r1376163691)
What changed?
gogo/protobuf has been replaced with Google's official go compiler.
Why?
gogo/protobuf has been deprecated for some time and the community is moving on, building new tools (like vtproto) atop google's v2 compiler.
Breaking changes
*time.Timein proto structs will now be timestamppb.Timestamp*time.Durationwill now be durationpb.Durationgo vetwill scream at you about thisSCREAMING_SNAKE_CASErather thanPascalCase. We decided (in discussion with the SDK team) that now was as good a time as any to rip the bandage offNote that history loading will not be impacted by the JSON changes: I rewrote history loading to dynamically fix incoming history JSON data (like all our other sdks); you can find this code in
internal/temporalhistoryv1/load.go. That will be used by our Go sdk as well as our CLI when I get there.How did you test it?
All tests for this repo pass as do all SDK tests (except for integration tests, which time out on our official repo on master...)
Potential risks
All errors that could arise from this change should be compile-time errors, so your build will be broken if I haven't yet addressed your code. I plan to port all relevant temporal repos as a part of this effort.
In addition the enum-rewriting code (the
fix-enumsmakefile target) is rather heinous; if the official go plugin for protobuf ever changes how they name their enums (which they shouldn't) it will break.Work to be done
Rewrite the custom JSON coding logic to use protojsonWill be done later and moved to the server repo