From 4446f4e8b41d39a496d888243673314e910901df Mon Sep 17 00:00:00 2001 From: Jerop Date: Fri, 15 Sep 2023 15:09:01 -0400 Subject: [PATCH] Clean up getting substitution expressions Prior to this change, the functions for fetching substitution expressions were inconsistent in the codebase. This change updates the functions for fetching substitition expressions from Parameters and Pipeline Results such that they are member functions, in line with related functions. /kind cleanup --- pkg/apis/pipeline/v1/param_types.go | 23 ++++++++++++ pkg/apis/pipeline/v1/pipeline_types.go | 12 ++++++ pkg/apis/pipeline/v1/pipeline_validation.go | 8 ++-- .../pipeline/v1/pipelinerun_validation.go | 2 +- pkg/apis/pipeline/v1/resultref.go | 37 +------------------ pkg/apis/pipeline/v1/resultref_test.go | 8 ++-- pkg/reconciler/pipelinerun/resources/apply.go | 2 +- .../resources/pipelinerunresolution.go | 2 +- .../resources/validate_dependencies.go | 2 +- 9 files changed, 48 insertions(+), 48 deletions(-) diff --git a/pkg/apis/pipeline/v1/param_types.go b/pkg/apis/pipeline/v1/param_types.go index b47e45f72b3..86fe6ce3e72 100644 --- a/pkg/apis/pipeline/v1/param_types.go +++ b/pkg/apis/pipeline/v1/param_types.go @@ -154,6 +154,29 @@ type Param struct { Value ParamValue `json:"value"` } +// GetVarSubstitutionExpressions extracts all the value between "$(" and ")"" for a Parameter +func (p Param) GetVarSubstitutionExpressions() ([]string, bool) { + var allExpressions []string + switch p.Value.Type { + case ParamTypeArray: + // array type + for _, value := range p.Value.ArrayVal { + allExpressions = append(allExpressions, validateString(value)...) + } + case ParamTypeString: + // string type + allExpressions = append(allExpressions, validateString(p.Value.StringVal)...) + case ParamTypeObject: + // object type + for _, value := range p.Value.ObjectVal { + allExpressions = append(allExpressions, validateString(value)...) + } + default: + return nil, false + } + return allExpressions, len(allExpressions) != 0 +} + // ExtractNames returns a set of unique names func (ps Params) ExtractNames() sets.String { names := sets.String{} diff --git a/pkg/apis/pipeline/v1/pipeline_types.go b/pkg/apis/pipeline/v1/pipeline_types.go index 700717f1c31..25613f1a41a 100644 --- a/pkg/apis/pipeline/v1/pipeline_types.go +++ b/pkg/apis/pipeline/v1/pipeline_types.go @@ -330,3 +330,15 @@ type PipelineList struct { metav1.ListMeta `json:"metadata,omitempty"` Items []Pipeline `json:"items"` } + +// GetVarSubstitutionExpressions extracts all the value between "$(" and ")"" for a PipelineResult +func (result PipelineResult) GetVarSubstitutionExpressions() ([]string, bool) { + allExpressions := validateString(result.Value.StringVal) + for _, v := range result.Value.ArrayVal { + allExpressions = append(allExpressions, validateString(v)...) + } + for _, v := range result.Value.ObjectVal { + allExpressions = append(allExpressions, validateString(v)...) + } + return allExpressions, len(allExpressions) != 0 +} diff --git a/pkg/apis/pipeline/v1/pipeline_validation.go b/pkg/apis/pipeline/v1/pipeline_validation.go index 97ddd8801b1..71b002d9bcd 100644 --- a/pkg/apis/pipeline/v1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1/pipeline_validation.go @@ -530,7 +530,7 @@ func validateExecutionStatusVariablesInFinally(tasksNames sets.String, finally [ func (pt *PipelineTask) validateExecutionStatusVariablesDisallowed() (errs *apis.FieldError) { for _, param := range pt.Params { - if expressions, ok := GetVarSubstitutionExpressionsForParam(param); ok { + if expressions, ok := param.GetVarSubstitutionExpressions(); ok { errs = errs.Also(validateContainsExecutionStatusVariablesDisallowed(expressions, "value"). ViaFieldKey("params", param.Name)) } @@ -546,7 +546,7 @@ func (pt *PipelineTask) validateExecutionStatusVariablesDisallowed() (errs *apis func (pt *PipelineTask) validateExecutionStatusVariablesAllowed(ptNames sets.String) (errs *apis.FieldError) { for _, param := range pt.Params { - if expressions, ok := GetVarSubstitutionExpressionsForParam(param); ok { + if expressions, ok := param.GetVarSubstitutionExpressions(); ok { errs = errs.Also(validateExecutionStatusVariablesExpressions(expressions, ptNames, "value"). ViaFieldKey("params", param.Name)) } @@ -625,7 +625,7 @@ func validatePipelineResults(results []PipelineResult, tasks []PipelineTask, fin pipelineTaskNames := getPipelineTasksNames(tasks) pipelineFinallyTaskNames := getPipelineTasksNames(finally) for idx, result := range results { - expressions, ok := GetVarSubstitutionExpressionsForPipelineResult(result) + expressions, ok := result.GetVarSubstitutionExpressions() if !ok { errs = errs.Also(apis.ErrInvalidValue("expected pipeline results to be task result expressions but no expressions were found", "value").ViaFieldIndex("results", idx)) @@ -713,7 +713,7 @@ func validateFinalTasks(tasks []PipelineTask, finalTasks []PipelineTask) (errs * func validateTaskResultReferenceInFinallyTasks(finalTasks []PipelineTask, ts sets.String, fts sets.String) (errs *apis.FieldError) { for idx, t := range finalTasks { for _, p := range t.Params { - if expressions, ok := GetVarSubstitutionExpressionsForParam(p); ok { + if expressions, ok := p.GetVarSubstitutionExpressions(); ok { errs = errs.Also(validateResultsVariablesExpressionsInFinally(expressions, ts, fts, "value").ViaFieldKey( "params", p.Name).ViaFieldIndex("finally", idx)) } diff --git a/pkg/apis/pipeline/v1/pipelinerun_validation.go b/pkg/apis/pipeline/v1/pipelinerun_validation.go index 9de46800df4..dc95f47ccbe 100644 --- a/pkg/apis/pipeline/v1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1/pipelinerun_validation.go @@ -131,7 +131,7 @@ func (ps *PipelineRunSpec) validatePipelineRunParameters(ctx context.Context) (e // Validate that task results aren't used in param values for _, param := range ps.Params { - expressions, ok := GetVarSubstitutionExpressionsForParam(param) + expressions, ok := param.GetVarSubstitutionExpressions() if ok { if LooksLikeContainsResultRefs(expressions) { expressions = filter(expressions, looksLikeResultRef) diff --git a/pkg/apis/pipeline/v1/resultref.go b/pkg/apis/pipeline/v1/resultref.go index 5a7610e0325..14b0de17fdb 100644 --- a/pkg/apis/pipeline/v1/resultref.go +++ b/pkg/apis/pipeline/v1/resultref.go @@ -105,41 +105,6 @@ func looksLikeResultRef(expression string) bool { return len(subExpressions) >= 4 && (subExpressions[0] == ResultTaskPart || subExpressions[0] == ResultFinallyPart) && subExpressions[2] == ResultResultPart } -// GetVarSubstitutionExpressionsForParam extracts all the value between "$(" and ")"" for a parameter -func GetVarSubstitutionExpressionsForParam(param Param) ([]string, bool) { - var allExpressions []string - switch param.Value.Type { - case ParamTypeArray: - // array type - for _, value := range param.Value.ArrayVal { - allExpressions = append(allExpressions, validateString(value)...) - } - case ParamTypeString: - // string type - allExpressions = append(allExpressions, validateString(param.Value.StringVal)...) - case ParamTypeObject: - // object type - for _, value := range param.Value.ObjectVal { - allExpressions = append(allExpressions, validateString(value)...) - } - default: - return nil, false - } - return allExpressions, len(allExpressions) != 0 -} - -// GetVarSubstitutionExpressionsForPipelineResult extracts all the value between "$(" and ")"" for a pipeline result -func GetVarSubstitutionExpressionsForPipelineResult(result PipelineResult) ([]string, bool) { - allExpressions := validateString(result.Value.StringVal) - for _, v := range result.Value.ArrayVal { - allExpressions = append(allExpressions, validateString(v)...) - } - for _, v := range result.Value.ObjectVal { - allExpressions = append(allExpressions, validateString(v)...) - } - return allExpressions, len(allExpressions) != 0 -} - func validateString(value string) []string { expressions := VariableSubstitutionRegex.FindAllString(value, -1) if expressions == nil { @@ -208,7 +173,7 @@ func ParseResultName(resultName string) (string, string) { func PipelineTaskResultRefs(pt *PipelineTask) []*ResultRef { refs := []*ResultRef{} for _, p := range pt.extractAllParams() { - expressions, _ := GetVarSubstitutionExpressionsForParam(p) + expressions, _ := p.GetVarSubstitutionExpressions() refs = append(refs, NewResultRefs(expressions)...) } for _, whenExpression := range pt.When { diff --git a/pkg/apis/pipeline/v1/resultref_test.go b/pkg/apis/pipeline/v1/resultref_test.go index 5db4029405f..a471418edec 100644 --- a/pkg/apis/pipeline/v1/resultref_test.go +++ b/pkg/apis/pipeline/v1/resultref_test.go @@ -172,7 +172,7 @@ func TestNewResultReference(t *testing.T) { }}, }} { t.Run(tt.name, func(t *testing.T) { - expressions, ok := v1.GetVarSubstitutionExpressionsForParam(tt.param) + expressions, ok := tt.param.GetVarSubstitutionExpressions() if !ok && tt.want != nil { t.Fatalf("expected to find expressions but didn't find any") } else { @@ -272,7 +272,7 @@ func TestHasResultReference(t *testing.T) { }}, }} { t.Run(tt.name, func(t *testing.T) { - expressions, ok := v1.GetVarSubstitutionExpressionsForParam(tt.param) + expressions, ok := tt.param.GetVarSubstitutionExpressions() if !ok { t.Fatalf("expected to find expressions but didn't find any") } @@ -355,7 +355,7 @@ func TestLooksLikeResultRef(t *testing.T) { want: true, }} { t.Run(tt.name, func(t *testing.T) { - expressions, ok := v1.GetVarSubstitutionExpressionsForParam(tt.param) + expressions, ok := tt.param.GetVarSubstitutionExpressions() if ok { if got := v1.LooksLikeContainsResultRefs(expressions); got != tt.want { t.Errorf("LooksLikeContainsResultRefs() = %v, want %v", got, tt.want) @@ -755,7 +755,7 @@ func TestGetVarSubstitutionExpressionsForPipelineResult(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - get, _ := v1.GetVarSubstitutionExpressionsForPipelineResult(tt.result) + get, _ := tt.result.GetVarSubstitutionExpressions() if d := cmp.Diff(tt.want, get, cmpopts.SortSlices(sortStrings)); d != "" { t.Error(diff.PrintWantGot(d)) } diff --git a/pkg/reconciler/pipelinerun/resources/apply.go b/pkg/reconciler/pipelinerun/resources/apply.go index 63171581f2c..b356947bbbd 100644 --- a/pkg/reconciler/pipelinerun/resources/apply.go +++ b/pkg/reconciler/pipelinerun/resources/apply.go @@ -333,7 +333,7 @@ func ApplyTaskResultsToPipelineResults( arrayReplacements := map[string][]string{} objectReplacements := map[string]map[string]string{} for _, pipelineResult := range results { - variablesInPipelineResult, _ := v1.GetVarSubstitutionExpressionsForPipelineResult(pipelineResult) + variablesInPipelineResult, _ := pipelineResult.GetVarSubstitutionExpressions() if len(variablesInPipelineResult) == 0 { continue } diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index b30b0a7862f..8e47b94a303 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -697,7 +697,7 @@ func (t *ResolvedPipelineTask) hasResultReferences() bool { matrixParams = t.PipelineTask.Params } for _, param := range append(t.PipelineTask.Params, matrixParams...) { - if ps, ok := v1.GetVarSubstitutionExpressionsForParam(param); ok { + if ps, ok := param.GetVarSubstitutionExpressions(); ok { if v1.LooksLikeContainsResultRefs(ps) { return true } diff --git a/pkg/reconciler/pipelinerun/resources/validate_dependencies.go b/pkg/reconciler/pipelinerun/resources/validate_dependencies.go index 81b845e06c8..2ec3a2b3a8a 100644 --- a/pkg/reconciler/pipelinerun/resources/validate_dependencies.go +++ b/pkg/reconciler/pipelinerun/resources/validate_dependencies.go @@ -46,7 +46,7 @@ func ValidatePipelineTaskResults(state PipelineRunState) error { func ValidatePipelineResults(ps *v1.PipelineSpec, state PipelineRunState) error { ptMap := state.ToMap() for _, result := range ps.Results { - expressions, _ := v1.GetVarSubstitutionExpressionsForPipelineResult(result) + expressions, _ := result.GetVarSubstitutionExpressions() refs := v1.NewResultRefs(expressions) for _, ref := range refs { if err := validateResultRef(ref, ptMap); err != nil {