Repository: akuity/kargo
Pull request #1842 (2024-04-15 13:55:05)

feat: ensure Promotion's Stage has promo mechanism
Fixes: #1830 

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/akuity/kargo/pull/1842?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 44.90%. Comparing base [(`6902296`)](https://app.codecov.io/gh/akuity/kargo/commit/6902296b190fee4530b50b7375b618c802c18a8a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=akuity) to head [(`bcb15c2`)](https://app.codecov.io/gh/akuity/kargo/pull/1842?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    #1842      +/-   ##
==========================================
+ Coverage   44.88%   44.90%   +0.02%     
==========================================
  Files         213      213              
  Lines       13633    13639       +6     
==========================================
+ Hits         6119     6125       +6     
  Misses       7250     7250              
  Partials      264      264              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/akuity/kargo/pull/1842?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).

krancour: @hiddeco this looks good, but I think a necessary complement to this may be a change to the "promote to subscribers" endpoint so that we don't attempt creating Promos for control flow Stages that are destined to fail validation.
hiddeco: @krancour given that we attempt to promote to subscribers while collecting errors instead of failing the operation as a whole, isn't this actually a good way to make "invalid" subscribers surface?
krancour: @hiddeco consider the possibility that a Stage has many subscribers, some of which are normal stages and some of which are control flow Stages. I think "promote to subscribers" should succeed without any errors by only creating promotions for normal Stages.

Patch:
diff --git a/internal/api/promote_to_stage_subscribers_v1alpha1.go b/internal/api/promote_to_stage_subscribers_v1alpha1.go
index add4d122..3b84143d 100644
--- a/internal/api/promote_to_stage_subscribers_v1alpha1.go
+++ b/internal/api/promote_to_stage_subscribers_v1alpha1.go
@@ -134,6 +134,11 @@ func (s *server) PromoteToStageSubscribers(
 	createdPromos := make([]*kargoapi.Promotion, 0, len(subscribers))
 	for _, subscriber := range subscribers {
 		newPromo := kargo.NewPromotion(subscriber, freight.Name)
+		if subscriber.Spec.PromotionMechanisms == nil {
+			// Avoid creating a Promotion if the subscriber has no
+			// PromotionMechanisms, and is a "control flow" Stage.
+			continue
+		}
 		if err := s.createPromotionFn(ctx, &newPromo); err != nil {
 			promoteErrs = append(promoteErrs, err)
 			continue
diff --git a/internal/webhook/promotion/webhook.go b/internal/webhook/promotion/webhook.go
index 4801a2ea..62c25f93 100644
--- a/internal/webhook/promotion/webhook.go
+++ b/internal/webhook/promotion/webhook.go
@@ -137,6 +137,13 @@ func (w *webhook) Default(ctx context.Context, obj runtime.Object) error {
 			promo.Namespace,
 		)
 	}
+	if stage.Spec.PromotionMechanisms == nil {
+		return fmt.Errorf(
+			"Stage %q in namespace %q has no PromotionMechanisms",
+			promo.Spec.Stage,
+			promo.Namespace,
+		)
+	}
 
 	// Make sure the Promotion has the same shard as the Stage
 	if stage.Spec.Shard != "" {


Test patch:
diff --git a/internal/api/promote_to_stage_subscribers_v1alpha1_test.go b/internal/api/promote_to_stage_subscribers_v1alpha1_test.go
index a6c874f5..4a78c287 100644
--- a/internal/api/promote_to_stage_subscribers_v1alpha1_test.go
+++ b/internal/api/promote_to_stage_subscribers_v1alpha1_test.go
@@ -499,7 +499,13 @@ func TestPromoteToStageSubscribers(t *testing.T) {
 					context.Context,
 					*kargoapi.Stage,
 				) ([]kargoapi.Stage, error) {
-					return []kargoapi.Stage{{}}, nil
+					return []kargoapi.Stage{
+						{
+							Spec: &kargoapi.StageSpec{
+								PromotionMechanisms: &kargoapi.PromotionMechanisms{},
+							},
+						},
+					}, nil
 				},
 				authorizeFn: func(
 					context.Context,
@@ -573,7 +579,13 @@ func TestPromoteToStageSubscribers(t *testing.T) {
 					context.Context,
 					*kargoapi.Stage,
 				) ([]kargoapi.Stage, error) {
-					return []kargoapi.Stage{{}}, nil
+					return []kargoapi.Stage{
+						{
+							Spec: &kargoapi.StageSpec{
+								PromotionMechanisms: &kargoapi.PromotionMechanisms{},
+							},
+						},
+					}, nil
 				},
 				authorizeFn: func(
 					context.Context,
diff --git a/internal/webhook/promotion/webhook_test.go b/internal/webhook/promotion/webhook_test.go
index 8b02b316..02118c8e 100644
--- a/internal/webhook/promotion/webhook_test.go
+++ b/internal/webhook/promotion/webhook_test.go
@@ -77,6 +77,24 @@ func TestDefault(t *testing.T) {
 				require.Contains(t, err.Error(), "could not find Stage")
 			},
 		},
+		{
+			name: "stage without promotion mechanisms",
+			webhook: &webhook{
+				getStageFn: func(
+					context.Context,
+					client.Client,
+					types.NamespacedName,
+				) (*kargoapi.Stage, error) {
+					return &kargoapi.Stage{
+						Spec: &kargoapi.StageSpec{},
+					}, nil
+				},
+			},
+			assertions: func(t *testing.T, _ *kargoapi.Promotion, err error) {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), "has no PromotionMechanisms")
+			},
+		},
 		{
 			name: "success",
 			webhook: &webhook{
@@ -87,7 +105,8 @@ func TestDefault(t *testing.T) {
 				) (*kargoapi.Stage, error) {
 					return &kargoapi.Stage{
 						Spec: &kargoapi.StageSpec{
-							Shard: "fake-shard",
+							PromotionMechanisms: &kargoapi.PromotionMechanisms{},
+							Shard:               "fake-shard",
 						},
 					}, nil
 				},