Repository: akuity/kargo
Pull request #1632 (2024-03-15 21:11:44)

fix: fix freight created event handler
#1615 overlooked this particular use of the ownership ref on Freight to the Warehouse that created.

Since #1615, that information is in a field instead.

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


|  Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 5eeb9db71f825b3f800045582d16b62040b6a76a |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/65f4b994c96b250008277db5 |
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/akuity/kargo/pull/1632?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) Report
Attention: Patch coverage is `0%` with `2 lines` in your changes are missing coverage. Please review.
> Project coverage is 43.99%. 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 [(`5eeb9db`)](https://app.codecov.io/gh/akuity/kargo/pull/1632?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity).

| [Files](https://app.codecov.io/gh/akuity/kargo/pull/1632?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) | Patch % | Lines |
|---|---|---|
| [internal/controller/stages/watches.go](https://app.codecov.io/gh/akuity/kargo/pull/1632?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity#diff-aW50ZXJuYWwvY29udHJvbGxlci9zdGFnZXMvd2F0Y2hlcy5nbw==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/akuity/kargo/pull/1632?src=pr&el=tree&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    #1632      +/-   ##
==========================================
+ Coverage   43.96%   43.99%   +0.02%     
==========================================
  Files         199      199              
  Lines       12673    12665       -8     
==========================================
  Hits         5572     5572              
+ Misses       6864     6856       -8     
  Partials      237      237              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/akuity/kargo/pull/1632?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/controller/stages/watches.go b/internal/controller/stages/watches.go
index 997e4392..62b74780 100644
--- a/internal/controller/stages/watches.go
+++ b/internal/controller/stages/watches.go
@@ -228,16 +228,6 @@ func (c *createdFreightEventHandler) Create(
 ) {
 	logger := logging.LoggerFromContext(ctx)
 	freight := evt.Object.(*kargoapi.Freight) // nolint: forcetypeassert
-	// TODO: Get warehouse name freight.OwnerReferences
-	if len(freight.OwnerReferences) != 1 {
-		logger.Warnf(
-			"Expected Freight %q to have exactly 1 OwnerReference, got %d",
-			freight.Name,
-			len(freight.OwnerReferences),
-		)
-		return
-	}
-	warehouse := freight.OwnerReferences[0].Name
 	stages := kargoapi.StageList{}
 	if err := c.kargoClient.List(
 		ctx,
@@ -246,14 +236,14 @@ func (c *createdFreightEventHandler) Create(
 			Namespace: freight.Namespace,
 			FieldSelector: fields.OneTermEqualSelector(
 				kubeclient.StagesByWarehouseIndexField,
-				warehouse,
+				freight.Warehouse,
 			),
 			LabelSelector: c.shardSelector,
 		},
 	); err != nil {
 		logger.Errorf(
 			"Failed list Stages subscribed to Warehouse %q in namespace %q",
-			warehouse,
+			freight.Warehouse,
 			freight.Namespace,
 		)
 		return
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)
 			},
 		},