refactor: improve inflation of task step vars

Signed-off-by: Hidde Beydals <[email protected]>

internal/directives/promotions.go

@@ -126,6 +126,30 @@ func StepEnvWithSecrets(secrets map[string]map[string]string) PromotionStepEnvOp
 	}
 }
 
+// StepEnvWithOutputs returns a PromotionStepEnvOption that adds the provided outputs
+// to the environment of the PromotionStep.
+func StepEnvWithOutputs(outputs State) PromotionStepEnvOption {
+	return func(env map[string]any) {
+		env["outputs"] = outputs
+	}
+}
+
+// StepEnvWithTaskOutputs returns a PromotionStepEnvOption that adds the provided
+// task outputs to the environment of the PromotionStep.
+func StepEnvWithTaskOutputs(alias string, outputs State) PromotionStepEnvOption {
+	return func(env map[string]any) {
+		// Ensure that if the PromotionStep originated from a task, the task outputs
+		// are available to the PromotionStep. This allows inflated steps to access
+		// the outputs of the other steps in the task without needing to know the
+		// alias (namespace) of the task.
+		if taskOutput := getTaskOutputs(alias, outputs); taskOutput != nil {
+			env["task"] = map[string]any{
+				"outputs": taskOutput,
+			}
+		}
+	}
+}
+
 // GetTimeout returns the maximum interval the provided runner may spend
 // attempting to execute the step before retries are abandoned and the entire
 // Promotion is marked as failed. If the runner is a RetryableStepRunner, its
@@ -152,14 +176,13 @@ func (s *PromotionStep) GetErrorThreshold(runner any) uint32 {
 }
 
 // BuildEnv returns the environment for the PromotionStep. The environment
