Refactor job name handling and phase check (#31)

* refactor: Simplify phase check using slices.Contains and update job name handling

* refactor: Remove duplicate import of slices package

Author: ninjadq

internal/handler/operation.go

@@ -3,6 +3,7 @@ package handler
 import (
 	"context"
 	"fmt"
+	"slices"
 	"time"
 
 	"github.com/Azure/operation-cache-controller/internal/utils/reconciler"
@@ -57,13 +58,7 @@ func NewOperationHandler(ctx context.Context, operation *v1alpha1.Operation, log
 }
 
 func (o *OperationHandler) phaseIn(phases ...string) bool {
-
-	for _, phase := range phases {
-		if phase == o.operation.Status.Phase {
-			return true
-		}
-	}
-	return false
+	return slices.Contains(phases, o.operation.Status.Phase)
 }
 
 func (o *OperationHandler) EnsureNotExpired(ctx context.Context) (reconciler.OperationResult, error) {

internal/handler/operation_test.go

@@ -42,14 +42,17 @@ var (
 				},
 			},
 		},
+		Status: v1alpha1.OperationStatus{
+			OperationID: "test-operation",
+		},
 	}
 	emptyAppDeploymentList = &v1alpha1.AppDeploymentList{}
 
 	validAppDeploymentList = &v1alpha1.AppDeploymentList{
 		Items: []v1alpha1.AppDeployment{
 			{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-app1",
+					Name:      "test-operation-test-app1",
 					Namespace: "default",
 				},
 				Spec: v1alpha1.AppDeploymentSpec{
@@ -64,7 +67,7 @@ var (
 			},
 			{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-app2",
+					Name:      "test-operation-test-app2",
 					Namespace: "default",
 				},
 				Spec: v1alpha1.AppDeploymentSpec{
@@ -83,7 +86,7 @@ var (
 		Items: []v1alpha1.AppDeployment{
 			{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-app2",
+					Name:      "test-operation-test-app2",
 					Namespace: "default",
 				},
 				Spec: v1alpha1.AppDeploymentSpec{
@@ -115,7 +118,7 @@ var (
 			},
 			{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-app3",
+					Name:      "test-operation-test-app3",
 					Namespace: "default",
 				},
 				Spec: v1alpha1.AppDeploymentSpec{
@@ -277,14 +280,14 @@ func TestOperationHandler_EnsureAllAppsAreReady(t *testing.T) {
 		operation.Status.Phase = v1alpha1.OperationPhaseReconciling
 
 		appList := emptyAppDeploymentList.DeepCopy()
-		mockClient.EXPECT().List(ctx, appList, gomock.Any()).DoAndReturn(func(ctx context.Context, list *v1alpha1.AppDeploymentList, opts ...interface{}) error {
+		mockClient.EXPECT().List(ctx, appList, gomock.Any()).DoAndReturn(func(ctx context.Context, list *v1alpha1.AppDeploymentList, opts ...any) error {
 			*list = *changedValidAppDeploymentList
 			return nil
 		})
 		scheme := runtime.NewScheme()
 		utilruntime.Must(v1alpha1.AddToScheme(scheme))
 		mockClient.EXPECT().Scheme().Return(scheme).AnyTimes()
-		mockClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key client.ObjectKey, obj runtime.Object, opt ...interface{}) error {
+		mockClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).DoAndReturn(func(ctx context.Context, key client.ObjectKey, obj runtime.Object, opt ...any) error {
 			*obj.(*v1alpha1.AppDeployment) = v1alpha1.AppDeployment{}
 			return nil
 		}).AnyTimes()

internal/handler/requirement.go

@@ -286,7 +286,7 @@ func (r *RequirementHandler) EnsureOperationReady(ctx context.Context) (reconcil
 	}
 	if r.rqutils.IsCacheMissed(r.requirement) {
 		r.logger.V(1).Info("cache missed, creating operation")
-		r.requirement.Status.OperationName = r.requirement.Name + "-" + "operation"
+		r.requirement.Status.OperationName = r.requirement.Name // reset operation name to requirement name
 	}
 	// check operation status
 	if op, err := r.getOperation(); err == nil {

internal/utils/controller/appdeployment_job.go

@@ -13,23 +13,20 @@ import (
 const (
 	JobTypeProvision = "provision"
 	JobTypeTeardown  = "teardown"
+	JobNamePattern   = "%s-%s-%s" // jobType-appName-operationId
 )
 
 var (
 	backOffLimit            int32 = 10
 	ttlSecondsAfterFinished int32 = 3600
 	MaxAppNameLength        int   = 36
-	MaxOpIdLength           int   = 18
 )
 
-func validJName(appName, operationId, jobType string) string {
-	if len(appName) > MaxAppNameLength {
-		appName = appName[:MaxAppNameLength]
-	}
-	if len(operationId) > MaxOpIdLength {
-		operationId = operationId[:MaxOpIdLength]
+func validJobName(appName, jobType string) string {
+	originName := jobType + "-" + appName
+	if len(originName) > MaxResourceNameLength {
+		return originName[:MaxResourceNameLength]
 	}
-	originName := jobType + "-" + appName + "-" + operationId
 	return originName
 }
 
@@ -42,16 +39,16 @@ func TeardownJobFromAppDeploymentSpec(appDeployment *v1alpha1.AppDeployment) *ba
 }
 
 func GetProvisionJobName(appDeployment *v1alpha1.AppDeployment) string {
-	return validJName(appDeployment.Name, appDeployment.Spec.OpId, JobTypeProvision)
+	return validJobName(appDeployment.Name, JobTypeProvision)
 }
 
 func GetTeardownJobName(appDeployment *v1alpha1.AppDeployment) string {
-	return validJName(appDeployment.Name, appDeployment.Spec.OpId, JobTypeTeardown)
+	return validJobName(appDeployment.Name, JobTypeTeardown)
 }
 
 func jobFromAppDeploymentSpec(appDeployment *v1alpha1.AppDeployment, suffix string) *batchv1.Job {
 	ops := jobOptions{
-		name:        validJName(appDeployment.Name, appDeployment.Spec.OpId, suffix),
+		name:        validJobName(appDeployment.Name, suffix),
 		namespace:   appDeployment.Namespace,
 		labels:      appDeployment.Labels,
 		jobSpec:     appDeployment.Spec.Provision,
@@ -117,14 +114,16 @@ func CheckJobStatus(ctx context.Context, job *batchv1.Job) JobStatus {
 }
 
 func OperationScopedAppDeployment(appName, opId string) string {
+	originalName := opId + "-" + appName
+	if len(originalName) < MaxResourceNameLength {
+		return originalName
+	}
 	if len(appName) > MaxAppNameLength {
 		appName = appName[:MaxAppNameLength]
 	}
-	if len(opId) > MaxOpIdLength {
-		opId = opId[:MaxOpIdLength]
-	}
-	if opId == "" {
-		return appName
+	resisualLength := MaxResourceNameLength - len(appName) - 1 // -1 for the hyphen
+	if len(opId) > resisualLength {
+		opId = opId[:resisualLength]
 	}
 	return opId + "-" + appName
 }

internal/utils/controller/appdeployment_job_test.go

@@ -2,6 +2,7 @@ package controller
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	"github.com/Azure/operation-cache-controller/api/v1alpha1"
@@ -94,21 +95,18 @@ func TestGetProvisionJobName(t *testing.T) {
 	}{
 		{
 			name:     "Valid app name and operation ID",
-			appName:  "my-app",
-			opId:     "op123",
-			expected: "provision-my-app-op123",
+			appName:  "op123-my-app",
+			expected: "provision-op123-my-app",
 		},
 		{
 			name:     "App name exceeds max length",
-			appName:  "a-very-long-application-name-exceeding-limit",
-			opId:     "op123",
-			expected: "provision-a-very-long-application-name-exceedi-op123",
+			appName:  "op1234567890-a-very-long-application-name-exceeding-limit",
+			expected: "provision-op1234567890-a-very-long-application-name-exceeding-l",
 		},
 		{
 			name:     "Operation ID exceeds max length",
-			appName:  "my-app",
-			opId:     "operation-id-exceeding-length",
-			expected: "provision-my-app-operation-id-excee",
+			appName:  "operationid1234567890123456789012345678901234567890-my-application",
+			expected: "provision-operationid1234567890123456789012345678901234567890-m",
 		},
 	}
 
@@ -118,9 +116,6 @@ func TestGetProvisionJobName(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Name: tt.appName,
 				},
-				Spec: v1alpha1.AppDeploymentSpec{
-					OpId: tt.opId,
-				},
 			}
 			res := GetProvisionJobName(appDeployment)
 			assert.Equal(t, tt.expected, res)
@@ -138,20 +133,17 @@ func TestGetTeardownJobName(t *testing.T) {
 		{
 			name:     "Valid app name and operation ID",
 			appName:  "my-app",
-			opId:     "op123",
-			expected: "teardown-my-app-op123",
+			expected: "teardown-my-app",
 		},
 		{
 			name:     "App name exceeds max length",
-			appName:  "a-very-long-application-name-exceeding-limit",
-			opId:     "op123",
-			expected: "teardown-a-very-long-application-name-exceedi-op123",
+			appName:  "op1234567890-a-very-long-application-name-exceeding-limit",
+			expected: "teardown-op1234567890-a-very-long-application-name-exceeding-li",
 		},
 		{
 			name:     "Operation ID exceeds max length",
-			appName:  "my-app",
-			opId:     "operation-id-exceeding-length",
-			expected: "teardown-my-app-operation-id-excee",
+			appName:  "operationid1234567890123456789012345678901234567890-my-application",
+			expected: "teardown-operationid1234567890123456789012345678901234567890-my",
 		},
 	}
 
@@ -161,9 +153,6 @@ func TestGetTeardownJobName(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Name: tt.appName,
 				},
-				Spec: v1alpha1.AppDeploymentSpec{
-					OpId: tt.opId,
-				},
 			}
 			res := GetTeardownJobName(appDeployment)
 			assert.Equal(t, tt.expected, res)
@@ -172,59 +161,91 @@ func TestGetTeardownJobName(t *testing.T) {
 }
 func TestOperationScopedAppDeployment(t *testing.T) {
 	tests := []struct {
-		name     string
-		appName  string
-		opId     string
-		expected string
+		name    string
+		appName string
+		opId    string
+		want    string
 	}{
 		{
-			name:     "Both appName and opId within limits",
-			appName:  "my-app",
-			opId:     "op123",
-			expected: "op123-my-app",
+			name:    "normal case - combined length within limit",
+			appName: "my-app",
+			opId:    "op-12345",
+			want:    "op-12345-my-app",
 		},
 		{
-			name:     "appName exceeds max length",
-			appName:  "a-very-long-application-name-exceeding-limit",
-			opId:     "op123",
-			expected: "op123-a-very-long-application-name-exceedi",
+			name:    "app name truncated when too long",
+			appName: strings.Repeat("a", 60), // 60 chars, exceeds MaxAppNameLength
+			opId:    "op-12345",
+			want:    "op-12345-" + strings.Repeat("a", MaxAppNameLength),
 		},
 		{
-			name:     "opId exceeds max length",
-			appName:  "my-app",
-			opId:     "operation-id-exceeding-length",
-			expected: "operation-id-excee-my-app",
+			name:    "operation ID truncated when combined length exceeds limit",
+			appName: strings.Repeat("a", MaxAppNameLength),
+			opId:    strings.Repeat("b", 30), // This will exceed when combined
+			want:    strings.Repeat("b", MaxResourceNameLength-MaxAppNameLength-1) + "-" + strings.Repeat("a", MaxAppNameLength),
 		},
 		{
-			name:     "Both appName and opId exceed max length",
-			appName:  "another-very-long-application-name-exceeding-limit",
-			opId:     "another-operation-id-exceeding-length",
-			expected: "another-operation--another-very-long-application-name-e",
+			name:    "both truncated for very long inputs",
+			appName: strings.Repeat("a", 100),
+			opId:    strings.Repeat("b", 100),
+			want:    strings.Repeat("b", MaxResourceNameLength-MaxAppNameLength-1) + "-" + strings.Repeat("a", MaxAppNameLength),
 		},
 		{
-			name:     "Empty appName and opId",
-			appName:  "",
-			opId:     "",
-			expected: "",
+			name:    "empty app name",
+			appName: "",
+			opId:    "op-12345",
+			want:    "op-12345-",
 		},
 		{
-			name:     "Empty appName",
-			appName:  "",
-			opId:     "op123",
-			expected: "op123-",
+			name:    "empty operation ID",
+			appName: "my-app",
+			opId:    "",
+			want:    "-my-app",
 		},
 		{
-			name:     "Empty opId",
-			appName:  "my-app",
-			opId:     "",
-			expected: "my-app",
+			name:    "both empty",
+			appName: "",
+			opId:    "",
+			want:    "-",
+		},
+		{
+			name:    "exact max length",
+			appName: "app",
+			opId:    strings.Repeat("x", MaxResourceNameLength-4), // -4 for "-app"
+			want:    strings.Repeat("x", MaxResourceNameLength-4) + "-app",
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			res := OperationScopedAppDeployment(tt.appName, tt.opId)
-			assert.Equal(t, tt.expected, res)
+			got := OperationScopedAppDeployment(tt.appName, tt.opId)
+			assert.Equal(t, tt.want, got, "OperationScopedAppDeployment() mismatch for %s", tt.name)
 		})
 	}
 }
+
+func TestOperationScopedAppDeployment_LengthConstraints(t *testing.T) {
+	// Test that the function always respects the maximum length constraint
+	testCases := []struct {
+		appNameLen int
+		opIdLen    int
+	}{
+		{10, 10},
+		{MaxAppNameLength, 10},
+		{50, 50},
+		{100, 100},
+		{MaxResourceNameLength, MaxResourceNameLength},
+	}
+
+	for _, tc := range testCases {
+		appName := strings.Repeat("a", tc.appNameLen)
+		opId := strings.Repeat("b", tc.opIdLen)
+
+		result := OperationScopedAppDeployment(appName, opId)
+
+		if len(result) > MaxResourceNameLength {
+			t.Errorf("Result length %d exceeds MaxResourceNameLength %d for appName length %d and opId length %d",
+				len(result), MaxResourceNameLength, tc.appNameLen, tc.opIdLen)
+		}
+	}
+}

internal/utils/controller/const.go

@@ -2,11 +2,11 @@ package controller
 
 const (
 	LabelNameCacheKey = "operation-cache-controller.azure.github.com/cache-key"
-)
 
-const (
 	AnnotationNameCacheMode = "operation-cache-controller.azure.github.com/cache-mode"
 	AnnotationNameCacheKey  = "operation-cache-controller.azure.github.com/cache-key"
 	AnnotationValueTrue     = "true"
 	AnnotationValueFalse    = "false"
+
+	MaxResourceNameLength int = 63
 )