Support Worker Deployments 3.1 #1832
Conversation
.github/workflows/ci.yml
Outdated
| @@ -87,6 +87,10 @@ jobs: | |||
| - name: Docker compose - integration tests | |||
| if: ${{ matrix.testDockerCompose }} | |||
| run: go run . integration-test | |||
| env: | |||
| # TODO(antlai-temporal): Remove this flag once server 1.27 released. | |||
| DISABLE_WORKER_DEPLOYMENT_TESTS: "1" | |||
There was a problem hiding this comment.
Why do we have two flags? My suggestion in the last PR was to just rename this one because it will be used by more then just worker deployment tests.
There was a problem hiding this comment.
I misunderstood, I though there were already other tests using that flag and you wanted both. Changing to one with your suggestion...
contrib/opentracing/go.mod
Outdated
| @@ -24,6 +24,7 @@ require ( | |||
| github.com/robfig/cron v1.2.0 // indirect | |||
| github.com/stretchr/objx v0.5.2 // indirect | |||
| go.temporal.io/api v1.44.0 // indirect | |||
| golang.org/x/exp v0.0.0-20231127185646-65229373498e // indirect | |||
There was a problem hiding this comment.
why did this change?
There was a problem hiding this comment.
I need to go mod tidy again...
There was a problem hiding this comment.
I am confused because I don't see any other dependencies upgraded, the ones in cmd/build wouldn't effect this package. Can you double check if these are actually needed?
There was a problem hiding this comment.
I think that package holds experimental and deprecated packages, and when we upgraded other packages they no longer need it... The problem is that I did a conflict merge on the go.mod and forgot to go mod tidy...
There was a problem hiding this comment.
I see what the problem is for internal/cmd/build/go.mod. I accidentally downgraded github.com/grpc-ecosystem/grpc-gateway/v2 v2.26.1 in the merge, and that requires the exp package...
There was a problem hiding this comment.
The problem is that in main we are importing api v1.44.0 at the top level, but inside internal/cmd/build/go.mod we have api v1.44.1. My PR was using 1.44.0 everywhere and that's why the go.mod was different.
Looking at the differences, there are no api differences, it is just the proxy security fix in api-go.
I'm upgrading everywhere to 1.44.1 so they get the fix...
I'm also matching the indirect deps in internal/cmd/build/go.mod with what we had.
There was a problem hiding this comment.
I'm upgrading everywhere to 1.44.1 so they get the fix...
Yes please do
internal/internal_worker.go
Outdated
| DeploymentSeriesName: options.DeploymentOptions.DeploymentSeriesName, | ||
| WorkerDeploymentVersion: options.DeploymentOptions.Version.String(), |
There was a problem hiding this comment.
Why not use the structure throughout the versioning project? Why have a stringly typed API thing that is only parsed in SDK languages? Are HTTP API and/or CLI users going to use this stringly typed form while SDK users use a structural form? Is it possible to be consistent here?
There was a problem hiding this comment.
Reverting, further discussion needed...
internal/worker.go
Outdated
| // Exposed as: [go.temporal.io/sdk/worker.NewDeploymentVersionFromString] | ||
| func NewWorkerDeploymentVersionFromString(version string) WorkerDeploymentVersion { | ||
| splitVersion := strings.SplitN(version, ".", 2) | ||
| if len(splitVersion) != 2 { |
There was a problem hiding this comment.
Why are we forcing this restriction on users? Why can't a user have any string they want in both values? Why is this not structural all the way to the server instead of imposing SDK specific rules to this stringly typed thing in every language separately? The API chose a string by intention I assume and so all API users will see a string, so can we do similarly without having custom format expectations?
There was a problem hiding this comment.
Ditto, reverting last commit...
What was changed
This PR merges the previously reviewed #1814 into main and activates
system test with the dev-server 1.27.0-128.4
It introduces Worker Deployments using the versioning 3.1 API
Checklist
Closes
Rename Worker Deployment API interfaces #1778 Add ramp to Worker Deployments #1777
How was this tested:
Both system and unit tests