Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String interpolation on validation errors when feature flag is not enabled? #7493

Closed
chmouel opened this issue Dec 15, 2023 · 2 comments
Closed
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@chmouel
Copy link
Member

chmouel commented Dec 15, 2023

Expected Behavior

Actual Behavior

showing this when applying a stepactions and feature flag is not enabled

+ kubectl create -f task.yaml
Error from server (BadRequest): error when creating "task.yaml": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: feature flag %s should be set to true to reference StepActions in Steps.: spec.steps[0].enable-step-actions

the %s interpolation is not done

Steps to Reproduce the Problem

  1. create a stepaction
  2. don't enable the feature-flag for step action

Additional Info

  • Kubernetes version:

    Output of kubectl version:

(paste your output here)
  • Tekton Pipeline version:
Client version: dev
Pipeline version: v0.54.0
Dashboard version: v0.42.0

Notes

I tried to fix this with this patch:

diff --git a/pkg/apis/pipeline/v1/result_validation.go b/pkg/apis/pipeline/v1/result_validation.go
index 27cc4016f..233c48b50 100644
--- a/pkg/apis/pipeline/v1/result_validation.go
+++ b/pkg/apis/pipeline/v1/result_validation.go
@@ -74,7 +74,7 @@ func (tr TaskResult) validateValue(ctx context.Context) (errs *apis.FieldError)
 		return nil
 	}
 	if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableStepActions {
-		return apis.ErrGeneric("feature flag %s should be set to true to fetch Results from Steps using StepActions.", config.EnableStepActions)
+		return apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to fetch Results from Steps using StepActions.", config.EnableStepActions))
 	}
 	if tr.Value.Type != ParamTypeString {
 		return &apis.FieldError{
diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go
index c98819cf5..1bc5d122b 100644
--- a/pkg/apis/pipeline/v1/task_validation.go
+++ b/pkg/apis/pipeline/v1/task_validation.go
@@ -325,7 +325,7 @@ func isCreateOrUpdateAndDiverged(ctx context.Context, s Step) bool {
 func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.FieldError) {
 	if s.Ref != nil {
 		if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableStepActions && isCreateOrUpdateAndDiverged(ctx, s) {
-			return apis.ErrGeneric("feature flag %s should be set to true to reference StepActions in Steps.", config.EnableStepActions)
+			return apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true to reference StepActions in Steps.", config.EnableStepActions))
 		}
 		errs = errs.Also(s.Ref.Validate(ctx))
 		if s.Image != "" {
diff --git a/pkg/apis/pipeline/v1beta1/result_validation.go b/pkg/apis/pipeline/v1beta1/result_validation.go
index a9f776b52..deadbd534 100644

which now output the error properly:

+ kubectl create -f task.yaml
Error from server (BadRequest): error when creating "task.yaml": admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: feature flag enable-step-actions should be set to true to reference StepActions in Steps.: 

but i am not quite sure if that's the right way, apis.ErrGeneric should do that interpolation already? or ther is more to fix than this across all errors returns?

@chmouel chmouel added the kind/bug Categorizes issue or PR as related to a bug. label Dec 15, 2023
@Yongxuanzhang
Copy link
Member

Thanks! I think your fix is correct, could you raise a pr to fix?

@chmouel
Copy link
Member Author

chmouel commented Dec 20, 2023

/assign

chmouel added a commit to chmouel/tektoncd-pipeline that referenced this issue Dec 20, 2023
When step action is used in step, and feature flag is not enabled, the
error message is not shown correctly and would show a %s.

We are now using fmt.Sprintf to format the error message and show the
feature flag that need to be enabled properly.

Fixes bug tektoncd#7493
chmouel added a commit to chmouel/tektoncd-pipeline that referenced this issue Dec 20, 2023
When step action is used in step, and feature flag is not enabled, the
error message is not shown correctly and would show a %s.

We are now using fmt.Sprintf to format the error message and show the
feature flag that need to be enabled properly. apis.ErrGeneric need a
empty string as second argument to be able to show the path steps so
adding this.

Fixes bug tektoncd#7493

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
chmouel added a commit to chmouel/tektoncd-pipeline that referenced this issue Dec 20, 2023
When step action is used in step, and feature flag is not enabled, the
error message is not shown correctly and would show a %s.

We are now using fmt.Sprintf to format the error message and show the
feature flag that need to be enabled properly. apis.ErrGeneric need a
empty string as second argument to be able to show the path steps so
adding this.

Fixes bug tektoncd#7493

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
tekton-robot pushed a commit that referenced this issue Dec 20, 2023
When step action is used in step, and feature flag is not enabled, the
error message is not shown correctly and would show a %s.

We are now using fmt.Sprintf to format the error message and show the
feature flag that need to be enabled properly. apis.ErrGeneric need a
empty string as second argument to be able to show the path steps so
adding this.

Fixes bug #7493

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
@chmouel chmouel closed this as completed Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

2 participants