From 36de579d9e6197186a9f86e77511ab512e2b173f Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Wed, 20 Dec 2023 10:26:15 +0100 Subject: [PATCH] Fix showing error message when validation fail 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 --- pkg/apis/pipeline/v1/result_validation.go | 2 +- pkg/apis/pipeline/v1/result_validation_test.go | 3 +-- pkg/apis/pipeline/v1/task_validation.go | 4 ++-- pkg/apis/pipeline/v1/task_validation_test.go | 16 ++++++++-------- pkg/apis/pipeline/v1beta1/result_validation.go | 4 ++-- .../pipeline/v1beta1/result_validation_test.go | 3 +-- pkg/apis/pipeline/v1beta1/task_validation.go | 4 ++-- .../pipeline/v1beta1/task_validation_test.go | 16 ++++++++-------- 8 files changed, 25 insertions(+), 27 deletions(-) diff --git a/pkg/apis/pipeline/v1/result_validation.go b/pkg/apis/pipeline/v1/result_validation.go index 27cc4016f32..233c48b5083 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/result_validation_test.go b/pkg/apis/pipeline/v1/result_validation_test.go index 4026e0b923c..10f4f3c1f08 100644 --- a/pkg/apis/pipeline/v1/result_validation_test.go +++ b/pkg/apis/pipeline/v1/result_validation_test.go @@ -175,8 +175,7 @@ func TestResultsValidateValueError(t *testing.T) { }, enableStepActions: false, expectedError: apis.FieldError{ - Message: "feature flag %s should be set to true to fetch Results from Steps using StepActions.", - Paths: []string{"enable-step-actions"}, + Message: "feature flag enable-step-actions should be set to true to fetch Results from Steps using StepActions.", }, }, { name: "invalid result value type array", diff --git a/pkg/apis/pipeline/v1/task_validation.go b/pkg/apis/pipeline/v1/task_validation.go index c98819cf592..38b316dc7a2 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 != "" { @@ -385,7 +385,7 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi } if len(s.Results) > 0 { if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableStepActions && isCreateOrUpdateAndDiverged(ctx, s) { - return apis.ErrGeneric("feature flag %s should be set to true in order to use Results in Steps.", config.EnableStepActions) + return apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true in order to use Results in Steps.", config.EnableStepActions), "") } } if s.Image == "" { diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index 60ce1bc12fd..0786a0171c3 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -1429,8 +1429,8 @@ func TestTaskSpecValidateErrorWithStepActionRef_CreateUpdateEvent(t *testing.T) isCreate: true, isUpdate: false, expectedError: apis.FieldError{ - Message: "feature flag %s should be set to true to reference StepActions in Steps.", - Paths: []string{"steps[0].enable-step-actions"}, + Message: "feature flag enable-step-actions should be set to true to reference StepActions in Steps.", + Paths: []string{"steps[0]"}, }, }, { name: "is update ctx", @@ -1442,8 +1442,8 @@ func TestTaskSpecValidateErrorWithStepActionRef_CreateUpdateEvent(t *testing.T) isCreate: false, isUpdate: true, expectedError: apis.FieldError{ - Message: "feature flag %s should be set to true to reference StepActions in Steps.", - Paths: []string{"steps[0].enable-step-actions"}, + Message: "feature flag enable-step-actions should be set to true to reference StepActions in Steps.", + Paths: []string{"steps[0]"}, }, }, { name: "ctx is not create or update", @@ -2642,8 +2642,8 @@ func TestTaskSpecValidate_StepResults_Error(t *testing.T) { enableStepActions: false, isCreate: true, expectedError: apis.FieldError{ - Message: "feature flag %s should be set to true in order to use Results in Steps.", - Paths: []string{"steps[0].enable-step-actions"}, + Message: "feature flag enable-step-actions should be set to true in order to use Results in Steps.", + Paths: []string{"steps[0]"}, }, }, { name: "step result not allowed without enable step actions - update and diverged event", @@ -2664,8 +2664,8 @@ func TestTaskSpecValidate_StepResults_Error(t *testing.T) { }, }, expectedError: apis.FieldError{ - Message: "feature flag %s should be set to true in order to use Results in Steps.", - Paths: []string{"steps[0].enable-step-actions"}, + Message: "feature flag enable-step-actions should be set to true in order to use Results in Steps.", + Paths: []string{"steps[0]"}, }, }, { name: "step result allowed without enable step actions - update but not diverged", diff --git a/pkg/apis/pipeline/v1beta1/result_validation.go b/pkg/apis/pipeline/v1beta1/result_validation.go index a9f776b5277..deadbd53497 100644 --- a/pkg/apis/pipeline/v1beta1/result_validation.go +++ b/pkg/apis/pipeline/v1beta1/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{ @@ -89,7 +89,7 @@ func (tr TaskResult) validateValue(ctx context.Context) (errs *apis.FieldError) stepName, resultName, err := v1.ExtractStepResultName(tr.Value.StringVal) if err != nil { return &apis.FieldError{ - Message: fmt.Sprintf("%v", err), + Message: err.Error(), Paths: []string{fmt.Sprintf("%s.value", tr.Name)}, } } diff --git a/pkg/apis/pipeline/v1beta1/result_validation_test.go b/pkg/apis/pipeline/v1beta1/result_validation_test.go index 18d01f640fb..b2917767286 100644 --- a/pkg/apis/pipeline/v1beta1/result_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/result_validation_test.go @@ -176,8 +176,7 @@ func TestResultsValidateValueError(t *testing.T) { }, enableStepActions: false, expectedError: apis.FieldError{ - Message: "feature flag %s should be set to true to fetch Results from Steps using StepActions.", - Paths: []string{"enable-step-actions"}, + Message: "feature flag enable-step-actions should be set to true to fetch Results from Steps using StepActions.", }, }, { name: "invalid result value type array", diff --git a/pkg/apis/pipeline/v1beta1/task_validation.go b/pkg/apis/pipeline/v1beta1/task_validation.go index 574812867bc..95ddbc1f6d4 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation.go +++ b/pkg/apis/pipeline/v1beta1/task_validation.go @@ -314,7 +314,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 != "" { @@ -374,7 +374,7 @@ func validateStep(ctx context.Context, s Step, names sets.String) (errs *apis.Fi } if len(s.Results) > 0 { if !config.FromContextOrDefaults(ctx).FeatureFlags.EnableStepActions && isCreateOrUpdateAndDiverged(ctx, s) { - return apis.ErrGeneric("feature flag %s should be set to true in order to use Results in Steps.", config.EnableStepActions) + return apis.ErrGeneric(fmt.Sprintf("feature flag %s should be set to true in order to use Results in Steps.", config.EnableStepActions), "") } } if s.Image == "" { diff --git a/pkg/apis/pipeline/v1beta1/task_validation_test.go b/pkg/apis/pipeline/v1beta1/task_validation_test.go index 117cccf0dbc..9e89ee83fbd 100644 --- a/pkg/apis/pipeline/v1beta1/task_validation_test.go +++ b/pkg/apis/pipeline/v1beta1/task_validation_test.go @@ -1442,8 +1442,8 @@ func TestTaskSpecValidateErrorWithStepActionRef_CreateUpdateEvent(t *testing.T) isCreate: true, isUpdate: false, expectedError: apis.FieldError{ - Message: "feature flag %s should be set to true to reference StepActions in Steps.", - Paths: []string{"steps[0].enable-step-actions"}, + Message: "feature flag enable-step-actions should be set to true to reference StepActions in Steps.", + Paths: []string{"steps[0]"}, }, }, { name: "is update ctx", @@ -1455,8 +1455,8 @@ func TestTaskSpecValidateErrorWithStepActionRef_CreateUpdateEvent(t *testing.T) isCreate: false, isUpdate: true, expectedError: apis.FieldError{ - Message: "feature flag %s should be set to true to reference StepActions in Steps.", - Paths: []string{"steps[0].enable-step-actions"}, + Message: "feature flag enable-step-actions should be set to true to reference StepActions in Steps.", + Paths: []string{"steps[0]"}, }, }, { name: "ctx is not create or update", @@ -2428,8 +2428,8 @@ func TestTaskSpecValidate_StepResults_Error(t *testing.T) { enableStepActions: false, isCreate: true, expectedError: apis.FieldError{ - Message: "feature flag %s should be set to true in order to use Results in Steps.", - Paths: []string{"steps[0].enable-step-actions"}, + Message: "feature flag enable-step-actions should be set to true in order to use Results in Steps.", + Paths: []string{"steps[0]"}, }, }, { name: "step result not allowed without enable step actions - update and diverged event", @@ -2450,8 +2450,8 @@ func TestTaskSpecValidate_StepResults_Error(t *testing.T) { }, }, expectedError: apis.FieldError{ - Message: "feature flag %s should be set to true in order to use Results in Steps.", - Paths: []string{"steps[0].enable-step-actions"}, + Message: "feature flag enable-step-actions should be set to true in order to use Results in Steps.", + Paths: []string{"steps[0]"}, }, }, { name: "step result allowed without enable step actions - update but not diverged",