Update Temporal Go SDK to v1.35.0 and index versions by build ID #119
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Upgrade go.temporal.io/sdk from v1.34.0 to v1.35.0 - Refactor VersionInfo to separate DeploymentName and BuildID fields - Index version collections by build ID instead of version ID - Update all Temporal API calls to use new WorkerDeploymentVersion structure - Fix PinnedVersioningOverride to use pointer and new Version field - Update tests to use new VersionInfo structure Fixes #27: Version collections now properly indexed by build ID with deployment name and build ID maintained separately.
jlegrone
force-pushed
the
jlegrone/update-sdk
branch
from
13200d1 to
5d34813
Compare
…ution - Update startWorkflowConfig struct to use separate deploymentName and buildID fields - Move version ID splitting logic from execution phase to plan generation phase - Simplify executePlan by directly using pre-separated deployment name and build ID - Add error handling for invalid version IDs during planning This improves efficiency by avoiding string parsing during the critical execution phase and better aligns with the new SDK structure that uses separate deployment name and build ID fields.
This commit completes the refactoring to eliminate redundant VersionID usage throughout the codebase: - Remove VersionID field from BaseWorkerDeploymentVersion API type and replace with BuildID field - Remove VersionID from VersionConfig struct in planner, keeping only DeploymentName and BuildID fields - Update state mapper to work directly with build IDs instead of converting from version IDs - Refactor all test data to use BuildID instead of VersionID - Update k8sState indexing to use buildID as keys throughout - Simplify execution plan logic by removing version ID splitting The external API now uses separate deployment names and build IDs as intended, eliminating the redundant combined VersionID format while maintaining backward compatibility for internal temporal state.
Only log buildID for version operations since deploymentName is redundant in the logging context.
Update integration test helpers to work with the new BuildID-based API: - Replace VersionID field references with BuildID in test validation - Update Temporal SDK calls to use BuildID instead of Version parameter - Construct version IDs from deployment name and build ID when needed - Remove unused k8s import from validation helpers Integration tests now compile and run correctly with the new API structure.
- Update kubebuilder printcolumn annotations to reference buildID fields - Regenerate CRD OpenAPI schema with buildID instead of versionID fields - Update kubectl column definitions to display buildID values - All test workflows and internal version ID handling remains unchanged The external API now consistently uses buildID terminology while preserving internal version ID functionality for workflow execution.
Refactor WorkflowConfig to use separate DeploymentName and BuildID fields instead of a combined VersionID field, eliminating the need to split version IDs back into components. Changes: - Replace WorkflowConfig.VersionID with DeploymentName and BuildID fields - Update planner to populate separate fields directly - Remove SplitVersionID calls from controller and integration tests - Delete unused SplitVersionID function and cleanup imports - Update tests to work with new structure
Get deploymentName from plan.WorkerDeploymentName instead of storing it redundantly in each workflow config. This simplifies the structure and eliminates duplication since all workflows in a plan share the same deployment name.
Remove extra blank lines in imports to satisfy fmt-imports CI check.
Address PR feedback to remove redundant comment.
Replace CurrentVersionID/RampingVersionID with CurrentBuildID/RampingBuildID in TemporalWorkerState to work directly with build IDs instead of extracting them from version ID strings. This eliminates the need for the strings import in state_mapper.go and simplifies the code by removing unnecessary string manipulation. Changes: - Update TemporalWorkerState struct to use build ID fields directly - Remove strings.TrimPrefix calls in state mapper - Remove strings import from state_mapper.go - Update all references to use new field names - Update tests to use build IDs instead of version IDs
Use targetBuildID instead of targetVersionID when looking up version info and calling GetTestWorkflowStatus, since we now work directly with build IDs.
Update GetTestWorkflowID function to take deploymentName and buildID as separate parameters instead of a combined versionID string. This eliminates the need to construct version IDs just to generate test workflow IDs. Changes: - Update GetTestWorkflowID signature to take (deploymentName, buildID, taskQueue) - Add deprecated GetTestWorkflowIDFromVersionID for backward compatibility - Update planner to call GetTestWorkflowID with separate parameters - Update all tests to use new function signature - Remove unused MakeVersionId helper function The new approach is cleaner and avoids unnecessary string construction.
The DeploymentName field in VersionConfig was redundant since the deployment name is already available in the plan context as WorkerDeploymentName. This change eliminates data duplication and improves separation of concerns by keeping deployment metadata in the plan and version-specific data in VersionConfig. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
jlegrone
commented
Aug 19, 2025
The DeploymentName field in WorkflowConfig was never used - the execution plan already uses p.WorkerDeploymentName from the plan context instead of reading from individual workflow configs. This eliminates unnecessary data duplication and keeps deployment context in the plan where it belongs.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades the Temporal Go SDK from v1.34.0 to v1.35.0 and refactors the codebase to use the new SDK's separate DeploymentName and BuildID fields instead of the combined VersionID format.
- Updates SDK to v1.35.0 which changes WorkerDeploymentVersion to use separate fields
- Refactors all version handling throughout the codebase to use buildID-based indexing
- Updates CRD schema to replace
versionIDwithbuildIDfields
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tests/internal/validation_helpers.go | Updates version comparison and API calls to use BuildID instead of combined VersionID |
| internal/tests/internal/deployment_controller.go | Replaces VersionID usage with BuildID and removes version splitting helper |
| internal/testhelpers/workers.go | Updates worker creation to use new WorkerDeploymentVersion struct |
| internal/testhelpers/make.go | Removes VersionID helper functions and updates to use BuildID |
| internal/temporal/worker_deployment_test.go | Updates test parameters to use separate deploymentName and buildID |
| internal/temporal/worker_deployment.go | Major refactor to index versions by buildID and add legacy compatibility methods |
| internal/planner/planner_test.go | Updates all test data to use BuildID instead of VersionID |
| internal/planner/planner.go | Refactors plan generation to use BuildID throughout |
| internal/k8s/deployments_test.go | Updates tests to reflect buildID-based indexing |
| internal/k8s/deployments.go | Changes deployment state indexing from versionID to buildID |
| internal/controller/state_mapper_test.go | Updates state mapping tests for new buildID structure |
| internal/controller/state_mapper.go | Refactors state mapping to use buildID instead of versionID |
| internal/controller/genstatus.go | Updates status generation to use buildID |
| internal/controller/genplan.go | Updates plan generation to use buildID |
| internal/controller/execplan.go | Updates plan execution to use new SDK API structure |
| helm/temporal-worker-controller/templates/crds/temporal.io_temporalworkerdeployments.yaml | Updates CRD schema to replace versionID with buildID |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Inline buildID variable instead of declaring it separately
jlegrone
commented
Aug 19, 2025
- Clear BuildID when resetting ramp percentage to 0 - Remove unused GetTestWorkflowIDFromVersionID function
carlydf
reviewed
Aug 20, 2025
- Update comment in worker_types.go to specify "worker deployment" - Fix function comment in planner.go to use "buildID" instead of "versionID" - Refactor createTestDeploymentWithConnection to accept separate deploymentName and buildID parameters - Update all test calls to use separate parameters instead of combined versionID string - Remove unnecessary string splitting logic and imports
- Fix error message to use "ramping deployment version" terminology - Change GetTestWorkflowID format to use colon separator instead of dot - Remove unused VersionID() method from VersionInfo struct - Update test expectations to match new colon separator format
Remove redundant explanatory text from test comments that was left over from the refactor.
carlydf
requested changes
Aug 20, 2025
Collaborator
carlydf
left a comment
carlydf
left a comment
There was a problem hiding this comment.
two nits about referring to "version" instead of "build id", then I approve!
Co-authored-by: Carly de Frondeville <[email protected]>
carlydf
approved these changes
Aug 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was changed
go.temporal.io/sdkfrom v1.34.0 to v1.35.0WorkerDeploymentVersionstructureWhy?
The new SDK version changes WorkerDeploymentVersion to use separate fields instead of a combined version ID. This refactoring eliminates the need to split and reconstruct version IDs in several places.
Checklist