better step result referencing and docs for step param substitution order and

docs/stepactions.md

@@ -136,6 +136,21 @@ spec:
 
 **Note:** If a `Step` declares `params` for an `inlined Step`, it will also lead to a validation error. This is because an `inlined Step` gets its `params` from the `TaskRun`.
 
+#### Parameter Substitution Precedence
+
+When applying parameters to a StepAction, the substitutions are applied in the following order:
+
+1. TaskRun parameter values in step parameters
+2. Parameters from StepAction defaults
+3. Parameters from the step (overwriting any defaults)
+4. Step result replacements
+
+This order ensures that:
+- Step parameters can reference TaskRun parameters
+- StepAction defaults provide fallback values
+- Step-specific parameters take precedence over defaults
+- Step result references are resolved last to allow referencing results from previous steps
+
 ### Emitting Results
 
 A `StepAction` also declares the results that it will emit.

pkg/reconciler/taskrun/resources/apply.go

@@ -66,16 +66,22 @@ var (
 	}
 )
 
-// applyStepActionParameters applies the params from the Task and the underlying Step to the referenced StepAction.
+// applyStepActionParameters applies the params from the task and the underlying step to the referenced stepaction.
+// substitution order:
+// 1. taskrun parameter values in step parameters
+// 2. set params from StepAction defaults
+// 3. set and overwrite params with the ones from the step
+// 4. set step result replacements last
 func applyStepActionParameters(step *v1.Step, spec *v1.TaskSpec, tr *v1.TaskRun, stepParams v1.Params, defaults []v1.ParamSpec) (*v1.Step, error) {
+	// 1. taskrun parameter substitutions to step parameters
 	if stepParams != nil {
 		stringR, arrayR, objectR := getTaskParameters(spec, tr, spec.Params...)
 		stepParams = stepParams.ReplaceVariables(stringR, arrayR, objectR)
 	}
-	// Set params from StepAction defaults
+	// 2. set params from stepaction defaults
 	stringReplacements, arrayReplacements, _ := replacementsFromDefaultParams(defaults)
 
-	// Set and overwrite params with the ones from the Step
+	// 3. set and overwrite params with the ones from the step
 	stepStrings, stepArrays, _ := replacementsFromParams(stepParams)
 	for k, v := range stepStrings {
 		stringReplacements[k] = v
@@ -84,16 +90,19 @@ func applyStepActionParameters(step *v1.Step, spec *v1.TaskSpec, tr *v1.TaskRun,
 		arrayReplacements[k] = v
 	}
 
-	stepResultReplacements, _ := replacementsFromStepResults(step, stepParams, defaults)
-	for k, v := range stepResultReplacements {
-		stringReplacements[k] = v
+	// 4. set step result replacements last
+	if stepResultReplacements, err := replacementsFromStepResults(step, stepParams, defaults); err != nil {
+		return nil, err
+	} else {
+		for k, v := range stepResultReplacements {
+			stringReplacements[k] = v
+		}
 	}
 
-	// Check if there are duplicate keys in the replacements
-	// If the same key is present in both stringReplacements and arrayReplacements, it means
+	// check if there are duplicate keys in the replacements
+	// if the same key is present in both stringReplacements and arrayReplacements, it means
 	// that the default value and the passed value have different types.
-	err := checkForDuplicateKeys(stringReplacements, arrayReplacements)
-	if err != nil {
+	if err := checkForDuplicateKeys(stringReplacements, arrayReplacements); err != nil {
 		return nil, err
 	}
 
@@ -165,8 +174,9 @@ func replacementsArrayIdxStepResults(step *v1.Step, paramName string, stepName s
 func replacementsFromStepResults(step *v1.Step, stepParams v1.Params, defaults []v1.ParamSpec) (map[string]string, error) {
 	stringReplacements := map[string]string{}
 	for _, sp := range stepParams {
-		if sp.Value.StringVal != "" {
-			//  $(params.p1) --> $(steps.step1.results.foo) (normal substitution)
+		if sp.Value.StringVal != "" && strings.HasPrefix(sp.Value.StringVal, "$(steps.") {
+			// eg: when parameter p1 references a step result, replace:
+			// $(params.p1) with $(steps.step1.results.foo)
 			value := strings.TrimSuffix(strings.TrimPrefix(sp.Value.StringVal, "$("), ")")
 			pr, err := resultref.ParseStepExpression(value)
 			if err != nil {
@@ -180,19 +190,26 @@ func replacementsFromStepResults(step *v1.Step, stepParams v1.Params, defaults [
 							stringReplacements[fmt.Sprintf("params.%s.%s", d.Name, k)] = fmt.Sprintf("$(steps.%s.results.%s.%s)", pr.ResourceName, pr.ResultName, k)
 						}
 					case v1.ParamTypeArray:
-						//  $(params.p1[*]) --> $(steps.step1.results.foo)
+						// with star notation, replace:
+						// $(params.p1[*]) with $(steps.step1.results.foo[*])
 						for _, pattern := range paramPatterns {
 							stringReplacements[fmt.Sprintf(pattern+"[*]", d.Name)] = fmt.Sprintf("$(steps.%s.results.%s[*])", pr.ResourceName, pr.ResultName)
 						}
-						//  $(params.p1[idx]) --> $(steps.step1.results.foo[idx])
+						// with index notation, replace:
+						// $(params.p1[idx]) with $(steps.step1.results.foo[idx])
 						for k, v := range replacementsArrayIdxStepResults(step, d.Name, pr.ResourceName, pr.ResultName) {
 							stringReplacements[k] = v
 						}
-					// This is handled by normal param substitution.
-					// $(params.p1.key) --> $(steps.step1.results.foo)
 					case v1.ParamTypeString:
-					// Since String is the default, This is handled by normal param substitution.
+						fallthrough
 					default:
+						// for string parameters and default case,
+						// replace any reference to the parameter with the step result reference
+						// since both use simple value substitution
+						// eg: replace $(params.p1) with $(steps.step1.results.foo)
+						for _, pattern := range paramPatterns {
+							stringReplacements[fmt.Sprintf(pattern, d.Name)] = sp.Value.StringVal
+						}
 					}
 				}
 			}

pkg/reconciler/taskrun/resources/taskspec_test.go

@@ -1349,6 +1349,127 @@ func TestGetStepActionsData(t *testing.T) {
 			Args:   []string{"Hello, I am of type list"},
 			Script: "echo $@",
 		}},
+	}, {
+		name: "step result reference in parameter",
+		tr: &v1.TaskRun{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "mytaskrun",
+				Namespace: "default",
+			},
+			Spec: v1.TaskRunSpec{
+				TaskSpec: &v1.TaskSpec{
+					Steps: []v1.Step{{
+						Ref: &v1.Ref{
+							Name: "stepAction",
+						},
+						Params: v1.Params{{
+							Name:  "message",
+							Value: *v1.NewStructuredValues("$(steps.step1.results.output)"),
+						}},
+					}},
+				},
+			},
+		},
+		stepAction: &v1beta1.StepAction{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "stepAction",
+				Namespace: "default",
+			},
+			Spec: v1beta1.StepActionSpec{
+				Image: "myimage",
+				Args:  []string{"$(params.message)"},
+				Params: v1.ParamSpecs{{
+					Name: "message",
+					Type: v1.ParamTypeString,
+				}},
+			},
+		},
+		want: []v1.Step{{
+			Image: "myimage",
+			Args:  []string{"$(steps.step1.results.output)"},
+		}},
+	}, {
+		name: "step result reference with array type",
+		tr: &v1.TaskRun{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "mytaskrun",
+				Namespace: "default",
+			},
+			Spec: v1.TaskRunSpec{
+				TaskSpec: &v1.TaskSpec{
+					Steps: []v1.Step{{
+						Ref: &v1.Ref{
+							Name: "stepAction",
+						},
+						Params: v1.Params{{
+							Name:  "items",
+							Value: *v1.NewStructuredValues("$(steps.step1.results.items)"),
+						}},
+					}},
+				},
+			},
+		},
+		stepAction: &v1beta1.StepAction{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "stepAction",
+				Namespace: "default",
+			},
+			Spec: v1beta1.StepActionSpec{
+				Image: "myimage",
+				Args:  []string{"$(params.items[*])", "$(params.items[0])"},
+				Params: v1.ParamSpecs{{
+					Name: "items",
+					Type: v1.ParamTypeArray,
+				}},
+			},
+		},
+		want: []v1.Step{{
+			Image: "myimage",
+			Args:  []string{"$(steps.step1.results.items[*])", "$(steps.step1.results.items[0])"},
+		}},
+	}, {
+		name: "step result reference with object type",
+		tr: &v1.TaskRun{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "mytaskrun",
+				Namespace: "default",
+			},
+			Spec: v1.TaskRunSpec{
+				TaskSpec: &v1.TaskSpec{
+					Steps: []v1.Step{{
+						Ref: &v1.Ref{
+							Name: "stepAction",
+						},
+						Params: v1.Params{{
+							Name:  "config",
+							Value: *v1.NewStructuredValues("$(steps.step1.results.config)"),
+						}},
+					}},
+				},
+			},
+		},
+		stepAction: &v1beta1.StepAction{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "stepAction",
+				Namespace: "default",
+			},
+			Spec: v1beta1.StepActionSpec{
+				Image: "myimage",
+				Args:  []string{"$(params.config.key1)", "$(params.config.key2)"},
+				Params: v1.ParamSpecs{{
+					Name: "config",
+					Type: v1.ParamTypeObject,
+					Properties: map[string]v1.PropertySpec{
+						"key1": {Type: v1.ParamTypeString},
+						"key2": {Type: v1.ParamTypeString},
+					},
+				}},
+			},
+		},
+		want: []v1.Step{{
+			Image: "myimage",
+			Args:  []string{"$(steps.step1.results.config.key1)", "$(steps.step1.results.config.key2)"},
+		}},
 	}}
 	for _, tt := range tests {
 		ctx := context.Background()
@@ -1504,3 +1625,50 @@ func TestGetStepActionsData_Error(t *testing.T) {
 		}
 	}
 }
