diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go index 0075e0beb0c..69c30284b75 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go @@ -153,7 +153,12 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { } func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) error { - p, serviceAccount, verr := validatePipelineRun(c, pr) + p, serviceAccount, verr := ValidatePipelineRun( + pr, + c.pipelineLister.Pipelines(pr.Namespace).Get, + c.taskLister.Tasks(pr.Namespace).Get, + c.resourceLister.PipelineResources(pr.Namespace).Get, + ) if verr != nil { c.Logger.Error("Failed to validate pipelinerun %s with error %v", pr.Name, verr) pr.Status.SetCondition(&duckv1alpha1.Condition{ diff --git a/pkg/reconciler/v1alpha1/pipelinerun/validate.go b/pkg/reconciler/v1alpha1/pipelinerun/validate.go index 0fc603b3e2a..722298b6094 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/validate.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/validate.go @@ -22,21 +22,30 @@ import ( "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" ) -// validate all references in pipelinerun exist at runtime -func validatePipelineRun(c *Reconciler, pr *v1alpha1.PipelineRun) (*v1alpha1.Pipeline, string, error) { +// GetPipeline is used to retrieve existing instance of Pipeline called name +type GetPipeline func(name string) (*v1alpha1.Pipeline, error) + +// GetResource is a function used to retrieve PipelineResources. +type GetResource func(string) (*v1alpha1.PipelineResource, error) + +// GetTask is a function that will retrieve the task called name. +type GetTask func(name string) (*v1alpha1.Task, error) + +// ValidatePipelineRun validates all references in pipelinerun exist at runtime +func ValidatePipelineRun(pr *v1alpha1.PipelineRun, getPipeline GetPipeline, getTask GetTask, getResource GetResource) (*v1alpha1.Pipeline, string, error) { sa := pr.Spec.ServiceAccount // verify pipeline reference and all corresponding tasks exist - p, err := c.pipelineLister.Pipelines(pr.Namespace).Get(pr.Spec.PipelineRef.Name) + p, err := getPipeline(pr.Spec.PipelineRef.Name) if err != nil { return nil, sa, fmt.Errorf("error listing pipeline ref %s: %v", pr.Spec.PipelineRef.Name, err) } - if err := validatePipelineTask(c, p.Spec.Tasks, pr.Spec.PipelineTaskResources, pr.Namespace); err != nil { + if err := validatePipelineTask(getTask, getResource, p.Spec.Tasks, pr.Spec.PipelineTaskResources, pr.Namespace); err != nil { return nil, sa, fmt.Errorf("error validating tasks in pipeline %s: %v", p.Name, err) } return p, sa, nil } -func validatePipelineTaskAndTask(c *Reconciler, ptask v1alpha1.PipelineTask, task *v1alpha1.Task, inputs []v1alpha1.TaskResourceBinding, outputs []v1alpha1.TaskResourceBinding, ns string) error { +func validatePipelineTaskAndTask(getResource GetResource, ptask v1alpha1.PipelineTask, task *v1alpha1.Task, inputs []v1alpha1.TaskResourceBinding, outputs []v1alpha1.TaskResourceBinding, ns string) error { // a map from the name of the input resource to the type inputMapping := map[string]v1alpha1.PipelineResourceType{} // a map from the name of the output resource to the type @@ -52,7 +61,7 @@ func validatePipelineTaskAndTask(c *Reconciler, ptask v1alpha1.PipelineTask, tas inputMapping[r.Name] = "" // TODO(#213): if this is the empty string should it be an error? or maybe let the lookup fail? if r.ResourceRef.Name != "" { - rr, err := c.resourceLister.PipelineResources(ns).Get(r.ResourceRef.Name) + rr, err := getResource(r.ResourceRef.Name) if err != nil { return fmt.Errorf("error listing input pipeline resource for task %s: %v ", ptask.Name, err) } @@ -64,7 +73,7 @@ func validatePipelineTaskAndTask(c *Reconciler, ptask v1alpha1.PipelineTask, tas outputMapping[r.Name] = "" // TODO(#213): if this is the empty string should it be an error? or maybe let the lookup fail? if r.ResourceRef.Name != "" { - rr, err := c.resourceLister.PipelineResources(ns).Get(r.ResourceRef.Name) + rr, err := getResource(r.ResourceRef.Name) if err != nil { return fmt.Errorf("error listing output pipeline resource for task %s: %v ", ptask.Name, err) } @@ -104,9 +113,9 @@ func validatePipelineTaskAndTask(c *Reconciler, ptask v1alpha1.PipelineTask, tas return nil } -func validatePipelineTask(c *Reconciler, tasks []v1alpha1.PipelineTask, resources []v1alpha1.PipelineTaskResource, ns string) error { +func validatePipelineTask(getTask GetTask, getResource GetResource, tasks []v1alpha1.PipelineTask, resources []v1alpha1.PipelineTaskResource, ns string) error { for _, pt := range tasks { - t, err := c.taskLister.Tasks(ns).Get(pt.TaskRef.Name) + t, err := getTask(pt.TaskRef.Name) if err != nil { return fmt.Errorf("couldn't get task %q: %s", pt.TaskRef.Name, err) } @@ -114,7 +123,7 @@ func validatePipelineTask(c *Reconciler, tasks []v1alpha1.PipelineTask, resource found := false for _, tr := range resources { if tr.Name == pt.Name { - if err := validatePipelineTaskAndTask(c, pt, t, tr.Inputs, tr.Outputs, ns); err != nil { + if err := validatePipelineTaskAndTask(getResource, pt, t, tr.Inputs, tr.Outputs, ns); err != nil { return fmt.Errorf("pipelineTask %q is invalid: %s", pt.Name, err) } found = true diff --git a/pkg/reconciler/v1alpha1/pipelinerun/validate_test.go b/pkg/reconciler/v1alpha1/pipelinerun/validate_test.go index 0696a618597..f86f0a7f7ec 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/validate_test.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/validate_test.go @@ -1,13 +1,11 @@ package pipelinerun_test import ( - "context" "testing" "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" - "github.com/knative/build-pipeline/test" - duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" - corev1 "k8s.io/api/core/v1" + "github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/pipelinerun" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -310,22 +308,33 @@ func Test_InvalidPipelineTask(t *testing.T) { for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - d := test.Data{ - PipelineRuns: prs, - Pipelines: ps, - Tasks: ts, - PipelineResources: rr, + getPipeline := func(name string) (*v1alpha1.Pipeline, error) { + for _, p := range ps { + if p.Name == name { + return p, nil + } + } + return nil, errors.NewNotFound(v1alpha1.Resource("pipeline"), name) } - - c, _, _ := test.GetPipelineRunController(d) - err := c.Reconciler.Reconcile(context.Background(), "foo/"+tc.pipelineRun.Name) - - if err != nil { - t.Errorf("Did not expect to see error when reconciling invalid PipelineRun but saw %q", err) + getTask := func(name string) (*v1alpha1.Task, error) { + for _, t := range ts { + if t.Name == name { + return t, nil + } + } + return nil, errors.NewNotFound(v1alpha1.Resource("task"), name) + } + getResource := func(name string) (*v1alpha1.PipelineResource, error) { + for _, r := range rr { + if r.Name == name { + return r, nil + } + } + return nil, errors.NewNotFound(v1alpha1.Resource("pipelineResource"), name) } - condition := tc.pipelineRun.Status.GetCondition(duckv1alpha1.ConditionSucceeded) - if condition == nil || condition.Status != corev1.ConditionFalse { - t.Errorf("Expected status to be failed on invalid PipelineRun %s but was: %v", tc.pipeline.Name, condition) + _, _, err := pipelinerun.ValidatePipelineRun(tc.pipelineRun, getPipeline, getTask, getResource) + if err == nil { + t.Errorf("Expected error from validating invalid PipelineRun but was none") } }) }