From cf2ecd56bcb73fa25118a280a2562a69b8957b83 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Mon, 24 Jun 2019 11:57:06 +0200 Subject: [PATCH] Fix Pipeline error with ClusterTasks It is possible to use `ClusterTask` in a Pipeline, for that you specify that the `Task` you want to use is of type `ClusterTask` (default being `Task`). But, before this change, the `PipelineRun` using this `Pipeline` would fail as the `Kind` information is not passed from the `PipelineTask.TaskRef` to the actual `TaskRun`. This fixes that by passing the `Kind` of a `TaskRef` when resolving the `Task`/`TaskRun` in the `PipelineRun` reconcilier. A yaml test is added as a regression test. Signed-off-by: Vincent Demeester --- .../pipelineruns/clustertask-pipelinerun.yaml | 32 +++++++++++++++++++ .../v1alpha1/pipelinerun/pipelinerun.go | 4 ++- .../resources/pipelinerunresolution.go | 2 +- .../resources/taskresourceresolution.go | 4 ++- .../resources/taskresourceresolution_test.go | 9 +++--- pkg/reconciler/v1alpha1/taskrun/taskrun.go | 10 +++--- 6 files changed, 50 insertions(+), 11 deletions(-) create mode 100644 examples/pipelineruns/clustertask-pipelinerun.yaml diff --git a/examples/pipelineruns/clustertask-pipelinerun.yaml b/examples/pipelineruns/clustertask-pipelinerun.yaml new file mode 100644 index 00000000000..a5369ee4452 --- /dev/null +++ b/examples/pipelineruns/clustertask-pipelinerun.yaml @@ -0,0 +1,32 @@ +apiVersion: tekton.dev/v1alpha1 +kind: ClusterTask +metadata: + name: cluster-task-pipeline-4 +spec: + steps: + - name: task-two-step-one + image: ubuntu + command: ["/bin/bash"] + args: ['-c', 'echo success'] +--- +apiVersion: tekton.dev/v1alpha1 +kind: Pipeline +metadata: + name: sample-pipeline-cluster-task-4 +spec: + tasks: + - name: cluster-task-pipeline-4 + taskRef: + name: cluster-task-pipeline-4 + kind: ClusterTask +--- +apiVersion: tekton.dev/v1alpha1 +kind: PipelineRun +metadata: + name: demo-pipeline-run-4 +spec: + pipelineRef: + name: sample-pipeline-cluster-task-4 + trigger: + type: manual + serviceAccount: 'default' diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go index afa2df6d710..23a602fb683 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go @@ -504,6 +504,7 @@ func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.Re Spec: v1alpha1.TaskRunSpec{ TaskRef: &v1alpha1.TaskRef{ Name: rprt.ResolvedTaskResources.TaskName, + Kind: rprt.ResolvedTaskResources.Kind, }, Inputs: v1alpha1.TaskRunInputs{ Params: rprt.PipelineTask.Params, @@ -513,7 +514,8 @@ func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, rprt *resources.Re NodeSelector: pr.Spec.NodeSelector, Tolerations: pr.Spec.Tolerations, Affinity: pr.Spec.Affinity, - }} + }, + } resources.WrapSteps(&tr.Spec, rprt.PipelineTask, rprt.ResolvedTaskResources.Inputs, rprt.ResolvedTaskResources.Outputs, storageBasePath) diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go index 341ac59dd9f..74515fc6446 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go @@ -246,7 +246,7 @@ func ResolvePipelineRun( } spec := t.TaskSpec() - rtr, err := resources.ResolveTaskResources(&spec, t.TaskMetadata().Name, inputs, outputs, getResource) + rtr, err := resources.ResolveTaskResources(&spec, t.TaskMetadata().Name, pt.TaskRef.Kind, inputs, outputs, getResource) if err != nil { return nil, &ResourceNotFoundError{Msg: err.Error()} } diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution.go b/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution.go index 9e8e1f11c98..12c8087eca1 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution.go @@ -25,6 +25,7 @@ import ( // the TaskRun: the TaskRun, it's Task and the PipelineResources it needs. type ResolvedTaskResources struct { TaskName string + Kind v1alpha1.TaskKind TaskSpec *v1alpha1.TaskSpec // Inputs is a map from the name of the input required by the Task // to the actual Resource to use for it @@ -40,10 +41,11 @@ type GetResource func(string) (*v1alpha1.PipelineResource, error) // ResolveTaskResources looks up PipelineResources referenced by inputs and outputs and returns // a structure that unites the resolved references and the Task Spec. If referenced PipelineResources // can't be found, an error is returned. -func ResolveTaskResources(ts *v1alpha1.TaskSpec, taskName string, inputs []v1alpha1.TaskResourceBinding, outputs []v1alpha1.TaskResourceBinding, gr GetResource) (*ResolvedTaskResources, error) { +func ResolveTaskResources(ts *v1alpha1.TaskSpec, taskName string, kind v1alpha1.TaskKind, inputs []v1alpha1.TaskResourceBinding, outputs []v1alpha1.TaskResourceBinding, gr GetResource) (*ResolvedTaskResources, error) { rtr := ResolvedTaskResources{ TaskName: taskName, TaskSpec: ts, + Kind: kind, Inputs: map[string]*v1alpha1.PipelineResource{}, Outputs: map[string]*v1alpha1.PipelineResource{}, } diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution_test.go index 71684831182..da9b36de79c 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/taskresourceresolution_test.go @@ -61,6 +61,7 @@ func TestResolveTaskRun(t *testing.T) { }} taskName := "orchestrate" + kind := v1alpha1.NamespacedTaskKind taskSpec := v1alpha1.TaskSpec{ Steps: []corev1.Container{{ Name: "step1", @@ -90,7 +91,7 @@ func TestResolveTaskRun(t *testing.T) { return r, nil } - rtr, err := ResolveTaskResources(&taskSpec, taskName, inputs, outputs, gr) + rtr, err := ResolveTaskResources(&taskSpec, taskName, kind, inputs, outputs, gr) if err != nil { t.Fatalf("Did not expect error trying to resolve TaskRun: %s", err) } @@ -170,7 +171,7 @@ func TestResolveTaskRun_missingOutput(t *testing.T) { }}} gr := func(n string) (*v1alpha1.PipelineResource, error) { return nil, xerrors.New("nope") } - _, err := ResolveTaskResources(&v1alpha1.TaskSpec{}, "orchestrate", []v1alpha1.TaskResourceBinding{}, outputs, gr) + _, err := ResolveTaskResources(&v1alpha1.TaskSpec{}, "orchestrate", v1alpha1.NamespacedTaskKind, []v1alpha1.TaskResourceBinding{}, outputs, gr) if err == nil { t.Fatalf("Expected to get error because output resource couldn't be resolved") } @@ -184,7 +185,7 @@ func TestResolveTaskRun_missingInput(t *testing.T) { }}} gr := func(n string) (*v1alpha1.PipelineResource, error) { return nil, xerrors.New("nope") } - _, err := ResolveTaskResources(&v1alpha1.TaskSpec{}, "orchestrate", inputs, []v1alpha1.TaskResourceBinding{}, gr) + _, err := ResolveTaskResources(&v1alpha1.TaskSpec{}, "orchestrate", v1alpha1.NamespacedTaskKind, inputs, []v1alpha1.TaskResourceBinding{}, gr) if err == nil { t.Fatalf("Expected to get error because output resource couldn't be resolved") } @@ -198,7 +199,7 @@ func TestResolveTaskRun_noResources(t *testing.T) { gr := func(n string) (*v1alpha1.PipelineResource, error) { return &v1alpha1.PipelineResource{}, nil } - rtr, err := ResolveTaskResources(&taskSpec, "orchestrate", []v1alpha1.TaskResourceBinding{}, []v1alpha1.TaskResourceBinding{}, gr) + rtr, err := ResolveTaskResources(&taskSpec, "orchestrate", v1alpha1.NamespacedTaskKind, []v1alpha1.TaskResourceBinding{}, []v1alpha1.TaskResourceBinding{}, gr) if err != nil { t.Fatalf("Did not expect error trying to resolve TaskRun: %s", err) } diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go index e40fd1788fc..cb55c749ee0 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun.go @@ -193,8 +193,9 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { return err } -func (c *Reconciler) getTaskFunc(tr *v1alpha1.TaskRun) resources.GetTask { +func (c *Reconciler) getTaskFunc(tr *v1alpha1.TaskRun) (resources.GetTask, v1alpha1.TaskKind) { var gtFunc resources.GetTask + kind := v1alpha1.NamespacedTaskKind if tr.Spec.TaskRef != nil && tr.Spec.TaskRef.Kind == v1alpha1.ClusterTaskKind { gtFunc = func(name string) (v1alpha1.TaskInterface, error) { t, err := c.clusterTaskLister.Get(name) @@ -203,6 +204,7 @@ func (c *Reconciler) getTaskFunc(tr *v1alpha1.TaskRun) resources.GetTask { } return t, nil } + kind = v1alpha1.ClusterTaskKind } else { gtFunc = func(name string) (v1alpha1.TaskInterface, error) { t, err := c.taskLister.Tasks(tr.Namespace).Get(name) @@ -212,7 +214,7 @@ func (c *Reconciler) getTaskFunc(tr *v1alpha1.TaskRun) resources.GetTask { return t, nil } } - return gtFunc + return gtFunc, kind } func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error { @@ -225,7 +227,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error return err } - getTaskFunc := c.getTaskFunc(tr) + getTaskFunc, kind := c.getTaskFunc(tr) taskMeta, taskSpec, err := resources.GetTaskData(tr, getTaskFunc) if err != nil { c.Logger.Errorf("Failed to determine Task spec to use for taskrun %s: %v", tr.Name, err) @@ -265,7 +267,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error return nil } - rtr, err := resources.ResolveTaskResources(taskSpec, taskMeta.Name, tr.Spec.Inputs.Resources, tr.Spec.Outputs.Resources, c.resourceLister.PipelineResources(tr.Namespace).Get) + rtr, err := resources.ResolveTaskResources(taskSpec, taskMeta.Name, kind, tr.Spec.Inputs.Resources, tr.Spec.Outputs.Resources, c.resourceLister.PipelineResources(tr.Namespace).Get) if err != nil { c.Logger.Errorf("Failed to resolve references for taskrun %s: %v", tr.Name, err) tr.Status.SetCondition(&apis.Condition{