Repository: akuity/kargo
Pull request #1634 (2024-03-15 21:45:00)

fix: make stage defaulting webhook run on update
This was missing from #1584

netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *docs-kargo-akuity-io* processing.


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 509b155a4d7fc392b2ef27fac14c34ed08e54ebb |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/65f4c15f3123f000070b9b69 |
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/akuity/kargo/pull/1634?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 43.96%. Comparing base [(`f2149ef`)](https://app.codecov.io/gh/akuity/kargo/commit/f2149ef2b0ed8130b0e9929df60e73a4533802d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) to head [(`509b155`)](https://app.codecov.io/gh/akuity/kargo/pull/1634?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).


<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##             main    #1634   +/-   ##
=======================================
  Coverage   43.96%   43.96%           
=======================================
  Files         199      199           
  Lines       12673    12673           
=======================================
  Hits         5572     5572           
  Misses       6864     6864           
  Partials      237      237           
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/akuity/kargo/pull/1634?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).


Patch:
diff --git a/charts/kargo/templates/webhooks/webhooks.yaml b/charts/kargo/templates/webhooks/webhooks.yaml
index 9859139f..2428eea4 100644
--- a/charts/kargo/templates/webhooks/webhooks.yaml
+++ b/charts/kargo/templates/webhooks/webhooks.yaml
@@ -52,7 +52,7 @@ webhooks:
     apiGroups: ["kargo.akuity.io"]
     apiVersions: ["v1alpha1"]
     resources: ["stages"]
-    operations: ["CREATE"]
+    operations: ["CREATE", "UPDATE"]
   failurePolicy: Fail
 ---
 apiVersion: admissionregistration.k8s.io/v1
diff --git a/internal/kargo/kargo.go b/internal/kargo/kargo.go
index 64a731cd..0b3816da 100644
--- a/internal/kargo/kargo.go
+++ b/internal/kargo/kargo.go
@@ -20,8 +20,8 @@ const (
 	maxStageNamePrefixLength = 218
 )
 
-// NewPromotion returns a new Promotion from a given stage and freight with our naming convention.
-// Ensures the owner reference is set to be the stage, and carries over any shard labels
+// NewPromotion returns a new Promotion from a given stage and freight with our
+// naming convention.
 func NewPromotion(stage kargoapi.Stage, freight string) kargoapi.Promotion {
 	shortHash := freight
 	if len(shortHash) > 7 {
@@ -36,24 +36,16 @@ func NewPromotion(stage kargoapi.Stage, freight string) kargoapi.Promotion {
 	// We just want a unique ID that can be sorted lexicographically
 	promoName := strings.ToLower(fmt.Sprintf("%s.%s.%s", shortStageName, ulid.Make(), shortHash))
 
-	ownerRef := metav1.NewControllerRef(&stage, kargoapi.GroupVersion.WithKind("Stage"))
-
 	promotion := kargoapi.Promotion{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:            promoName,
-			Namespace:       stage.Namespace,
-			OwnerReferences: []metav1.OwnerReference{*ownerRef},
+			Name:      promoName,
+			Namespace: stage.Namespace,
 		},
 		Spec: &kargoapi.PromotionSpec{
 			Stage:   stage.Name,
 			Freight: freight,
 		},
 	}
-	if stage.Labels != nil && stage.Labels[kargoapi.ShardLabelKey] != "" {
-		promotion.ObjectMeta.Labels = map[string]string{
-			kargoapi.ShardLabelKey: stage.Labels[kargoapi.ShardLabelKey],
-		}
-	}
 	return promotion
 }
 
diff --git a/internal/webhook/promotion/webhook.go b/internal/webhook/promotion/webhook.go
index fe8a86a4..2f640440 100644
--- a/internal/webhook/promotion/webhook.go
+++ b/internal/webhook/promotion/webhook.go
@@ -110,6 +110,17 @@ func (w *webhook) Default(ctx context.Context, obj runtime.Object) error {
 			promo.Namespace,
 		)
 	}
+
+	// Make sure the Promotion has the same shard as the Stage
+	if stage.Spec.Shard != "" {
+		if promo.Labels == nil {
+			promo.Labels = make(map[string]string, 1)
+		}
+		promo.Labels[kargoapi.ShardLabelKey] = stage.Spec.Shard
+	} else {
+		delete(promo.Labels, kargoapi.ShardLabelKey)
+	}
+
 	ownerRef :=
 		metav1.NewControllerRef(stage, kargoapi.GroupVersion.WithKind("Stage"))
 	promo.ObjectMeta.OwnerReferences = []metav1.OwnerReference{*ownerRef}


Test patch:
diff --git a/internal/kargo/kargo_test.go b/internal/kargo/kargo_test.go
index c93d42db..c01d9ef9 100644
--- a/internal/kargo/kargo_test.go
+++ b/internal/kargo/kargo_test.go
@@ -44,26 +44,6 @@ func TestNewPromotion(t *testing.T) {
 				require.Equal(t, testFreight[0:7], parts[2])
 			},
 		},
-		{
-			name: "Promote stage with shard",
-			stage: kargoapi.Stage{
-				ObjectMeta: metav1.ObjectMeta{
-					UID:       "80b44831-ac8d-4900-9df9-ee95f80c0fae",
-					Name:      "test",
-					Namespace: "kargo-demo",
-					Labels: map[string]string{
-						kargoapi.ShardLabelKey: "another-shard",
-					},
-				},
-			},
-			freight: testFreight,
-			assertions: func(t *testing.T, _ kargoapi.Stage, promo kargoapi.Promotion) {
-				parts := strings.Split(promo.Name, ".")
-				require.Equal(t, "test", parts[0])
-				require.Equal(t, testFreight[0:7], parts[2])
-				require.Equal(t, "another-shard", promo.Labels[kargoapi.ShardLabelKey])
-			},
-		},
 		{
 			name: "Promote stage with very long name",
 			stage: kargoapi.Stage{
@@ -86,7 +66,6 @@ func TestNewPromotion(t *testing.T) {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			promo := NewPromotion(tc.stage, tc.freight)
-			require.True(t, metav1.IsControlledBy(&promo, &tc.stage))
 			require.Equal(t, tc.freight, promo.Spec.Freight)
 			require.Equal(t, tc.stage.Name, promo.Spec.Stage)
 			require.Equal(t, tc.freight, promo.Spec.Freight)
diff --git a/internal/webhook/promotion/webhook_test.go b/internal/webhook/promotion/webhook_test.go
index b4d31a99..e5118567 100644
--- a/internal/webhook/promotion/webhook_test.go
+++ b/internal/webhook/promotion/webhook_test.go
@@ -74,11 +74,16 @@ func TestDefault(t *testing.T) {
 					client.Client,
 					types.NamespacedName,
 				) (*kargoapi.Stage, error) {
-					return &kargoapi.Stage{}, nil
+					return &kargoapi.Stage{
+						Spec: &kargoapi.StageSpec{
+							Shard: "fake-shard",
+						},
+					}, nil
 				},
 			},
 			assertions: func(t *testing.T, promo *kargoapi.Promotion, err error) {
 				require.NoError(t, err)
+				require.Equal(t, "fake-shard", promo.Labels[kargoapi.ShardLabelKey])
 				require.NotEmpty(t, promo.OwnerReferences)
 			},
 		},