-// includes the context of the Promotion, the outputs of the previous steps,
-// and any additional options provided.
+// includes the context of the Promotion and any additional options provided
+// (e.g. outputs, task outputs, vars, secrets).
 //
 // The environment is a (nested) map of string keys to any values. The keys
 // are used as variables in the PromotionStep configuration.
 func (s *PromotionStep) BuildEnv(
 	promoCtx PromotionContext,
-	state State,
 	opts ...PromotionStepEnvOption,
 ) map[string]any {
 	env := map[string]any{
@@ -168,17 +191,6 @@ func (s *PromotionStep) BuildEnv(
 			"promotion": promoCtx.Promotion,
 			"stage":     promoCtx.Stage,
 		},
-		"outputs": state,
-	}
-
-	// Ensure that if the PromotionStep originated from a task, the task outputs
-	// are available to the PromotionStep. This allows inflated steps to access
-	// the outputs of the other steps in the task without needing to know the
-	// alias (namespace) of the task.
-	if taskOutput := s.getTaskOutputs(state); taskOutput != nil {
-		env["task"] = map[string]any{
-			"outputs": taskOutput,
-		}
 	}
 
 	// Apply all provided options
@@ -207,7 +219,13 @@ func (s *PromotionStep) GetConfig(
 		return nil, err
 	}
 
-	env := s.BuildEnv(promoCtx, state, StepEnvWithVars(vars), StepEnvWithSecrets(promoCtx.Secrets))
+	env := s.BuildEnv(
+		promoCtx,
+		StepEnvWithOutputs(state),
+		StepEnvWithTaskOutputs(s.Alias, state),
+		StepEnvWithVars(vars),
+		StepEnvWithSecrets(promoCtx.Secrets),
+	)
 
 	evaledCfgJSON, err := expressions.EvaluateJSONTemplate(
 		s.Config,
@@ -250,41 +268,40 @@ func (s *PromotionStep) GetVars(
 	promoCtx PromotionContext,
 	state State,
 ) (map[string]any, error) {
-	var rawVars = make(map[string]string, len(promoCtx.Vars))
-	for _, v := range promoCtx.Vars {
-		rawVars[v.Name] = v.Value
-	}
-	for _, v := range s.Vars {
-		rawVars[v.Name] = v.Value
-	}
-
-	vars := make(map[string]any, len(rawVars))
-	for k, v := range rawVars {
-		env := s.BuildEnv(promoCtx, state, StepEnvWithVars(vars))
+	vars := make(map[string]any)
 
-		newVar, err := expressions.EvaluateTemplate(v, env)
+	// Evaluate the global variables defined in the Promotion itself, these
+	// variables DO NOT have access to the (task) outputs.
+	for _, v := range promoCtx.Vars {
+		newVar, err := expressions.EvaluateTemplate(
+			v.Value,
+			s.BuildEnv(promoCtx, StepEnvWithVars(vars)),
+		)
 		if err != nil {
-			return nil, fmt.Errorf("error pre-processing promotion variable %q: %w", k, err)
+			return nil, fmt.Errorf("error pre-processing promotion variable %q: %w", v.Name, err)
 		}
-		vars[k] = newVar
+		vars[v.Name] = newVar
 	}
-	return vars, nil
-}
 
-// getTaskOutputs returns the outputs of a task that are relevant to the current
-// step. This is useful when a step is inflated from a task and needs to access
-// the outputs of that task.
-func (s *PromotionStep) getTaskOutputs(state State) State {
-	if namespace := getAliasNamespace(s.Alias); namespace != "" {
-		taskOutputs := make(State)
-		for k, v := range state.DeepCopy() {
-			if getAliasNamespace(k) == namespace {
-				taskOutputs[k[len(namespace)+2:]] = v
-			}
+	// Evaluate the variables defined in the PromotionStep, these variables
+	// DO have access to the (task) outputs.
+	for _, v := range s.Vars {
+		newVar, err := expressions.EvaluateTemplate(
+			v.Value,
+			s.BuildEnv(
+				promoCtx,
+				StepEnvWithOutputs(state),
+				StepEnvWithTaskOutputs(s.Alias, state),
+				StepEnvWithVars(vars),
+			),
+		)
+		if err != nil {
+			return nil, fmt.Errorf("error pre-processing promotion variable %q: %w", v.Name, err)
 		}
-		return taskOutputs
+		vars[v.Name] = newVar
 	}
-	return nil
+
+	return vars, nil
 }
 
 // PromotionResult is the result of a user-defined promotion process executed by
@@ -492,6 +509,22 @@ func getChartFunc(
 	}
 }
 
+// getTaskOutputs returns the outputs of a task that are relevant to the current
+// step. This is useful when a step is inflated from a task and needs to access
+// the outputs of that task.
+func getTaskOutputs(alias string, state State) State {
+	if namespace := getAliasNamespace(alias); namespace != "" {
+		taskOutputs := make(State)
+		for k, v := range state.DeepCopy() {
+			if getAliasNamespace(k) == namespace {
+				taskOutputs[k[len(namespace)+2:]] = v
+			}
+		}
+		return taskOutputs
+	}
+	return nil
+}
+
 // getAliasNamespace returns the namespace part of an alias, if it exists.
 // The namespace part is the part before the first "::" separator. Typically,
 // this is used for steps inflated from a task.

internal/indexer/indexer.go

@@ -222,7 +222,8 @@ func RunningPromotionsByArgoCDApplications(
 							if nameTemplate, ok := app["name"].(string); ok {
 								env := dirStep.BuildEnv(
 									promoCtx,
-									promo.Status.GetState(),
+									directives.StepEnvWithOutputs(promoCtx.State),
+									directives.StepEnvWithTaskOutputs(dirStep.Alias, promoCtx.State),
 									directives.StepEnvWithVars(vars),
 								)
 

internal/kargo/promotion_builder.go

@@ -144,7 +144,7 @@ func (b *PromotionBuilder) inflateTaskSteps(
 
 		// With the variables validated and mapped, they are now available to
 		// the Config of the step during the Promotion execution.
-		step.Vars = vars
+		step.Vars = append(vars, step.Vars...)
 
 		// Append the inflated step to the list of steps.
 		steps = append(steps, *step)
@@ -221,39 +221,52 @@ func generatePromotionTaskStepAlias(taskAlias, stepAlias string) string {
 func promotionTaskVarsToStepVars(
 	taskVars, promoVars, stepVars []kargoapi.PromotionVariable,
 ) ([]kargoapi.PromotionVariable, error) {
-	if len(taskVars) == 0 {
-		return nil, nil
-	}
-
-	promoVarsMap := make(map[string]kargoapi.PromotionVariable, len(promoVars))
+	// Promotion variables can be used to set (or override) the variables
+	// required by the PromotionTask, but they are not inflated into the
+	// variables for the step. This map is used to check if a variable is
+	// set on the Promotion, to avoid overriding it with the default value
+	// and to validate that the variable is set.
+	promoVarsMap := make(map[string]struct{}, len(promoVars))
 	for _, v := range promoVars {
-		promoVarsMap[v.Name] = v
+		if v.Value != "" {
+			promoVarsMap[v.Name] = struct{}{}
+		}
 	}
 
-	stepVarsMap := make(map[string]kargoapi.PromotionVariable, len(stepVars))
+	// Step variables are inflated into the variables for the step. This map
+	// is used to ensure all variables required by the PromotionTask without
+	// a default value are set.
+	stepVarsMap := make(map[string]struct{}, len(stepVars))
 	for _, v := range stepVars {
-		stepVarsMap[v.Name] = v
+		if v.Value != "" {
+			stepVarsMap[v.Name] = struct{}{}
+		}
 	}
 
-	vars := make([]kargoapi.PromotionVariable, 0, len(taskVars))
+	var vars []kargoapi.PromotionVariable
+
+	// Set the PromotionTask variable default values, but only if the variable
+	// is not set on the Promotion.
 	for _, v := range taskVars {
-		if stepVar, ok := stepVarsMap[v.Name]; ok && stepVar.Value != "" {
-			vars = append(vars, stepVar)
+		// Variable is set on the Promotion, we do not need to set the default.
+		if _, ok := promoVarsMap[v.Name]; ok {
 			continue
 		}
 
-		if promoVar, ok := promoVarsMap[v.Name]; ok && promoVar.Value != "" {
-			// If the variable is defined in the Promotion, the engine will
-			// automatically use the value from the Promotion, and we do not
-			// have to explicitly set it here.
+		// Set the variable if it has a default value.
+		if v.Value != "" {
+			vars = append(vars, v)
 			continue
 		}
 
-		if v.Value == "" {
+		// If not, the variable must be set in the step variables.
+		if _, ok := stepVarsMap[v.Name]; !ok {
 			return nil, fmt.Errorf("missing value for variable %q", v.Name)
 		}
-
-		vars = append(vars, v)
 	}
+
+	// Set the step variables.
+	vars = append(vars, stepVars...)
+
 	return vars, nil
 }

internal/kargo/promotion_builder_test.go

@@ -486,6 +486,9 @@ func TestPromotionBuilder_inflateTaskSteps(t *testing.T) {
 							{
 								As:   "step2",
 								Uses: "other-fake-step",
+								Vars: []kargoapi.PromotionVariable{
+									{Name: "input4", Value: "value4"},
+								},
 							},
 						},
 					},
@@ -498,15 +501,18 @@ func TestPromotionBuilder_inflateTaskSteps(t *testing.T) {
 				assert.Equal(t, "task-1::step1", steps[0].As)
 				assert.Equal(t, "fake-step", steps[0].Uses)
 				assert.ElementsMatch(t, []kargoapi.PromotionVariable{
+					{Name: "input2", Value: "default2"},
 					{Name: "input1", Value: "value1"},
 					{Name: "input2", Value: "value2"},
 				}, steps[0].Vars)
 
 				assert.Equal(t, "task-1::step2", steps[1].As)
 				assert.Equal(t, "other-fake-step", steps[1].Uses)
 				assert.ElementsMatch(t, []kargoapi.PromotionVariable{
+					{Name: "input2", Value: "default2"},
 					{Name: "input1", Value: "value1"},
 					{Name: "input2", Value: "value2"},
+					{Name: "input4", Value: "value4"},
 				}, steps[1].Vars)
 			},
 		},
@@ -981,7 +987,7 @@ func Test_promotionTaskVarsToStepVars(t *testing.T) {
 			},
 		},
 		{
-			name: "step value overrides default value",
+			name: "step value appends task default value",
 			taskVars: []kargoapi.PromotionVariable{
 				{Name: "input1", Value: "default1"},
 			},
@@ -991,6 +997,7 @@ func Test_promotionTaskVarsToStepVars(t *testing.T) {
 			assertions: func(t *testing.T, result []kargoapi.PromotionVariable, err error) {
 				require.NoError(t, err)
 				assert.ElementsMatch(t, []kargoapi.PromotionVariable{
+					{Name: "input1", Value: "default1"},
 					{Name: "input1", Value: "override1"},
 				}, result)
 			},
@@ -1024,8 +1031,9 @@ func Test_promotionTaskVarsToStepVars(t *testing.T) {
 			assertions: func(t *testing.T, result []kargoapi.PromotionVariable, err error) {
 				require.NoError(t, err)
 				assert.ElementsMatch(t, []kargoapi.PromotionVariable{
-					{Name: "input1", Value: "override1"},
+					{Name: "input1", Value: "default1"},
 					{Name: "input2", Value: "default2"},
+					{Name: "input1", Value: "override1"},
 					{Name: "input3", Value: "value3"},
 				}, result)
 			},