Repository: kubeflow/kfp-tekton (language: python)

Issue:
Enhance unit tests: Add pipelineloop validation
/kind feature

**Description:**
As part of our unit tests, we also want to check the pipelineloop validation to make sure all the parameters and tasks are mapped correctly.

Todo:
Update the unit tests below to also check against the pipelineloop validation cli.
https://github.com/kubeflow/kfp-tekton/blob/master/.github/workflows/kfp-tekton-unittests.yml#L27-L28

Github actions are possible to install both golang and python runtime in the same job, so we can also build the pipelineloop cli as part of these unit tests.

**Additional information:**
[Miscellaneous information that will assist in solving the issue.]



Discussion / hints:
/assign @ScrapCodes 

Fix patch:
diff --git a/Makefile b/Makefile
index f814bb1b6a..0a5cecbecf 100644
--- a/Makefile
+++ b/Makefile
@@ -42,8 +42,12 @@ $(VENV)/bin/activate: sdk/python/setup.py
 install: venv ## Install the kfp_tekton compiler in a virtual environment
 	@echo "Run 'source $(VENV)/bin/activate' to activate the virtual environment."
 
+.PHONY: validate_testdata
+validate-testdata:
+	@cd tekton-catalog/pipeline-loops/ && make validate-testdata-python-sdk
+
 .PHONY: unit_test
-unit_test: venv ## Run compiler unit tests
+unit_test: venv validate-testdata ## Run compiler unit tests
 	@echo "=================================================================="
 	@echo "Optional environment variables to configure $@, examples:"
 	@sed -n -e 's/# *\(make $@ .*\)/  \1/p' sdk/python/tests/compiler/compiler_tests.py
diff --git a/tekton-catalog/pipeline-loops/Makefile b/tekton-catalog/pipeline-loops/Makefile
index eb5bf378c1..ae1a4c0801 100644
--- a/tekton-catalog/pipeline-loops/Makefile
+++ b/tekton-catalog/pipeline-loops/Makefile
@@ -18,18 +18,28 @@ all:
 KO = ko
 ko: PACKAGE=github.com/google/ko/cmd/ko
 
-apply: $(info apply PipelineLoop controller to the k8s cluster )
+apply: $(info apply: Apply PipelineLoop controller to the k8s cluster )
 	$(KO) apply -f config
 
 test-all:  $(info test-all: run unit tests )
 	go test -v -cover -timeout=${TIMEOUT_TESTS} ./...
 
+.PHONY: init
 init:
 	mkdir -p ${BIN_DIR}
 
+.PHONY: cli
 cli: init $(info cli: build validation cli )
 	go build -o=${BIN_DIR}/pipelineloop-cli ./cmd/cli
 
+.PHONY: validate-testdata-python-sdk
+validate-testdata-python-sdk: cli $(info validate-testdata-python-sdk: validate testdata for python SDK )
+## works w/o findutils installed, but globbing has a limit
+## for x in ../../sdk/python/tests/compiler/testdata/*yaml; do ${BIN_DIR}/pipelineloop-cli -f $$x || true ; done
+	@find ../../sdk/python/tests/compiler/testdata \
+		\( -type f -name "*yaml" -not -path "*\test_data\*" -not -name "*component.yaml" \) \
+		-print0 | xargs -0 -n1 ${BIN_DIR}/pipelineloop-cli -f
+
 local: init
 	go build -o=${BIN_DIR}/pipelineloop-controller ./cmd/controller
 	go build -o=${BIN_DIR}/pipelineloop-webhook ./cmd/webhook
