⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Conversation

@carlydf
Copy link
Collaborator

@carlydf carlydf commented Jan 14, 2026

What was changed

See title.

Why?

  • To prevent human users or agents from accidentally filling in this field without considering the full implications.
  • To test and prevent regressions

Checklist

  1. Closes

  2. How was this tested:
    New test

  3. Any docs updates needed?

tshtark and others added 6 commits December 11, 2025 13:58
Adds support for user-controlled build IDs via spec.workerOptions.buildID,
enabling rolling updates for non-workflow code changes while preserving
new deployment creation for workflow code changes.

Key changes:
- Add BuildID field to WorkerOptions struct in API types
- Update ComputeBuildID to use spec field instead of annotation
- Implement drift detection by comparing deployed spec with desired spec
- Only check for drift when buildID is explicitly set by user

Drift detection currently monitors: replicas, minReadySeconds, container
images, container resources (limits/requests), and init container images.
Other fields (env vars, volumes, commands) are not monitored - this is
documented in the BuildID field comment.

Note: CRD regeneration includes some unrelated changes from controller-gen
(default values for name fields, x-kubernetes-map-type annotations). These
are standard regeneration artifacts and don't affect functionality.

This solves deployment proliferation for PINNED versioning strategy where
any pod spec change (image tag, env vars, resources) would generate a new
build ID and create unnecessary deployments.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Replace per-field comparison with SHA256 hash of user-provided pod template
spec. This detects ALL changes (env vars, commands, volumes, etc.) rather
than a subset of fields.

Changes:
- Add ComputePodTemplateSpecHash() using spew for deterministic hashing
- Store hash in pod-template-spec-hash annotation on deployments
- Compare hashes instead of individual fields for drift detection
- Add backwards compatibility for legacy deployments without hash

Tests:
- 6 tests for hash computation (images, env vars, commands, volumes)
- Updated drift detection tests including env var change case
- Backwards compatibility test for deployments without hash annotation

Docs:
- Update BuildID field documentation to reflect hash-based detection
@carlydf carlydf requested review from a team and jlegrone as code owners January 14, 2026 01:21
@carlydf carlydf changed the title Integration test for Build ID override Rename CustomBuildID -> UnsafeCustomBuildID and integration test it Jan 14, 2026
Copy link

@eniko-dif eniko-dif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good!


func MakeBaseVersion(namespace, twdName, imageName string, status temporaliov1alpha1.VersionStatus, createDeployment, hasDeployment bool) temporaliov1alpha1.BaseWorkerDeploymentVersion {
func MakeBaseVersion(namespace, twdName, imageName, unsafeCustomBuildID string, status temporaliov1alpha1.VersionStatus, createDeployment, hasDeployment bool) temporaliov1alpha1.BaseWorkerDeploymentVersion {
buildID := unsafeCustomBuildID

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think this logic should be handled inside the MakeBuildId function similarly how ComputeBuildID works to keep build ID handling logic at the same place.

}
for _, info := range tcb.existingDeploymentInfos {
buildId := MakeBuildId(tcb.name, info.image, nil)
buildId := info.unsafeCustomBuildID

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then you also don't need to duplicate the logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants