💥 [Breaking] Update versioning APIs to use struct #1962
Conversation
Sushisource
force-pushed
the
update-versioning-apis
branch
3 times, most recently
from
098c12e to
b182d26
Compare
Sushisource
force-pushed
the
update-versioning-apis
branch
2 times, most recently
from
0f29dce to
22787a4
Compare
Sushisource
force-pushed
the
update-versioning-apis
branch
from
22787a4 to
9257ab5
Compare
| // Ignore connection loss on server shutdown. This helps with quiescing spurious error messages | ||
| // upon server shutdown (where server is using the SDK). | ||
| st, ok := status.FromError(err) | ||
| if ok && st.Code() == codes.Unavailable && strings.Contains(st.Message(), "graceful_stop") { | ||
| return | ||
| } |
There was a problem hiding this comment.
This is unrelated, but, just wanted to do it while I was in here - helps shut up some pointless logspew in the dev server.
There was a problem hiding this comment.
but a normal user would want to get logs if the poller received errors from the server is shutting down
There was a problem hiding this comment.
Yeah, that's a good point. We'd need some kind of indication we're in the server environment.
There was a problem hiding this comment.
Got some advice from David on what to check
| // NOTE: Experimental | ||
| WorkflowExecutionOptionsChanges struct { | ||
| VersioningOverride *VersioningOverride | ||
| VersioningOverride *VersioningOverrideChange |
There was a problem hiding this comment.
nit: Add docs and move the comment about nil down here
There was a problem hiding this comment.
I think you still want it up top for the future, but added it here too.
internal/worker_deployments.go
Outdated
| } | ||
| } | ||
|
|
||
| func (wd *WorkerDeploymentVersion) ToCanonicalString() string { |
There was a problem hiding this comment.
should this be also private the same way that workerDeploymentVersionFromString is? it is only useful for the deprecated fields, right?
Sushisource
force-pushed
the
update-versioning-apis
branch
from
812ab1a to
7dcae8f
Compare
Sushisource
force-pushed
the
update-versioning-apis
branch
from
7dcae8f to
fbddace
Compare
| go.work.sum | ||
| *~ | ||
| *~ | ||
| internal/cmd/build/*.sqlite* |
There was a problem hiding this comment.
Is something creating this file? (no comment on the review itself, LGTM)
There was a problem hiding this comment.
Yeah, when you run the integ tests w/ dev server mode
There was a problem hiding this comment.
@Quinn-With-Two-Ns - I see we changed from in-memory mode to DB filename mode via DBFilename: "temporal.sqlite" in #1893. Was that intentional?
There was a problem hiding this comment.
Why? I am concerned successive tests will reuse a DB from a previous test (and in general our tests should be able to run via in memory dev server).
DRAFT just to make sure tests pass & give myself a review pass
What was changed
Change APIs to use struct form instead of string form for worker deployment versions & update protos & use new fields in them.
Why?
Much more usable and less error prone, consistent with other SDKs
Checklist
Closes
How was this tested:
Existing tests
Any docs updates needed?