diff --git a/tekton-catalog/pipeline-loops/cmd/cli/main.go b/tekton-catalog/pipeline-loops/cmd/cli/main.go
index 79f8ba4ab0..5b3fee73a4 100644
--- a/tekton-catalog/pipeline-loops/cmd/cli/main.go
+++ b/tekton-catalog/pipeline-loops/cmd/cli/main.go
@@ -50,7 +50,7 @@ func validateFlags(action, inputFileName, inputFileType string) {
 		errs = append(errs, err)
 	}
 	if len(errs) > 0 {
-		fmt.Println("Errors while processing input:")
+		fmt.Println("Errors while processing input for filename: ", inputFileName)
 		for i, e := range errs {
 			fmt.Printf("Error: %d -> %v\n", i, e)
 		}
@@ -60,10 +60,11 @@ func validateFlags(action, inputFileName, inputFileType string) {
 
 func main() {
 	var action, inputFileName, inputFileType string
-
+	var quiet bool
 	flag.StringVar(&action, "a", "validate", "The `action` on the resource.")
 	flag.StringVar(&inputFileName, "f", "", "path to input `filename` either a yaml or json.")
 	flag.StringVar(&inputFileType, "file-type", "yaml", "`yaml` or a json")
+	flag.BoolVar(&quiet, "q", false, "quiet mode, report only errors.")
 	flag.Parse()
 	validateFlags(action, inputFileName, inputFileType)
 	if inputFileType == "json" {
@@ -74,14 +75,16 @@ func main() {
 	if inputFileType == "yaml" {
 		objs, err := readFile(inputFileName)
 		if err != nil {
-			fmt.Printf("\nError while reading input spec: %v\n", err)
-			os.Exit(2)
+			fmt.Printf("\nWarn: skipping file:%s, %v\n", inputFileName, err)
+			// os.Exit(2) These are warning because, certain yaml are not k8s resource and we can skip.
+			os.Exit(0)
 		}
 		for _, o := range objs {
 			marshalledBytes, err := o.MarshalJSON()
 			if err != nil {
-				fmt.Printf("\nError while marshalling json: %v\n", err)
-				os.Exit(3)
+				fmt.Printf("\nWarn: skipping file due to json Marshal errors:%s, %v\n", inputFileName, err)
+				// os.Exit(3) These are warning because, certain yaml are not k8s resource and we can skip.
+				os.Exit(0)
 			}
 			err = nil
 			switch kind := o.GetKind(); kind {
@@ -96,25 +99,29 @@ func main() {
 			case "PipelineRun":
 				err = validatePipelineRun(marshalledBytes)
 			default:
-				fmt.Printf("\nWarn: Unsupported kind: %s.\n", kind)
+				if !quiet {
+					fmt.Printf("\nWarn: Unsupported kind: %s.\n", kind)
+				}
 			}
 			if err != nil {
 				errs = append(errs, err.Error())
 			}
 		}
 		if len(errs) > 0 {
-			fmt.Printf("\nValidation errors: %s\n", strings.Join(errs, "\n"))
+			fmt.Printf("\nFound validation errors in %s: \n%s\n", inputFileName,
+				strings.Join(errs, "\n"))
 			os.Exit(100)
 		} else {
-			fmt.Printf("\nCongratulations, all checks passed !!\n")
+			if !quiet {
+				fmt.Printf("\nCongratulations, all checks passed in %s\n", inputFileName)
+			}
 		}
 	}
 }
 
 func validatePipeline(bytes []byte) error {
 	p := v1beta1.Pipeline{}
-	err := json.Unmarshal(bytes, &p)
-	if err != nil {
+	if err := json.Unmarshal(bytes, &p); err != nil {
 		return err
 	}
 	return validatePipelineSpec(&p.Spec, p.Name)
@@ -146,7 +153,7 @@ func validateRun(bytes []byte) error {
 	r := v1alpha1.Run{}
 	err := json.Unmarshal(bytes, &r)
 	if err != nil {
-		return err
+		return fmt.Errorf("Error while unmarshal Run:%s\n", err.Error())
 	}
 	// We do not need to validate Run because it is validated by tekton admission webhook
 	// And r.Spec.Ref is also validated by tekton.
@@ -161,9 +168,8 @@ func validateRun(bytes []byte) error {
 
 func validatePipelineRun(bytes []byte) error {
 	pr := v1beta1.PipelineRun{}
-	err := json.Unmarshal(bytes, &pr)
-	if err != nil {
-		return err
+	if err := json.Unmarshal(bytes, &pr); err != nil {
+		return fmt.Errorf("Error while unmarshal PipelineRun spec:%s\n", err.Error())
 	}
 	return validatePipelineSpec(pr.Spec.PipelineSpec, pr.Name)
 }
@@ -171,7 +177,7 @@ func validatePipelineRun(bytes []byte) error {
 func validatePipelineLoopEmbedded(bytes []byte) error {
 	var embeddedSpec map[string]interface{}
 	if err := json.Unmarshal(bytes, &embeddedSpec); err != nil {
-		return err
+		return fmt.Errorf("Error while unmarshal PipelineLoop embedded spec:%s\n", err.Error())
 	}
 	r1 := map[string]interface{}{
 		"kind":       pipelineloop.PipelineLoopControllerName,
@@ -182,7 +188,7 @@ func validatePipelineLoopEmbedded(bytes []byte) error {
 
 	marshalBytes, err := json.Marshal(r1)
 	if err != nil {
-		return err
+		return fmt.Errorf("Error while marshalling embedded to PipelineLoop:%s\n", err.Error())
 	}
 	return validatePipelineLoop(marshalBytes)
 }
@@ -190,7 +196,7 @@ func validatePipelineLoopEmbedded(bytes []byte) error {
 func validatePipelineLoop(bytes []byte) error {
 	pipelineLoop := pipelineloopv1alpha1.PipelineLoop{}
 	if err := json.Unmarshal(bytes, &pipelineLoop); err != nil {
-		return err
+		return fmt.Errorf("Error while unmarshal PipelineLoop:%s\n", err.Error())
 	}
 	ctx := context.Background()
 	ctx = pipelinelooprun.EnableCustomTaskFeatureFlag(ctx)


Test patch:
diff --git a/sdk/python/tests/compiler/testdata/conditions_and_loops.py b/sdk/python/tests/compiler/testdata/conditions_and_loops.py
index 13455ac38d..5040ed81b1 100644
--- a/sdk/python/tests/compiler/testdata/conditions_and_loops.py
+++ b/sdk/python/tests/compiler/testdata/conditions_and_loops.py
@@ -64,8 +64,8 @@ def produce_number() -> int:
 
 
 @dsl.pipeline(name='Conditions and loops')
-def conditions_and_loops(n='3', threshold='20'):
-    produce_numbers_task = produce_numbers(n)
+def conditions_and_loops(n1='3', threshold='20'):
+    produce_numbers_task = produce_numbers(n1)
     with dsl.ParallelFor(produce_numbers_task.output) as loop_item:
         add_numbers_task = add_numbers(loop_item, '10')
         print_number_task = print_number(add_numbers_task.output)
diff --git a/sdk/python/tests/compiler/testdata/conditions_and_loops.yaml b/sdk/python/tests/compiler/testdata/conditions_and_loops.yaml
index 1f48e198a8..77a9095d4a 100644
--- a/sdk/python/tests/compiler/testdata/conditions_and_loops.yaml
+++ b/sdk/python/tests/compiler/testdata/conditions_and_loops.yaml
@@ -16,7 +16,7 @@ apiVersion: tekton.dev/v1beta1
 kind: PipelineRun
 metadata:
   annotations:
-    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "3", "name": "n",
+    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "3", "name": "n1",
       "optional": true}, {"default": "20", "name": "threshold", "optional": true}],
       "name": "Conditions and loops"}'
     sidecar.istio.io/inject: 'false'
@@ -36,21 +36,21 @@ metadata:
   name: conditions-and-loops
 spec:
   params:
-  - name: n
+  - name: n1
     value: '3'
   - name: threshold
     value: '20'
   pipelineSpec:
     params:
     - default: '3'
-      name: n
+      name: n1
     - default: '20'
       name: threshold
     tasks:
     - name: produce-numbers
       params:
-      - name: n
-        value: $(params.n)
+      - name: n1
+        value: $(params.n1)
       taskSpec:
         metadata:
           annotations:
@@ -81,14 +81,14 @@ spec:
             pipelines.kubeflow.org/generation: ''
             pipelines.kubeflow.org/pipelinename: ''
         params:
-        - name: n
+        - name: n1
         results:
         - description: /tmp/outputs/Output/data
           name: output
         steps:
         - args:
           - --n
-          - $(inputs.params.n)
+          - $(inputs.params.n1)
           - '----output-paths'
           - $(results.output.path)
           command:
diff --git a/sdk/python/tests/compiler/testdata/conditions_and_loops_noninlined.yaml b/sdk/python/tests/compiler/testdata/conditions_and_loops_noninlined.yaml
index afea45b45a..3b699ee14e 100644
--- a/sdk/python/tests/compiler/testdata/conditions_and_loops_noninlined.yaml
+++ b/sdk/python/tests/compiler/testdata/conditions_and_loops_noninlined.yaml
@@ -16,7 +16,7 @@ apiVersion: tekton.dev/v1beta1
 kind: PipelineRun
 metadata:
   annotations:
-    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "3", "name": "n",
+    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "3", "name": "n1",
       "optional": true}, {"default": "20", "name": "threshold", "optional": true}],
       "name": "Conditions and loops"}'
     sidecar.istio.io/inject: 'false'
@@ -36,21 +36,21 @@ metadata:
   name: conditions-and-loops
 spec:
   params:
-  - name: n
+  - name: n1
     value: '3'
   - name: threshold
     value: '20'
   pipelineSpec:
     params:
     - default: '3'
-      name: n
+      name: n1
     - default: '20'
       name: threshold
     tasks:
     - name: produce-numbers
       params:
-      - name: n
-        value: $(params.n)
+      - name: n1
+        value: $(params.n1)
       taskSpec:
         metadata:
           annotations:
@@ -81,14 +81,14 @@ spec:
             pipelines.kubeflow.org/generation: ''
             pipelines.kubeflow.org/pipelinename: ''
         params:
-        - name: n
+        - name: n1
         results:
         - description: /tmp/outputs/Output/data
           name: output
         steps:
         - args:
           - --n
-          - $(inputs.params.n)
+          - $(inputs.params.n1)
           - '----output-paths'
           - $(results.output.path)
           command:
diff --git a/sdk/python/tests/compiler/testdata/conditions_with_global_params.py b/sdk/python/tests/compiler/testdata/conditions_with_global_params.py
index 70b3b53aa1..f9e9d01e57 100644
--- a/sdk/python/tests/compiler/testdata/conditions_with_global_params.py
+++ b/sdk/python/tests/compiler/testdata/conditions_with_global_params.py
@@ -48,8 +48,8 @@ def produce_number() -> int:
 
 
 @dsl.pipeline(name='Conditions with global params')
-def conditions_with_global_params(n='5', threshold='10', lower_bound='15'):
-    add_numbers_task = add_numbers(n, lower_bound)
+def conditions_with_global_params(n1='5', threshold='10', lower_bound='15'):
+    add_numbers_task = add_numbers(n1, lower_bound)
     print_number_task = print_number(add_numbers_task.output)
     with dsl.Condition(print_number_task.output > threshold):
         notify_success()
diff --git a/sdk/python/tests/compiler/testdata/conditions_with_global_params.yaml b/sdk/python/tests/compiler/testdata/conditions_with_global_params.yaml
index ab3965c26b..aea4505522 100644
--- a/sdk/python/tests/compiler/testdata/conditions_with_global_params.yaml
+++ b/sdk/python/tests/compiler/testdata/conditions_with_global_params.yaml
@@ -16,7 +16,7 @@ apiVersion: tekton.dev/v1beta1
 kind: PipelineRun
 metadata:
   annotations:
-    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "5", "name": "n",
+    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "5", "name": "n1",
       "optional": true}, {"default": "10", "name": "threshold", "optional": true},
       {"default": "15", "name": "lower_bound", "optional": true}], "name": "Conditions
       with global params"}'
@@ -35,7 +35,7 @@ metadata:
   name: conditions-with-global-params
 spec:
   params:
-  - name: n
+  - name: n1
     value: '5'
   - name: threshold
     value: '10'
@@ -44,7 +44,7 @@ spec:
   pipelineSpec:
     params:
     - default: '5'
-      name: n
+      name: n1
     - default: '10'
       name: threshold
     - default: '15'
@@ -54,8 +54,8 @@ spec:
       params:
       - name: lower_bound
         value: $(params.lower_bound)
-      - name: n
-        value: $(params.n)
+      - name: n1
+        value: $(params.n1)
       taskSpec:
         metadata:
           annotations:
@@ -86,14 +86,14 @@ spec:
             pipelines.kubeflow.org/pipelinename: ''
         params:
         - name: lower_bound
-        - name: n
+        - name: n1
         results:
         - description: /tmp/outputs/Output/data
           name: output
         steps:
         - args:
           - --a
-          - $(inputs.params.n)
+          - $(inputs.params.n1)
           - --b
           - $(inputs.params.lower_bound)
           - '----output-paths'
diff --git a/sdk/python/tests/compiler/testdata/conditions_with_global_params_noninlined.yaml b/sdk/python/tests/compiler/testdata/conditions_with_global_params_noninlined.yaml
index ab3965c26b..aea4505522 100644
--- a/sdk/python/tests/compiler/testdata/conditions_with_global_params_noninlined.yaml
+++ b/sdk/python/tests/compiler/testdata/conditions_with_global_params_noninlined.yaml
@@ -16,7 +16,7 @@ apiVersion: tekton.dev/v1beta1
 kind: PipelineRun
 metadata:
   annotations:
-    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "5", "name": "n",
+    pipelines.kubeflow.org/pipeline_spec: '{"inputs": [{"default": "5", "name": "n1",
       "optional": true}, {"default": "10", "name": "threshold", "optional": true},
       {"default": "15", "name": "lower_bound", "optional": true}], "name": "Conditions
       with global params"}'
@@ -35,7 +35,7 @@ metadata:
   name: conditions-with-global-params
 spec:
   params:
-  - name: n
+  - name: n1
     value: '5'
   - name: threshold
     value: '10'
@@ -44,7 +44,7 @@ spec:
   pipelineSpec:
     params:
     - default: '5'
-      name: n
+      name: n1
     - default: '10'
       name: threshold
     - default: '15'
@@ -54,8 +54,8 @@ spec:
       params:
       - name: lower_bound
         value: $(params.lower_bound)
-      - name: n
-        value: $(params.n)
+      - name: n1
+        value: $(params.n1)
       taskSpec:
         metadata:
           annotations:
@@ -86,14 +86,14 @@ spec:
             pipelines.kubeflow.org/pipelinename: ''
         params:
         - name: lower_bound
-        - name: n
+        - name: n1
         results:
         - description: /tmp/outputs/Output/data
           name: output
         steps:
         - args:
           - --a
-          - $(inputs.params.n)
+          - $(inputs.params.n1)
           - --b
           - $(inputs.params.lower_bound)
           - '----output-paths'