+
+func TestGetStepActionsData_InvalidStepResultReference(t *testing.T) {
+	tr := &v1.TaskRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "mytaskrun",
+			Namespace: "default",
+		},
+		Spec: v1.TaskRunSpec{
+			TaskSpec: &v1.TaskSpec{
+				Steps: []v1.Step{{
+					Name: "step1",
+					Ref: &v1.Ref{
+						Name: "stepAction",
+					},
+					Params: v1.Params{{
+						Name: "param1",
+						Value: v1.ParamValue{
+							Type:      v1.ParamTypeString,
+							StringVal: "$(steps.invalid.step)",
+						},
+					}},
+				}},
+			},
+		},
+	}
+
+	stepAction := &v1beta1.StepAction{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "stepAction",
+			Namespace: "default",
+		},
+		Spec: v1beta1.StepActionSpec{
+			Image: "myimage",
+			Params: v1.ParamSpecs{{
+				Name: "param1",
+				Type: v1.ParamTypeString,
+			}},
+		},
+	}
+
+	expectedError := `must be one of the form 1). "steps.<stepName>.results.<resultName>"; 2). "steps.<stepName>.results.<objectResultName>.<individualAttribute>"`
+	ctx := context.Background()
+	tektonclient := fake.NewSimpleClientset(stepAction)
+	if _, err := resources.GetStepActionsData(ctx, *tr.Spec.TaskSpec, tr, tektonclient, nil, nil); err.Error() != expectedError {
+		t.Errorf("Expected error message %s but got %s", expectedError, err.Error())
+	}
+}