Keep dots in Build IDs and format Worker Deployment names <k8s-namespace>/<twd-name> (#133)

<!--- Note to EXTERNAL Contributors -->
<!-- Thanks for opening a PR! 
If it is a significant code change, please **make sure there is an open
issue** for this.
We work best with you when we have accepted the idea first before you
code. -->

<!--- For ALL Contributors 👇 -->

## What was changed
Keep dots in Build IDs and format Worker Deployment names
`<k8s-namespace>/<twd-name>`

## Why?
Dots are allowed in pod names and its nice to maintain the semantic
version appearance of an image.
Namespace first means the same alphabetization as `kubectl list
--all-namespaces`.
Namespace first is more expected for a slash delimiter that implies
hierarchy

## Checklist
<!--- add/delete as needed --->

1. Closes <!-- add issue number here -->

2. How was this tested:
Unit tests and functional tests

3. Any docs updates needed?
<!--- update README if applicable
      or point out where to update docs.temporal.io -->

Author: carlydf

internal/k8s/deployments.go

@@ -26,12 +26,11 @@ import (
 const (
 	DeployOwnerKey = ".metadata.controller"
 	// BuildIDLabel is the label that identifies the build ID for a deployment
-	BuildIDLabel                 = "temporal.io/build-id"
-	DeploymentNameSeparator      = "/" // TODO(carlydf): change this to "." once the server accepts `.` in deployment names
-	VersionIDSeparator           = "." // TODO(carlydf): change this to ":"
-	K8sResourceNameSeparator     = "-"
-	MaxBuildIdLen                = 63
-	ConnectionSpecHashAnnotation = "temporal.io/connection-spec-hash"
+	BuildIDLabel                  = "temporal.io/build-id"
+	WorkerDeploymentNameSeparator = "/"
+	K8sResourceNameSeparator      = "-"
+	MaxBuildIdLen                 = 63
+	ConnectionSpecHashAnnotation  = "temporal.io/connection-spec-hash"
 )
 
 // DeploymentState represents the Kubernetes state of all deployments for a temporal worker deployment
@@ -111,11 +110,6 @@ func NewObjectRef(obj client.Object) *corev1.ObjectReference {
 	}
 }
 
-// ComputeVersionID generates a version ID from the worker deployment spec
-func ComputeVersionID(w *temporaliov1alpha1.TemporalWorkerDeployment) string {
-	return ComputeWorkerDeploymentName(w) + VersionIDSeparator + ComputeBuildID(w)
-}
-
 func ComputeBuildID(w *temporaliov1alpha1.TemporalWorkerDeployment) string {
 	if containers := w.Spec.Template.Spec.Containers; len(containers) > 0 {
 		if img := containers[0].Image; img != "" {
@@ -131,7 +125,7 @@ func ComputeBuildID(w *temporaliov1alpha1.TemporalWorkerDeployment) string {
 // ComputeWorkerDeploymentName generates the base worker deployment name
 func ComputeWorkerDeploymentName(w *temporaliov1alpha1.TemporalWorkerDeployment) string {
 	// Use the name and namespace to form the worker deployment name
-	return w.GetName() + DeploymentNameSeparator + w.GetNamespace()
+	return w.GetNamespace() + WorkerDeploymentNameSeparator + w.GetName()
 }
 
 // ComputeVersionedDeploymentName generates a name for a versioned deployment
@@ -162,8 +156,8 @@ func CleanAndTruncateString(s string, n int) string {
 	if len(s) > n && n > 0 {
 		s = s[:n]
 	}
-	// Keep only letters, numbers, and dashes
-	re := regexp.MustCompile(`[^a-zA-Z0-9-]+`)
+	// Keep only letters, numbers, dashes, and dots
+	re := regexp.MustCompile(`[^a-zA-Z0-9-.]+`)
 	return re.ReplaceAllString(s, K8sResourceNameSeparator)
 }
 

internal/k8s/deployments_test.go

@@ -235,7 +235,7 @@ func TestGenerateBuildID(t *testing.T) {
 				twd2 := testhelpers.MakeTWD("", "", 1, pod2, nil, nil, nil)
 				return twd1, twd2
 			},
-			expectedPrefix:  "my-test-image",
+			expectedPrefix:  "my.test-image",
 			expectedHashLen: 4,
 			expectEquality:  false, // should be different
 		},
@@ -248,7 +248,7 @@ func TestGenerateBuildID(t *testing.T) {
 				twd2 := testhelpers.MakeTWD("", "", 2, pod, nil, nil, nil)
 				return twd1, twd2
 			},
-			expectedPrefix:  "my-test-image",
+			expectedPrefix:  "my.test-image",
 			expectedHashLen: 4,
 			expectEquality:  true, // should be the same
 		},
@@ -334,7 +334,7 @@ func TestGenerateBuildID(t *testing.T) {
 				twd := testhelpers.MakeTWDWithImage("", "", illegalCharsImg)
 				return twd, nil // only check 1 result, no need to compare
 			},
-			expectedPrefix:  "this-is-my-weird-image",
+			expectedPrefix:  "this.is.my-weird-image",
 			expectedHashLen: 4,
 			expectEquality:  false,
 		},
@@ -450,15 +450,9 @@ func TestComputeWorkerDeploymentName_Integration_WithVersionedName(t *testing.T)
 	versionedName := k8s.ComputeVersionedDeploymentName(workerDeploymentName, buildID)
 
 	// Verify the expected formats
-	assert.Equal(t, "hello-world"+k8s.DeploymentNameSeparator+"demo", workerDeploymentName)
-	assert.True(t, strings.HasPrefix(versionedName, "hello-world"+k8s.DeploymentNameSeparator+"demo-"))
-	assert.True(t, strings.Contains(versionedName, "v1-0-0"), "versioned name should contain cleaned image tag")
-
-	// Verify the version ID combines worker deployment name and build ID
-	versionID := k8s.ComputeVersionID(twd)
-	expectedVersionID := workerDeploymentName + k8s.VersionIDSeparator + buildID
-	assert.Equal(t, expectedVersionID, versionID)
-	assert.Equal(t, "hello-world"+k8s.DeploymentNameSeparator+"demo"+k8s.VersionIDSeparator+"v1-0-0-dd84", versionID)
+	assert.Equal(t, "demo"+k8s.WorkerDeploymentNameSeparator+"hello-world", workerDeploymentName)
+	assert.True(t, strings.HasPrefix(versionedName, "demo"+k8s.WorkerDeploymentNameSeparator+"hello-world-"))
+	assert.True(t, strings.Contains(versionedName, "v1.0.0"), "versioned name should contain cleaned image tag")
 }
 
 // TestNewDeploymentWithPodAnnotations tests that every new pod created has a connection spec hash annotation

internal/tests/internal/integration_test.go

@@ -47,53 +47,53 @@ func TestIntegration(t *testing.T) {
 			WithInput(
 				testhelpers.NewTemporalWorkerDeploymentBuilder().
 					WithManualStrategy().
-					WithTargetTemplate("v1"),
+					WithTargetTemplate("v1.0"),
 			).
 			WithWaitTime(5 * time.Second). // wait before checking to confirm no change
 			WithExpectedStatus(
 				testhelpers.NewStatusBuilder().
-					WithTargetVersion("v1", temporaliov1alpha1.VersionStatusInactive, -1, true, false),
+					WithTargetVersion("v1.0", temporaliov1alpha1.VersionStatusInactive, -1, true, false),
 			),
 		"all-at-once-rollout-2-replicas": testhelpers.NewTestCase().
 			WithInput(
 				testhelpers.NewTemporalWorkerDeploymentBuilder().
 					WithAllAtOnceStrategy().
 					WithReplicas(2).
-					WithTargetTemplate("v1"),
+					WithTargetTemplate("v1.0"),
 			).
 			WithExpectedStatus(
 				testhelpers.NewStatusBuilder().
-					WithTargetVersion("v1", temporaliov1alpha1.VersionStatusCurrent, -1, true, false).
-					WithCurrentVersion("v1", true, false),
+					WithTargetVersion("v1.0", temporaliov1alpha1.VersionStatusCurrent, -1, true, false).
+					WithCurrentVersion("v1.0", true, false),
 			),
 		"progressive-rollout-no-unversioned-pollers-expect-all-at-once": testhelpers.NewTestCase().
 			WithInput(
 				testhelpers.NewTemporalWorkerDeploymentBuilder().
 					WithProgressiveStrategy(testhelpers.ProgressiveStep(5, time.Hour)).
-					WithTargetTemplate("v1"),
+					WithTargetTemplate("v1.0"),
 			).
 			WithExpectedStatus(
 				testhelpers.NewStatusBuilder().
-					WithTargetVersion("v1", temporaliov1alpha1.VersionStatusCurrent, -1, true, false).
-					WithCurrentVersion("v1", true, false),
+					WithTargetVersion("v1.0", temporaliov1alpha1.VersionStatusCurrent, -1, true, false).
+					WithCurrentVersion("v1.0", true, false),
 			),
 		//// TODO(carlydf): this won't work until the controller detects unversioned pollers
 		// "progressive-rollout-yes-unversioned-pollers-expect-first-step": testhelpers.NewTestCase().
 		//	WithInput(
 		//		testhelpers.NewTemporalWorkerDeploymentBuilder().
 		//			WithProgressiveStrategy(testhelpers.ProgressiveStep(5, time.Hour)).
-		//			WithTargetTemplate("v1"),
+		//			WithTargetTemplate("v1.0"),
 		//	).
 		//	WithSetupFunction(setupUnversionedPoller).
 		//	WithExpectedStatus(
 		//		testhelpers.NewStatusBuilder().
-		//			WithTargetVersion("v1", temporaliov1alpha1.VersionStatusRamping, 5, true, false),
+		//			WithTargetVersion("v1.0", temporaliov1alpha1.VersionStatusRamping, 5, true, false),
 		//	),
 		"nth-progressive-rollout-expect-first-step": testhelpers.NewTestCase().
 			WithInput(
 				testhelpers.NewTemporalWorkerDeploymentBuilder().
 					WithProgressiveStrategy(testhelpers.ProgressiveStep(5, time.Hour)).
-					WithTargetTemplate("v1").
+					WithTargetTemplate("v1.0").
 					WithStatus(
 						testhelpers.NewStatusBuilder().
 							WithTargetVersion("v0", temporaliov1alpha1.VersionStatusCurrent, -1, true, true).
@@ -105,14 +105,14 @@ func TestIntegration(t *testing.T) {
 			).
 			WithExpectedStatus(
 				testhelpers.NewStatusBuilder().
-					WithTargetVersion("v1", temporaliov1alpha1.VersionStatusRamping, 5, true, false),
+					WithTargetVersion("v1.0", temporaliov1alpha1.VersionStatusRamping, 5, true, false),
 			),
 		"nth-progressive-rollout-with-success-gate": testhelpers.NewTestCase().
 			WithInput(
 				testhelpers.NewTemporalWorkerDeploymentBuilder().
 					WithProgressiveStrategy(testhelpers.ProgressiveStep(5, time.Hour)).
 					WithGate(true).
-					WithTargetTemplate("v1").
+					WithTargetTemplate("v1.0").
 					WithStatus(
 						testhelpers.NewStatusBuilder().
 							WithTargetVersion("v0", temporaliov1alpha1.VersionStatusCurrent, -1, true, true).
@@ -124,14 +124,14 @@ func TestIntegration(t *testing.T) {
 			).
 			WithExpectedStatus(
 				testhelpers.NewStatusBuilder().
-					WithTargetVersion("v1", temporaliov1alpha1.VersionStatusRamping, 5, true, false),
+					WithTargetVersion("v1.0", temporaliov1alpha1.VersionStatusRamping, 5, true, false),
 			),
 		"nth-progressive-rollout-with-failed-gate": testhelpers.NewTestCase().
 			WithInput(
 				testhelpers.NewTemporalWorkerDeploymentBuilder().
 					WithProgressiveStrategy(testhelpers.ProgressiveStep(5, time.Hour)).
 					WithGate(false).
-					WithTargetTemplate("v1").
+					WithTargetTemplate("v1.0").
 					WithStatus(
 						testhelpers.NewStatusBuilder().
 							WithTargetVersion("v0", temporaliov1alpha1.VersionStatusCurrent, -1, true, true).
@@ -144,52 +144,52 @@ func TestIntegration(t *testing.T) {
 			WithWaitTime(5 * time.Second).
 			WithExpectedStatus(
 				testhelpers.NewStatusBuilder().
-					WithTargetVersion("v1", temporaliov1alpha1.VersionStatusInactive, -1, true, false).
+					WithTargetVersion("v1.0", temporaliov1alpha1.VersionStatusInactive, -1, true, false).
 					WithCurrentVersion("v0", true, true),
 			),
 		"failed-gate-is-not-scaled-down-while-target": testhelpers.NewTestCase().
 			WithInput(
 				testhelpers.NewTemporalWorkerDeploymentBuilder().
 					WithAllAtOnceStrategy().
 					WithGate(false).
-					WithTargetTemplate("v1"),
+					WithTargetTemplate("v1.0"),
 			).
 			WithWaitTime(5 * time.Second).
 			WithExpectedStatus(
 				testhelpers.NewStatusBuilder().
-					WithTargetVersion("v1", temporaliov1alpha1.VersionStatusInactive, -1, true, false),
+					WithTargetVersion("v1.0", temporaliov1alpha1.VersionStatusInactive, -1, true, false),
 			),
 		"failed-gate-is-scaled-down-when-deprecated": testhelpers.NewTestCase().
 			WithInput(
 				testhelpers.NewTemporalWorkerDeploymentBuilder().
 					WithAllAtOnceStrategy().
-					WithTargetTemplate("v2").
+					WithTargetTemplate("v2.0").
 					WithStatus(
 						testhelpers.NewStatusBuilder().
-							WithTargetVersion("v1", temporaliov1alpha1.VersionStatusInactive, -1, true, true).
+							WithTargetVersion("v1.0", temporaliov1alpha1.VersionStatusInactive, -1, true, true).
 							WithDeprecatedVersions(
 								testhelpers.NewDeprecatedVersionInfo("v0", temporaliov1alpha1.VersionStatusDrained, true, true, true),
 							),
 					),
 			).
 			WithExistingDeployments(
 				testhelpers.NewDeploymentInfo("v0", 1),
-				testhelpers.NewDeploymentInfo("v1", 1),
+				testhelpers.NewDeploymentInfo("v1.0", 1),
 			).
 			WithWaitTime(5*time.Second).
 			WithExpectedStatus(
 				testhelpers.NewStatusBuilder().
-					WithTargetVersion("v2", temporaliov1alpha1.VersionStatusCurrent, -1, true, false).
-					WithCurrentVersion("v2", true, false).
+					WithTargetVersion("v2.0", temporaliov1alpha1.VersionStatusCurrent, -1, true, false).
+					WithCurrentVersion("v2.0", true, false).
 					WithDeprecatedVersions(
 						testhelpers.NewDeprecatedVersionInfo("v0", temporaliov1alpha1.VersionStatusDrained, true, false, true),
-						testhelpers.NewDeprecatedVersionInfo("v1", temporaliov1alpha1.VersionStatusInactive, true, false, true),
+						testhelpers.NewDeprecatedVersionInfo("v1.0", temporaliov1alpha1.VersionStatusInactive, true, false, true),
 					),
 			).
 			WithExpectedDeployments( // note: right now this is only checked for deprecated versions, TODO(carlydf) add for non-deprecated too
 				testhelpers.NewDeploymentInfo("v0", 1),
-				testhelpers.NewDeploymentInfo("v1", 0),
-				testhelpers.NewDeploymentInfo("v2", 1),
+				testhelpers.NewDeploymentInfo("v1.0", 0),
+				testhelpers.NewDeploymentInfo("v2.0", 1),
 			),
 		"nth-rollout-blocked-by-modifier": testhelpers.NewTestCase().
 			WithInput(