Clarify error message for PipelineRun alpha fields

This commit updates the error message when a user has specified PipelineRun.spec.taskRunSpecs.stepOverrides without the alpha feature flag enabled to clarify what they need to do to enable the field (and likewise for sidecarOverrides). A similar error message is already used for TaskRun's spec.stepOverrides. It also updates the error message for when a TaskRun's computeResources or PipelineRun.TaskRunSpecs.computeResources are used without the alpha feature flag.
tektoncd · Jun 28, 2022 · 006d2b2 · 006d2b2
1 parent 8611c55
commit 006d2b2
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 26 deletions.
diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation.go
@@ -22,7 +22,6 @@ import (
 	"time"
 
 	"github.com/tektoncd/pipeline/pkg/apis/config"
-	apisconfig "github.com/tektoncd/pipeline/pkg/apis/config"
 	"github.com/tektoncd/pipeline/pkg/apis/validate"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"knative.dev/pkg/apis"
@@ -72,8 +71,6 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) (errs *apis.FieldError)
 		}
 	}
 
-	// This is an alpha feature and will fail validation if it's used in a pipelinerun spec
-	// when the enable-api-fields feature gate is anything but "alpha".
 	if ps.Timeouts != nil {
 		if ps.Timeout != nil {
 			// can't have both at the same time
@@ -153,7 +150,7 @@ func (ps *PipelineRunSpec) validatePipelineTimeout(timeout time.Duration, errorM
 		if ps.Timeouts.Tasks.Duration > timeout {
 			tasksTimeoutErr = true
 		}
-		if ps.Timeouts.Tasks.Duration == apisconfig.NoTimeoutDuration && timeout != apisconfig.NoTimeoutDuration {
+		if ps.Timeouts.Tasks.Duration == config.NoTimeoutDuration && timeout != config.NoTimeoutDuration {
 			tasksTimeoutErr = true
 			tasksTimeoutStr += " (no timeout)"
 		}
@@ -168,7 +165,7 @@ func (ps *PipelineRunSpec) validatePipelineTimeout(timeout time.Duration, errorM
 		if ps.Timeouts.Finally.Duration > timeout {
 			finallyTimeoutErr = true
 		}
-		if ps.Timeouts.Finally.Duration == apisconfig.NoTimeoutDuration && timeout != apisconfig.NoTimeoutDuration {
+		if ps.Timeouts.Finally.Duration == config.NoTimeoutDuration && timeout != config.NoTimeoutDuration {
 			finallyTimeoutErr = true
 			finallyTimeoutStr += " (no timeout)"
 		}
@@ -187,24 +184,17 @@ func (ps *PipelineRunSpec) validatePipelineTimeout(timeout time.Duration, errorM
 }
 
 func validateTaskRunSpec(ctx context.Context, trs PipelineTaskRunSpec) (errs *apis.FieldError) {
-	cfg := config.FromContextOrDefaults(ctx)
-	if cfg.FeatureFlags.EnableAPIFields == config.AlphaAPIFields {
-		if trs.StepOverrides != nil {
-			errs = errs.Also(validateStepOverrides(trs.StepOverrides).ViaField("stepOverrides"))
-		}
-		if trs.SidecarOverrides != nil {
-			errs = errs.Also(validateSidecarOverrides(trs.SidecarOverrides).ViaField("sidecarOverrides"))
-		}
-		if trs.ComputeResources != nil {
-			errs = errs.Also(validateTaskRunComputeResources(trs.ComputeResources, trs.StepOverrides))
-		}
-	} else {
-		if trs.StepOverrides != nil {
-			errs = errs.Also(apis.ErrDisallowedFields("stepOverrides"))
-		}
-		if trs.SidecarOverrides != nil {
-			errs = errs.Also(apis.ErrDisallowedFields("sidecarOverrides"))
-		}
+	if trs.StepOverrides != nil {
+		errs = errs.Also(ValidateEnabledAPIFields(ctx, "stepOverrides", config.AlphaAPIFields).ViaField("stepOverrides"))
+		errs = errs.Also(validateStepOverrides(trs.StepOverrides).ViaField("stepOverrides"))
+	}
+	if trs.SidecarOverrides != nil {
+		errs = errs.Also(ValidateEnabledAPIFields(ctx, "sidecarOverrides", config.AlphaAPIFields).ViaField("sidecarOverrides"))
+		errs = errs.Also(validateSidecarOverrides(trs.SidecarOverrides).ViaField("sidecarOverrides"))
+	}
+	if trs.ComputeResources != nil {
+		errs = errs.Also(ValidateEnabledAPIFields(ctx, "computeResources", config.AlphaAPIFields).ViaField("computeResources"))
+		errs = errs.Also(validateTaskRunComputeResources(trs.ComputeResources, trs.StepOverrides))
 	}
 	return errs
 }
diff --git a/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelinerun_validation_test.go
@@ -543,7 +543,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) {
 				},
 			},
 		},
-		wantErr: apis.ErrDisallowedFields("stepOverrides").ViaIndex(0).ViaField("taskRunSpecs"),
+		wantErr: apis.ErrGeneric("stepOverrides requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"").ViaIndex(0).ViaField("taskRunSpecs"),
 	}, {
 		name: "sidecarOverride disallowed without alpha feature gate",
 		spec: v1beta1.PipelineRunSpec{
@@ -560,7 +560,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) {
 				},
 			},
 		},
-		wantErr: apis.ErrDisallowedFields("sidecarOverrides").ViaIndex(0).ViaField("taskRunSpecs"),
+		wantErr: apis.ErrGeneric("sidecarOverrides requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"").ViaIndex(0).ViaField("taskRunSpecs"),
 	}, {
 		name: "missing stepOverride name",
 		spec: v1beta1.PipelineRunSpec{
@@ -634,6 +634,20 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) {
 			"taskRunSpecs[0].computeResources",
 		),
 		withContext: enableAlphaAPIFields,
+	}, {
+		name: "computeResources disallowed without alpha feature gate",
+		spec: v1beta1.PipelineRunSpec{
+			PipelineRef: &v1beta1.PipelineRef{Name: "foo"},
+			TaskRunSpecs: []v1beta1.PipelineTaskRunSpec{
+				{
+					PipelineTaskName: "bar",
+					ComputeResources: &corev1.ResourceRequirements{
+						Requests: corev1.ResourceList{corev1.ResourceMemory: corev1resources.MustParse("2Gi")},
+					},
+				},
+			},
+		},
+		wantErr: apis.ErrGeneric("computeResources requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\"").ViaIndex(0).ViaField("taskRunSpecs"),
 	}}
 
 	for _, ps := range tests {

diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation.go b/pkg/apis/pipeline/v1beta1/taskrun_validation.go
@@ -67,12 +67,15 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) (errs *apis.FieldError) {
 	if ts.StepOverrides != nil {
 		errs = errs.Also(ValidateEnabledAPIFields(ctx, "stepOverrides", config.AlphaAPIFields).ViaField("stepOverrides"))
 		errs = errs.Also(validateStepOverrides(ts.StepOverrides).ViaField("stepOverrides"))
-		errs = errs.Also(validateTaskRunComputeResources(ts.ComputeResources, ts.StepOverrides))
 	}
 	if ts.SidecarOverrides != nil {
 		errs = errs.Also(ValidateEnabledAPIFields(ctx, "sidecarOverrides", config.AlphaAPIFields).ViaField("sidecarOverrides"))
 		errs = errs.Also(validateSidecarOverrides(ts.SidecarOverrides).ViaField("sidecarOverrides"))
 	}
+	if ts.ComputeResources != nil {
+		errs = errs.Also(ValidateEnabledAPIFields(ctx, "computeResources", config.AlphaAPIFields).ViaField("computeResources"))
+		errs = errs.Also(validateTaskRunComputeResources(ts.ComputeResources, ts.StepOverrides))
+	}
 
 	if ts.Status != "" {
 		if ts.Status != TaskRunSpecStatusCancelled {

diff --git a/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go b/pkg/apis/pipeline/v1beta1/taskrun_validation_test.go
@@ -538,6 +538,19 @@ func TestTaskRunSpec_Invalidate(t *testing.T) {
 			"computeResources",
 		),
 		wc: enableAlphaAPIFields,
+	}, {
+		name: "computeResources disallowed without alpha feature gate",
+		spec: v1beta1.TaskRunSpec{
+			TaskRef: &v1beta1.TaskRef{
+				Name: "foo",
+			},
+			ComputeResources: &corev1.ResourceRequirements{
+				Requests: corev1.ResourceList{
+					corev1.ResourceMemory: corev1resources.MustParse("2Gi"),
+				},
+			},
+		},
+		wantErr: apis.ErrGeneric("computeResources requires \"enable-api-fields\" feature gate to be \"alpha\" but it is \"stable\""),
 	}}
 
 	for _, ts := range tests {