Deprecate WorkerVersionStamp and replace it with Deployment #484
Conversation
…stamps # Conflicts: # openapi/openapiv2.json # openapi/openapiv3.yaml # temporal/api/common/v1/message.proto # temporal/api/enums/v1/workflow.proto # temporal/api/history/v1/message.proto # temporal/api/workflow/v1/message.proto
…stamps # Conflicts: # openapi/openapiv2.json # openapi/openapiv3.yaml # temporal/api/common/v1/message.proto
| // Must be set when versioning-3 is used (i.e. `worker_version_stamp.use_versioning` is `true` | ||
| // and `worker_version_stamp.deployment_series_name` is provided). | ||
| temporal.api.enums.v1.VersioningBehavior versioning_behavior = 15; |
There was a problem hiding this comment.
@antlai-temporal looking at the comment for versioning_behavior, I realized the stamp is not serving us well is v3 and is basically a twisted way of representing Deployment and versioned/unversioned boolean. I think Deployment and VersioningBehavior is perfect for these responsibilities, hence I'm deprecating the stamp. Please consider setting the deployment field instead of the stamp when v3 is used (WorkerDeploymentOptions is set).
|
@antlai-temporal merging this to unblock server. Will address change requests in a separate PR, if any. |
| // Must be sent if user has set a deployment series name (versioning-3). | ||
| // Deprecated. SDK should return `Deployment` in the requests to server instead of populating | ||
| // this field. | ||
| string deployment_series_name = 4; |
There was a problem hiding this comment.
Wait why is this both newly added and already deprecated?
There was a problem hiding this comment.
because we first added it. then I removed it in #483, then I realized it will break recent SDK changes that @antlai-temporal made in versioning-3 branch, then I added back in this PR as deprecated.
There was a problem hiding this comment.
I think if we don't need it and it is just in my branch we could just delete it...
| // Deprecated. Use `deployment` and `versioning_behavior` instead. | ||
| temporal.api.common.v1.WorkerVersionStamp worker_version_stamp = 10; |
There was a problem hiding this comment.
I'm guessing we don't do this consistently everywhere, but, I think deprecation warnings really should go first at the top of the comment.
Also we probably want to use the official [deprecated = true] tag: https://protobuf.dev/programming-guides/proto3/
(I'd say let's apply this everywhere we're deprecating things for this)
There was a problem hiding this comment.
Good to know. I'll add the official deprecation tag in my next PR.
What changed?
Deprecate WorkerVersionStamp and replace it with Deployment and Versioning Behavior in all APIs.
Why?
WorkerVersionStamp does not fit well with the versioning 3 concepts and terminology.
Breaking changes
SDKs that support versioning 3 need to populate Deployment in the task completion/failure responses. Values populated in WorkerVersionStamp will still be honored if Deployment is not set.
Server PR