From f1bd35f7b39b1974d24328be6295d845c9a3da63 Mon Sep 17 00:00:00 2001 From: Nader Ziada Date: Wed, 21 Nov 2018 16:56:31 -0500 Subject: [PATCH 1/4] Add TaskSpec to taskRun to allow users to run task with one file - users can specifiy TaskRef or TaskSpec, but not both - update type - add validation and tests - update taskRef everywhere to be a pointer since its now optional Signed-off-by: Nader Ziada --- pkg/apis/pipeline/v1alpha1/taskrun_types.go | 6 +- .../pipeline/v1alpha1/taskrun_validation.go | 12 +++- .../v1alpha1/taskrun_validation_test.go | 55 ++++++++++++++--- .../v1alpha1/pipelinerun/pipelinerun.go | 2 +- .../v1alpha1/pipelinerun/pipelinerun_test.go | 2 +- .../taskrun/resources/input_resource_test.go | 59 +++++++++++++++++-- .../v1alpha1/taskrun/taskrun_test.go | 16 ++--- test/cluster_resource_test.go | 2 +- test/crd.go | 2 +- test/helm_task_test.go | 4 +- test/kaniko_task_test.go | 2 +- 11 files changed, 131 insertions(+), 31 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_types.go b/pkg/apis/pipeline/v1alpha1/taskrun_types.go index 95ef449d7e0..1fc504079a0 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_types.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_types.go @@ -35,7 +35,6 @@ var _ webhook.GenericCRD = (*TaskRun)(nil) // TaskRunSpec defines the desired state of TaskRun type TaskRunSpec struct { - TaskRef TaskRef `json:"taskRef"` Trigger TaskTrigger `json:"trigger"` // +optional Inputs TaskRunInputs `json:"inputs,omitempty"` @@ -46,6 +45,11 @@ type TaskRunSpec struct { Generation int64 `json:"generation,omitempty"` // +optional ServiceAccount string `json:"serviceAccount,omitempty"` + // no more than one of the TaskRef and TaskSpec may be specified. + // +optional + TaskRef *TaskRef `json:"taskRef"` + //+optional + TaskSpec *TaskSpec `json:"taskSpec"` } // TaskRunInputs holds the input values that this task was invoked with. diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go index a388c98142c..5f48d623248 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go @@ -38,9 +38,15 @@ func (ts *TaskRunSpec) Validate() *apis.FieldError { return apis.ErrMissingField("spec") } - // Check for TaskRef - if ts.TaskRef.Name == "" { - return apis.ErrMissingField("spec.taskref.name") + // can't have both taskRef and taskSpec at the same time + if (ts.TaskRef != nil && ts.TaskRef.Name != "") && ts.TaskSpec != nil { + return apis.ErrDisallowedFields("spec.taskSpec") + } + + // Check that one of TaskRef and TaskSpec is present + if (ts.TaskRef == nil || (ts.TaskRef != nil && ts.TaskRef.Name == "")) && ts.TaskSpec == nil { + fields := []string{"spec.taskref.name", "spec.taskspec"} + return apis.ErrMissingField(fields...) } // Check for Trigger diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go index b6a21e0f025..61f93c3f5b5 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go @@ -20,6 +20,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/knative/pkg/apis" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -68,7 +69,7 @@ func TestTaskRun_Validate(t *testing.T) { Name: "taskname", }, Spec: TaskRunSpec{ - TaskRef: TaskRef{ + TaskRef: &TaskRef{ Name: "taskrefname", }, Trigger: TaskTrigger{ @@ -98,19 +99,19 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { { name: "invalid taskref name", spec: TaskRunSpec{ - TaskRef: TaskRef{}, + TaskRef: &TaskRef{}, Trigger: TaskTrigger{ TriggerRef: TaskTriggerRef{ Type: TaskTriggerTypeManual, }, }, }, - wantErr: apis.ErrMissingField("spec.taskref.name"), + wantErr: apis.ErrMissingField("spec.taskref.name, spec.taskspec"), }, { name: "invalid taskref type", spec: TaskRunSpec{ - TaskRef: TaskRef{ + TaskRef: &TaskRef{ Name: "taskrefname", }, Trigger: TaskTrigger{ @@ -124,7 +125,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { { name: "invalid taskref", spec: TaskRunSpec{ - TaskRef: TaskRef{ + TaskRef: &TaskRef{ Name: "taskrefname", }, Trigger: TaskTrigger{ @@ -135,7 +136,28 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }, }, wantErr: apis.ErrMissingField("spec.trigger.triggerref.name"), - }} + }, + { + name: "invalid taskref and taskspec together", + spec: TaskRunSpec{ + TaskRef: &TaskRef{ + Name: "taskrefname", + }, + TaskSpec: &TaskSpec{ + Steps: []corev1.Container{{ + Name: "mystep", + Image: "myimage", + }}, + }, + Trigger: TaskTrigger{ + TriggerRef: TaskTriggerRef{ + Type: "manual", + }, + }, + }, + wantErr: apis.ErrDisallowedFields("spec.taskSpec"), + }, + } for _, ts := range tests { t.Run(ts.name, func(t *testing.T) { @@ -155,7 +177,7 @@ func TestTaskRunSpec_Validate(t *testing.T) { { name: "task trigger run type", spec: TaskRunSpec{ - TaskRef: TaskRef{ + TaskRef: &TaskRef{ Name: "taskrefname", }, Trigger: TaskTrigger{ @@ -169,7 +191,7 @@ func TestTaskRunSpec_Validate(t *testing.T) { { name: "task trigger run type with different capitalization", spec: TaskRunSpec{ - TaskRef: TaskRef{ + TaskRef: &TaskRef{ Name: "taskrefname", }, Trigger: TaskTrigger{ @@ -180,6 +202,23 @@ func TestTaskRunSpec_Validate(t *testing.T) { }, }, }, + { + name: "taskspec without a taskRef", + spec: TaskRunSpec{ + TaskSpec: &TaskSpec{ + Steps: []corev1.Container{{ + Name: "mystep", + Image: "myimage", + }}, + }, + Trigger: TaskTrigger{ + TriggerRef: TaskTriggerRef{ + Type: "PiPeLiNeRuN", + Name: "testtrigger", + }, + }, + }, + }, } for _, ts := range tests { diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go index 03b51c719f9..8f0b8cc6c6c 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go @@ -239,7 +239,7 @@ func (c *Reconciler) createTaskRun(logger *zap.SugaredLogger, t *v1alpha1.Task, }, }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: t.Name, }, Inputs: v1alpha1.TaskRunInputs{ diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go index b524518d74c..540d1af0da6 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go @@ -226,7 +226,7 @@ func TestReconcile(t *testing.T) { }, Spec: v1alpha1.TaskRunSpec{ ServiceAccount: "test-sa", - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "unit-test-task", }, Inputs: v1alpha1.TaskRunInputs{ diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go index e9aaee85753..cd39c4d5b6a 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go @@ -148,7 +148,7 @@ func TestAddResourceToBuild(t *testing.T) { Namespace: "marshmallow", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "simpleTask", }, Inputs: v1alpha1.TaskRunInputs{ @@ -209,7 +209,58 @@ func TestAddResourceToBuild(t *testing.T) { Namespace: "marshmallow", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ + Name: "simpleTask", + }, + Inputs: v1alpha1.TaskRunInputs{ + + Resources: []v1alpha1.TaskRunResource{{ + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: "the-git", + }, + Name: "workspace", + }}, + }, + }, + }, + build: build(), + wantErr: false, + want: &buildv1alpha1.Build{ + TypeMeta: metav1.TypeMeta{ + Kind: "Build", + APIVersion: "build.knative.dev/v1alpha1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "build-from-repo", + Namespace: "marshmallow", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "pipeline.knative.dev/v1alpha1", + Kind: "TaskRun", + Name: "build-from-repo-run", + Controller: &boolTrue, + BlockOwnerDeletion: &boolTrue, + }, + }, + }, + Spec: buildv1alpha1.BuildSpec{ + Source: &buildv1alpha1.SourceSpec{ + Git: &buildv1alpha1.GitSourceSpec{ + Url: "https://github.com/grafeas/kritis", + Revision: "branch", + }, + }, + }, + }, + }, { + desc: "set revision to default value", + task: task, + taskRun: &v1alpha1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "build-from-repo-run", + Namespace: "marshmallow", + }, + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{ Name: "simpleTask", }, Inputs: v1alpha1.TaskRunInputs{ @@ -342,7 +393,7 @@ func TestAddResourceToBuild(t *testing.T) { Namespace: "marshmallow", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "build-from-repo", }, Inputs: v1alpha1.TaskRunInputs{ @@ -404,7 +455,7 @@ func TestAddResourceToBuild(t *testing.T) { Namespace: "marshmallow", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "build-from-repo", }, Inputs: v1alpha1.TaskRunInputs{ diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index 9d5b143a8d6..2726188d800 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -272,7 +272,7 @@ func TestReconcile(t *testing.T) { Namespace: "foo", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: simpleTask.Name, APIVersion: "a1", }, @@ -284,7 +284,7 @@ func TestReconcile(t *testing.T) { }, Spec: v1alpha1.TaskRunSpec{ ServiceAccount: "test-sa", - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: saTask.Name, APIVersion: "a1", }, @@ -295,7 +295,7 @@ func TestReconcile(t *testing.T) { Namespace: "foo", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: templatedTask.Name, APIVersion: "a1", }, @@ -332,7 +332,7 @@ func TestReconcile(t *testing.T) { Namespace: "foo", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: defaultTemplatedTask.Name, APIVersion: "a1", }, @@ -358,7 +358,7 @@ func TestReconcile(t *testing.T) { Namespace: "foo", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: defaultTemplatedTask.Name, APIVersion: "a1", }, @@ -707,7 +707,7 @@ func TestReconcile_InvalidTaskRuns(t *testing.T) { Namespace: "foo", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "notask", APIVersion: "a1", }, @@ -767,7 +767,7 @@ func TestReconcileBuildFetchError(t *testing.T) { Namespace: "foo", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "test-task", APIVersion: "a1", }, @@ -804,7 +804,7 @@ func TestReconcileBuildUpdateStatus(t *testing.T) { Namespace: "foo", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "test-task", APIVersion: "a1", }, diff --git a/test/cluster_resource_test.go b/test/cluster_resource_test.go index 520adbb7259..b8d648b69d3 100644 --- a/test/cluster_resource_test.go +++ b/test/cluster_resource_test.go @@ -184,7 +184,7 @@ func getClusterResourceTaskRun(namespace, name, taskName, resName string) *v1alp Name: name, }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: taskName, }, Trigger: v1alpha1.TaskTrigger{ diff --git a/test/crd.go b/test/crd.go index d1e95189fba..8bf2f55a4f1 100644 --- a/test/crd.go +++ b/test/crd.go @@ -107,7 +107,7 @@ func getHelloWorldTaskRun(namespace string) *v1alpha1.TaskRun { Name: hwTaskRunName, }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: hwTaskName, }, Trigger: v1alpha1.TaskTrigger{ diff --git a/test/helm_task_test.go b/test/helm_task_test.go index 97ca1bc3994..ba9c050b068 100644 --- a/test/helm_task_test.go +++ b/test/helm_task_test.go @@ -427,7 +427,7 @@ func removeAllHelmReleases(c *clients, t *testing.T, namespace string, logger *l Name: helmRemoveAllTaskRunName, }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: helmRemoveAllTaskName, }, Trigger: v1alpha1.TaskTrigger{ @@ -487,7 +487,7 @@ func removeHelmFromCluster(c *clients, t *testing.T, namespace string, logger *l Name: helmResetTaskRunName, }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: helmResetTaskName, }, Trigger: v1alpha1.TaskTrigger{ diff --git a/test/kaniko_task_test.go b/test/kaniko_task_test.go index c2a30e7f659..fa3e50a6e64 100644 --- a/test/kaniko_task_test.go +++ b/test/kaniko_task_test.go @@ -160,7 +160,7 @@ func getTaskRun(namespace string) *v1alpha1.TaskRun { Name: kanikoTaskRunName, }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: kanikoTaskName, }, Trigger: v1alpha1.TaskTrigger{ From f2054fa21d2244a3bb6dbb76f64a5265847ebbad Mon Sep 17 00:00:00 2001 From: Nader Ziada Date: Thu, 22 Nov 2018 17:43:54 -0500 Subject: [PATCH 2/4] Refactor TaskRun to support processing of TaskSpec --- .../v1alpha1/zz_generated.deepcopy.go | 19 +- .../v1alpha1/pipelinerun/pipelinerun_test.go | 2 +- .../taskrun/resources/input_resource_test.go | 5 +- .../taskrun/resources/input_resources.go | 15 +- pkg/reconciler/v1alpha1/taskrun/taskrun.go | 54 ++-- .../v1alpha1/taskrun/taskrun_test.go | 231 ++++++++++++++++++ pkg/reconciler/v1alpha1/taskrun/validate.go | 50 ++-- 7 files changed, 325 insertions(+), 51 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go index 05b8a5e594c..651e95d56af 100644 --- a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go @@ -1086,11 +1086,28 @@ func (in *TaskRunResource) DeepCopy() *TaskRunResource { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TaskRunSpec) DeepCopyInto(out *TaskRunSpec) { *out = *in - out.TaskRef = in.TaskRef out.Trigger = in.Trigger in.Inputs.DeepCopyInto(&out.Inputs) in.Outputs.DeepCopyInto(&out.Outputs) in.Results.DeepCopyInto(&out.Results) + if in.TaskRef != nil { + in, out := &in.TaskRef, &out.TaskRef + if *in == nil { + *out = nil + } else { + *out = new(TaskRef) + **out = **in + } + } + if in.TaskSpec != nil { + in, out := &in.TaskSpec, &out.TaskSpec + if *in == nil { + *out = nil + } else { + *out = new(TaskSpec) + (*in).DeepCopyInto(*out) + } + } return } diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go index 540d1af0da6..f3430a0dacc 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go @@ -471,7 +471,7 @@ func TestUpdateTaskRunsState(t *testing.T) { }, Spec: v1alpha1.TaskRunSpec{ ServiceAccount: "test-sa", - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "unit-test-task", }, }, diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go index cd39c4d5b6a..bcfed9b1144 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go @@ -213,10 +213,9 @@ func TestAddResourceToBuild(t *testing.T) { Name: "simpleTask", }, Inputs: v1alpha1.TaskRunInputs{ - Resources: []v1alpha1.TaskRunResource{{ ResourceRef: v1alpha1.PipelineResourceRef{ - Name: "the-git", + Name: "the-git-with-branch", }, Name: "workspace", }}, @@ -509,7 +508,7 @@ func TestAddResourceToBuild(t *testing.T) { }} { t.Run(c.desc, func(t *testing.T) { setUp() - got, err := AddInputResource(c.build, c.task, c.taskRun, pipelineResourceLister, logger) + got, err := AddInputResource(c.build, c.task.Name, c.task.Spec, c.taskRun, pipelineResourceLister, logger) if (err != nil) != c.wantErr { t.Errorf("Test: %q; NewControllerConfigFromConfigMap() error = %v, WantErr %v", c.desc, err, c.wantErr) } diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go index 2b055ff20b5..3c257f52254 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go @@ -42,26 +42,27 @@ func getBoundResource(resourceName string, boundResources []v1alpha1.TaskRunReso // AddInputResource will update the input build with the input resource from the task func AddInputResource( build *buildv1alpha1.Build, - task *v1alpha1.Task, + taskName string, + taskSpec v1alpha1.TaskSpec, taskRun *v1alpha1.TaskRun, pipelineResourceLister listers.PipelineResourceLister, logger *zap.SugaredLogger, ) (*buildv1alpha1.Build, error) { - if task.Spec.Inputs == nil { + if taskSpec.Inputs == nil { return build, nil } var gitResource *v1alpha1.GitResource - for _, input := range task.Spec.Inputs.Resources { + for _, input := range taskSpec.Inputs.Resources { boundResource, err := getBoundResource(input.Name, taskRun.Spec.Inputs.Resources) if err != nil { return nil, fmt.Errorf("Failed to get bound resource: %s", err) } - resource, err := pipelineResourceLister.PipelineResources(task.Namespace).Get(boundResource.ResourceRef.Name) + resource, err := pipelineResourceLister.PipelineResources(taskRun.Namespace).Get(boundResource.ResourceRef.Name) if err != nil { - return nil, fmt.Errorf("task %q failed to Get Pipeline Resource: %q", task.Name, boundResource) + return nil, fmt.Errorf("task %q failed to Get Pipeline Resource: %q", taskName, boundResource) } switch resource.Spec.Type { @@ -69,7 +70,7 @@ func AddInputResource( { gitResource, err = v1alpha1.NewGitResource(resource) if err != nil { - return nil, fmt.Errorf("task %q invalid Pipeline Resource: %q", task.Name, boundResource.ResourceRef.Name) + return nil, fmt.Errorf("task %q invalid Pipeline Resource: %q", taskName, boundResource.ResourceRef.Name) } gitSourceSpec := &buildv1alpha1.GitSourceSpec{ Url: gitResource.URL, @@ -84,7 +85,7 @@ func AddInputResource( case v1alpha1.PipelineResourceTypeCluster: clusterResource, err := v1alpha1.NewClusterResource(resource) if err != nil { - return nil, fmt.Errorf("task %q invalid Pipeline Resource: %q", task.Name, boundResource.ResourceRef.Name) + return nil, fmt.Errorf("task %q invalid Pipeline Resource: %q", taskName, boundResource.ResourceRef.Name) } addClusterBuildStep(build, clusterResource) } diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go index b6ebf2066a9..8f2aab68a19 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun.go @@ -223,12 +223,17 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error build, err = c.createBuild(ctx, tr, pvc.Name) if err != nil { // This Run has failed, so we need to mark it as failed and stop reconciling it + var msg string + if tr.Spec.TaskRef != nil { + msg = fmt.Sprintf("References a Task %s that doesn't exist: ", fmt.Sprintf("%s/%s", tr.Namespace, tr.Spec.TaskRef.Name)) + } else { + msg = fmt.Sprintf("References a TaskSpec with missing information: ") + } tr.Status.SetCondition(&duckv1alpha1.Condition{ - Type: duckv1alpha1.ConditionSucceeded, - Status: corev1.ConditionFalse, - Reason: ReasonCouldntGetTask, - Message: fmt.Sprintf("References a Task %s that doesn't exist: %v", - fmt.Sprintf("%s/%s", tr.Namespace, tr.Spec.TaskRef.Name), err), + Type: duckv1alpha1.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: ReasonCouldntGetTask, + Message: fmt.Sprintf("%s %v", msg, err), }) c.Recorder.Eventf(tr, corev1.EventTypeWarning, "BuildCreationFailed", "Failed to create build %q: %v", tr.Name, err) c.Logger.Errorf("Failed to create build for task %q :%v", err, tr.Name) @@ -333,19 +338,38 @@ func createPVC(kc kubernetes.Interface, tr *v1alpha1.TaskRun) (*corev1.Persisten return v, nil } +func (c *Reconciler) getTaskSpec(tr *v1alpha1.TaskRun) (v1alpha1.TaskSpec, string, error) { + taskSpec := v1alpha1.TaskSpec{} + taskName := "" + if tr.Spec.TaskRef != nil && tr.Spec.TaskRef.Name != "" { + // Get related task for taskrun + t, err := c.taskLister.Tasks(tr.Namespace).Get(tr.Spec.TaskRef.Name) + if err != nil { + return taskSpec, taskName, fmt.Errorf("error when listing tasks %v", err) + } + taskSpec = t.Spec + taskName = t.Name + } else if tr.Spec.TaskSpec != nil { + taskSpec = *tr.Spec.TaskSpec + taskName = tr.Name + } else { + return taskSpec, taskName, fmt.Errorf("TaskRun %s not providing TaskRef or TaskSpec", tr.Name) + } + return taskSpec, taskName, nil +} + // createBuild creates a build from the task, using the task's buildspec // with pvcName as a volumeMount func (c *Reconciler) createBuild(ctx context.Context, tr *v1alpha1.TaskRun, pvcName string) (*buildv1alpha1.Build, error) { - // Get related task for taskrun - t, err := c.taskLister.Tasks(tr.Namespace).Get(tr.Spec.TaskRef.Name) + ts, taskName, err := c.getTaskSpec(tr) if err != nil { - return nil, fmt.Errorf("error when listing tasks %v", err) + return nil, fmt.Errorf("taskRun %s has nil BuildSpec", tr.Name) } // TODO: Preferably use Validate on task.spec to catch validation error - bs := t.Spec.GetBuildSpec() + bs := ts.GetBuildSpec() if bs == nil { - return nil, fmt.Errorf("task %s has nil BuildSpec", t.Name) + return nil, fmt.Errorf("task %s has nil BuildSpec", taskName) } // For each step with no entrypoint set, try to populate it with the info @@ -368,7 +392,7 @@ func (c *Reconciler) createBuild(ctx context.Context, tr *v1alpha1.TaskRun, pvcN return nil, fmt.Errorf("couldn't create redirected Build: %v", err) } - build, err := resources.AddInputResource(b, t, tr, c.resourceLister, c.Logger) + build, err := resources.AddInputResource(b, taskName, ts, tr, c.resourceLister, c.Logger) if err != nil { c.Logger.Errorf("Failed to create a build for taskrun: %s due to input resource error %v", tr.Name, err) return nil, err @@ -382,18 +406,18 @@ func (c *Reconciler) createBuild(ctx context.Context, tr *v1alpha1.TaskRun, pvcN build.Spec.Volumes = append(build.Spec.Volumes, resources.GetPVCVolume(pipelineRunpvcName)) } var defaults []v1alpha1.TaskParam - if t.Spec.Inputs != nil { - defaults = append(defaults, t.Spec.Inputs.Params...) + if ts.Inputs != nil { + defaults = append(defaults, ts.Inputs.Params...) } // Apply parameter templating from the taskrun. build = resources.ApplyParameters(build, tr, defaults...) // Apply bound resource templating from the taskrun. - build, err = resources.ApplyResources(build, tr.Spec.Inputs.Resources, c.resourceLister.PipelineResources(t.Namespace), "inputs") + build, err = resources.ApplyResources(build, tr.Spec.Inputs.Resources, c.resourceLister.PipelineResources(tr.Namespace), "inputs") if err != nil { return nil, fmt.Errorf("couldnt apply input resource templating: %s", err) } - build, err = resources.ApplyResources(build, tr.Spec.Outputs.Resources, c.resourceLister.PipelineResources(t.Namespace), "outputs") + build, err = resources.ApplyResources(build, tr.Spec.Outputs.Resources, c.resourceLister.PipelineResources(tr.Namespace), "outputs") if err != nil { return nil, fmt.Errorf("couldnt apply output resource templating: %s", err) } diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index 2726188d800..be17e8bc5bb 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -410,6 +410,33 @@ func TestReconcile(t *testing.T) { }}, }, }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun-with-taskSpec", + Namespace: "foo", + }, + Spec: v1alpha1.TaskRunSpec{ + TaskSpec: &v1alpha1.TaskSpec{ + Inputs: &v1alpha1.Inputs{ + Params: []v1alpha1.TaskParam{{ + Name: "myarg", + Description: "mydesc", + Default: "mydefault", + }}, + }, + Steps: []corev1.Container{{ + Name: "mycontainer", + Image: "myimage", + Command: []string{"/mycmd"}, + Args: []string{"--my-arg=${inputs.params.myarg}"}, + }, { + Name: "myothercontainer", + Image: "myotherimage", + Command: []string{"/mycmd"}, + Args: []string{"--my-other-arg=${inputs.resources.git-resource.url}"}, + }}, + }, + }, }} d := test.Data{ @@ -1060,3 +1087,207 @@ func TestCreateRedirectedBuild(t *testing.T) { t.Errorf("services accounts do not match: %s should be %s", b.Spec.ServiceAccountName, tr.Spec.ServiceAccount) } } + +func TestTaskRunWithTaskSpec(t *testing.T) { + taskruns := []*v1alpha1.TaskRun{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun-with-taskspec", + Namespace: "foo", + }, + Spec: v1alpha1.TaskRunSpec{ + Inputs: v1alpha1.TaskRunInputs{ + Params: []v1alpha1.Param{ + { + Name: "myarg", + Value: "foo", + }, + }, + Resources: []v1alpha1.TaskRunResource{ + { + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: gitResource.Name, + APIVersion: "a1", + }, + Name: "workspace", + }, + }, + }, + TaskSpec: &v1alpha1.TaskSpec{ + Inputs: &v1alpha1.Inputs{ + Resources: []v1alpha1.TaskResource{{ + Type: "git", + Name: "workspace", + }}, + Params: []v1alpha1.TaskParam{{ + Name: "myarg", + Description: "mydesc", + Default: "mydefault", + }}, + }, + Steps: []corev1.Container{{ + Name: "mycontainer", + Image: "myimage", + Command: []string{"/mycmd"}, + Args: []string{"--my-arg=${inputs.params.myarg}"}, + }}, + }, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun-with-sources", + Namespace: "foo", + }, + Spec: v1alpha1.TaskRunSpec{ + TaskSpec: &v1alpha1.TaskSpec{ + Sources: []buildv1alpha1.SourceSpec{{ + Git: &buildv1alpha1.GitSourceSpec{ + Url: "https://foo.git", + Revision: "master", + }, + }}, + Steps: []corev1.Container{{ + Name: "mycontainer", + Image: "myimage", + Command: []string{"/mycmd"}, + Args: []string{"--my-arg=foo"}, + }}, + }, + }, + }} + d := test.Data{ + TaskRuns: taskruns, + Tasks: []*v1alpha1.Task{simpleTask, saTask, templatedTask, defaultTemplatedTask}, + PipelineResources: []*v1alpha1.PipelineResource{gitResource, imageResource}, + } + testcases := []struct { + name string + taskRun *v1alpha1.TaskRun + wantedBuildSpec buildv1alpha1.BuildSpec + }{ + { + name: "success0", + taskRun: taskruns[0], + wantedBuildSpec: buildv1alpha1.BuildSpec{ + Source: &buildv1alpha1.SourceSpec{ + Git: &buildv1alpha1.GitSourceSpec{ + Url: "https://foo.git", + Revision: "master", + }, + }, + Steps: []corev1.Container{ + entrypointCopyStep, + { + Name: "mycontainer", + Image: "myimage", + Command: []string{entrypointLocation}, + Args: []string{}, + Env: []corev1.EnvVar{ + { + Name: "ENTRYPOINT_OPTIONS", + Value: `{"args":["/mycmd","--my-arg=foo"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`, + }, + }, + VolumeMounts: []corev1.VolumeMount{toolsMount}, + }, + }, + Volumes: []corev1.Volume{ + getToolsVolume(taskruns[0].Name), + }, + }, + }, + { + name: "success1", + taskRun: taskruns[1], + wantedBuildSpec: buildv1alpha1.BuildSpec{ + Sources: []buildv1alpha1.SourceSpec{{ + Git: &buildv1alpha1.GitSourceSpec{ + Url: "https://foo.git", + Revision: "master", + }, + }}, + Steps: []corev1.Container{ + entrypointCopyStep, + { + Name: "mycontainer", + Image: "myimage", + Command: []string{entrypointLocation}, + Args: []string{}, + Env: []corev1.EnvVar{ + { + Name: "ENTRYPOINT_OPTIONS", + Value: `{"args":["/mycmd","--my-arg=foo"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`, + }, + }, + VolumeMounts: []corev1.VolumeMount{toolsMount}, + }, + }, + Volumes: []corev1.Volume{ + getToolsVolume(taskruns[1].Name), + }, + }, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + c, _, clients := test.GetTaskRunController(d) + if err := c.Reconciler.Reconcile(context.Background(), getRunName(tc.taskRun)); err != nil { + t.Errorf("expected no error. Got error %v", err) + } + if len(clients.Build.Actions()) == 0 { + t.Errorf("Expected actions to be logged in the buildclient, got none") + } + // check error + build, err := clients.Build.BuildV1alpha1().Builds(tc.taskRun.Namespace).Get(tc.taskRun.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to fetch build: %v", err) + } + if d := cmp.Diff(build.Spec, tc.wantedBuildSpec); d != "" { + t.Errorf("buildspec doesn't match, diff: %s", d) + } + + // This TaskRun is in progress now and the status should reflect that + condition := tc.taskRun.Status.GetCondition(duckv1alpha1.ConditionSucceeded) + if condition == nil || condition.Status != corev1.ConditionUnknown { + t.Errorf("Expected invalid TaskRun to have in progress status, but had %v", condition) + } + if condition != nil && condition.Reason != taskrun.ReasonRunning { + t.Errorf("Expected reason %q but was %s", taskrun.ReasonRunning, condition.Reason) + } + + namespace, name, err := cache.SplitMetaNamespaceKey(tc.taskRun.Name) + if err != nil { + t.Errorf("Invalid resource key: %v", err) + } + //Command, Args, Env, VolumeMounts + if len(clients.Kube.Actions()) == 0 { + t.Fatalf("Expected actions to be logged in the kubeclient, got none") + } + // 3. check that volume was created + pvc, err := clients.Kube.CoreV1().PersistentVolumeClaims(namespace).Get(name, metav1.GetOptions{}) + if err != nil { + t.Errorf("Failed to fetch build: %v", err) + } + + // get related TaskRun to populate expected PVC + tr, err := clients.Pipeline.PipelineV1alpha1().TaskRuns(namespace).Get(name, metav1.GetOptions{}) + if err != nil { + t.Errorf("Failed to fetch build: %v", err) + } + expectedVolume := getExpectedPVC(tr) + if d := cmp.Diff(pvc.Name, expectedVolume.Name); d != "" { + t.Errorf("pvc doesn't match, diff: %s", d) + } + if d := cmp.Diff(pvc.OwnerReferences, expectedVolume.OwnerReferences); d != "" { + t.Errorf("pvc doesn't match, diff: %s", d) + } + if d := cmp.Diff(pvc.Spec.AccessModes, expectedVolume.Spec.AccessModes); d != "" { + t.Errorf("pvc doesn't match, diff: %s", d) + } + if pvc.Spec.Resources.Requests["storage"] != expectedVolume.Spec.Resources.Requests["storage"] { + t.Errorf("pvc doesn't match, got: %v, expected: %v", + pvc.Spec.Resources.Requests["storage"], + expectedVolume.Spec.Resources.Requests["storage"]) + } + }) + } +} diff --git a/pkg/reconciler/v1alpha1/taskrun/validate.go b/pkg/reconciler/v1alpha1/taskrun/validate.go index 89ecef2c12f..e19a25c9276 100644 --- a/pkg/reconciler/v1alpha1/taskrun/validate.go +++ b/pkg/reconciler/v1alpha1/taskrun/validate.go @@ -32,35 +32,37 @@ func ValidateTaskRunAndTask(params []v1alpha1.Param, rtr *resources.ResolvedTask paramsMapping[param.Name] = "" } - if rtr.Task.Spec.Inputs != nil { - for _, inputResource := range rtr.Task.Spec.Inputs.Resources { - r, ok := rtr.Inputs[inputResource.Name] - if !ok { - return fmt.Errorf("input resource %q not provided for task %q", inputResource.Name, rtr.Task.Name) - } - // Validate the type of resource match - if inputResource.Type != r.Spec.Type { - return fmt.Errorf("input resource %q for task %q should be type %q but was %q", inputResource.Name, rtr.Task.Name, r.Spec.Type, inputResource.Type) + if rtr.Task != nil { + if rtr.Task.Spec.Inputs != nil { + for _, inputResource := range rtr.Task.Spec.Inputs.Resources { + r, ok := rtr.Inputs[inputResource.Name] + if !ok { + return fmt.Errorf("input resource %q not provided for task %q", inputResource.Name, rtr.Task.Name) + } + // Validate the type of resource match + if inputResource.Type != r.Spec.Type { + return fmt.Errorf("input resource %q for task %q should be type %q but was %q", inputResource.Name, rtr.Task.Name, r.Spec.Type, inputResource.Type) + } } - } - for _, inputResourceParam := range rtr.Task.Spec.Inputs.Params { - if _, ok := paramsMapping[inputResourceParam.Name]; !ok { - if inputResourceParam.Default == "" { - return fmt.Errorf("input param %q not provided for task %q", inputResourceParam.Name, rtr.Task.Name) + for _, inputResourceParam := range rtr.Task.Spec.Inputs.Params { + if _, ok := paramsMapping[inputResourceParam.Name]; !ok { + if inputResourceParam.Default == "" { + return fmt.Errorf("input param %q not provided for task %q", inputResourceParam.Name, rtr.Task.Name) + } } } } - } - if rtr.Task.Spec.Outputs != nil { - for _, outputResource := range rtr.Task.Spec.Outputs.Resources { - r, ok := rtr.Outputs[outputResource.Name] - if !ok { - return fmt.Errorf("output resource %q not provided for task %q", outputResource.Name, rtr.Task.Name) - } - // Validate the type of resource match - if outputResource.Type != r.Spec.Type { - return fmt.Errorf("output resource %q for task %q should be type %q but was %q", outputResource.Name, rtr.Task.Name, r.Spec.Type, outputResource.Type) + if rtr.Task.Spec.Outputs != nil { + for _, outputResource := range rtr.Task.Spec.Outputs.Resources { + r, ok := rtr.Outputs[outputResource.Name] + if !ok { + return fmt.Errorf("output resource %q not provided for task %q", outputResource.Name, rtr.Task.Name) + } + // Validate the type of resource match + if outputResource.Type != r.Spec.Type { + return fmt.Errorf("output resource %q for task %q should be type %q but was %q", outputResource.Name, rtr.Task.Name, r.Spec.Type, outputResource.Type) + } } } } From bd3d292d6a804b74bd2fc0ccef031f927f180731 Mon Sep 17 00:00:00 2001 From: Nader Ziada Date: Tue, 27 Nov 2018 10:54:50 -0500 Subject: [PATCH 3/4] updates to implementation of TaskSpec in taskRun - remove support for using sources in the taskSpec - use TaskResolver - refactor tests Signed-off-by: Nader Ziada --- docs/using.md | 40 +++ pkg/apis/pipeline/v1alpha1/task_types.go | 5 - .../pipeline/v1alpha1/taskrun_validation.go | 5 +- .../v1alpha1/taskrun_validation_test.go | 2 +- .../v1alpha1/zz_generated.deepcopy.go | 8 - .../taskrun/resources/input_resource_test.go | 4 +- .../taskrun/resources/input_resources.go | 2 +- .../taskrun/resources/taskrunresolution.go | 34 +- .../resources/taskrunresolution_test.go | 307 +++++++++++------- pkg/reconciler/v1alpha1/taskrun/taskrun.go | 33 +- .../v1alpha1/taskrun/taskrun_test.go | 262 +++------------ pkg/reconciler/v1alpha1/taskrun/validate.go | 22 +- .../v1alpha1/taskrun/validate_test.go | 147 ++++----- 13 files changed, 382 insertions(+), 489 deletions(-) diff --git a/docs/using.md b/docs/using.md index f03cc685197..04908a6e988 100644 --- a/docs/using.md +++ b/docs/using.md @@ -185,6 +185,46 @@ See [the example PipelineRun](../examples/invocations/kritis-pipeline-run.yaml). that the `Task` needs to run. 2. The `TaskRun` will also serve as a record of the history of the invocations of the `Task`. +3. Another way of running a Task is embedding the TaskSpec in the taskRun yaml as shown in the following example + +```yaml +apiVersion: pipeline.knative.dev/v1alpha1 +kind: PipelineResource +metadata: + name: go-example-git +spec: + type: git + params: + - name: url + value: https://github.com/pivotal-nader-ziada/gohelloworld +--- +apiVersion: pipeline.knative.dev/v1alpha1 +kind: TaskRun +metadata: + name: build-push-task-run-2 +spec: + trigger: + triggerRef: + type: manual + inputs: + resources: + - name: workspace + resourceRef: + name: go-example-git + taskSpec: + inputs: + resources: + - name: workspace + type: git + steps: + - name: build-and-push + image: gcr.io/kaniko-project/executor + command: + - /kaniko/executor + args: + - --destination=gcr.io/my-project/gohelloworld +``` +If the TaskSpec is provided, TaskRef is not allowed. See [the example TaskRun](../examples/invocations/run-kritis-test.yaml). diff --git a/pkg/apis/pipeline/v1alpha1/task_types.go b/pkg/apis/pipeline/v1alpha1/task_types.go index 6d5e9935515..633c8d9fc56 100644 --- a/pkg/apis/pipeline/v1alpha1/task_types.go +++ b/pkg/apis/pipeline/v1alpha1/task_types.go @@ -34,10 +34,6 @@ type TaskSpec struct { // +optional Generation int64 `json:"generation,omitempty"` - // Following fields are from build spec for a Build resource. - // Sources specifies the inputs to the build. - Sources []buildv1alpha1.SourceSpec `json:"sources,omitempty"` - // Steps are the steps of the build; each step is run sequentially with the // source mounted into /workspace. Steps []corev1.Container `json:"steps,omitempty"` @@ -151,7 +147,6 @@ type TaskList struct { func (ts *TaskSpec) GetBuildSpec() *buildv1alpha1.BuildSpec { return &buildv1alpha1.BuildSpec{ - Sources: ts.Sources, Steps: ts.Steps, Volumes: ts.Volumes, ServiceAccountName: ts.ServiceAccountName, diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go index 5f48d623248..1dae3185c05 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go @@ -40,13 +40,12 @@ func (ts *TaskRunSpec) Validate() *apis.FieldError { // can't have both taskRef and taskSpec at the same time if (ts.TaskRef != nil && ts.TaskRef.Name != "") && ts.TaskSpec != nil { - return apis.ErrDisallowedFields("spec.taskSpec") + return apis.ErrDisallowedFields("spec.taskref", "spec.taskspec") } // Check that one of TaskRef and TaskSpec is present if (ts.TaskRef == nil || (ts.TaskRef != nil && ts.TaskRef.Name == "")) && ts.TaskSpec == nil { - fields := []string{"spec.taskref.name", "spec.taskspec"} - return apis.ErrMissingField(fields...) + return apis.ErrMissingField("spec.taskref.name", "spec.taskspec") } // Check for Trigger diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go index 61f93c3f5b5..f18faf595b0 100644 --- a/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/taskrun_validation_test.go @@ -155,7 +155,7 @@ func TestTaskRunSpec_Invalidate(t *testing.T) { }, }, }, - wantErr: apis.ErrDisallowedFields("spec.taskSpec"), + wantErr: apis.ErrDisallowedFields("spec.taskspec", "spec.taskref"), }, } diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go index 651e95d56af..e531ea158c9 100644 --- a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go @@ -21,7 +21,6 @@ limitations under the License. package v1alpha1 import ( - build_v1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1" duck_v1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" core_v1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -1190,13 +1189,6 @@ func (in *TaskSpec) DeepCopyInto(out *TaskSpec) { (*in).DeepCopyInto(*out) } } - if in.Sources != nil { - in, out := &in.Sources, &out.Sources - *out = make([]build_v1alpha1.SourceSpec, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } if in.Steps != nil { in, out := &in.Steps, &out.Steps *out = make([]core_v1.Container, len(*in)) diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go index bcfed9b1144..e00134c814e 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go @@ -308,7 +308,7 @@ func TestAddResourceToBuild(t *testing.T) { Namespace: "marshmallow", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "simpleTask", }, Inputs: v1alpha1.TaskRunInputs{ @@ -508,7 +508,7 @@ func TestAddResourceToBuild(t *testing.T) { }} { t.Run(c.desc, func(t *testing.T) { setUp() - got, err := AddInputResource(c.build, c.task.Name, c.task.Spec, c.taskRun, pipelineResourceLister, logger) + got, err := AddInputResource(c.build, c.task.Name, &c.task.Spec, c.taskRun, pipelineResourceLister, logger) if (err != nil) != c.wantErr { t.Errorf("Test: %q; NewControllerConfigFromConfigMap() error = %v, WantErr %v", c.desc, err, c.wantErr) } diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go index 3c257f52254..fbd72c267db 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go @@ -43,7 +43,7 @@ func getBoundResource(resourceName string, boundResources []v1alpha1.TaskRunReso func AddInputResource( build *buildv1alpha1.Build, taskName string, - taskSpec v1alpha1.TaskSpec, + taskSpec *v1alpha1.TaskSpec, taskRun *v1alpha1.TaskRun, pipelineResourceLister listers.PipelineResourceLister, logger *zap.SugaredLogger, diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/taskrunresolution.go b/pkg/reconciler/v1alpha1/taskrun/resources/taskrunresolution.go index f81733d1bf6..c114065f0d7 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/taskrunresolution.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/taskrunresolution.go @@ -25,7 +25,8 @@ import ( // ResolvedTaskRun contains all the data that is needed to execute // the TaskRun: the TaskRun, it's Task and the PipelineResources it needs. type ResolvedTaskRun struct { - Task *v1alpha1.Task + TaskName string + 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 Inputs map[string]*v1alpha1.PipelineResource @@ -43,25 +44,26 @@ type GetTask func(string) (*v1alpha1.Task, error) // ResolveTaskRun looks up CRDs referenced by the TaskRun and returns an instance // of TaskRunResource with all of the relevant data populated. If referenced CRDs can't // be found, an error is returned. -func ResolveTaskRun(tr *v1alpha1.TaskRunSpec, getTask GetTask, gr GetResource) (*ResolvedTaskRun, error) { +func ResolveTaskRun(tr *v1alpha1.TaskRun, getTask GetTask, gr GetResource) (*ResolvedTaskRun, error) { var err error rtr := ResolvedTaskRun{ Inputs: map[string]*v1alpha1.PipelineResource{}, Outputs: map[string]*v1alpha1.PipelineResource{}, } - rtr.Task, err = getTask(tr.TaskRef.Name) + rtr.TaskSpec, rtr.TaskName, err = getTaskSpec(tr, getTask) if err != nil { - return nil, fmt.Errorf("couldn't retrieve referenced Task %q: %s", tr.TaskRef.Name, err) + return nil, fmt.Errorf("couldn't retrieve referenced Task %q: %s", tr.Spec.TaskRef.Name, err) } - for _, r := range tr.Inputs.Resources { + + for _, r := range tr.Spec.Inputs.Resources { rr, err := gr(r.ResourceRef.Name) if err != nil { return nil, fmt.Errorf("couldn't retrieve referenced input PipelineResource %q: %s", r.ResourceRef.Name, err) } rtr.Inputs[r.Name] = rr } - for _, r := range tr.Outputs.Resources { + for _, r := range tr.Spec.Outputs.Resources { rr, err := gr(r.ResourceRef.Name) if err != nil { return nil, fmt.Errorf("couldn't retrieve referenced output PipelineResource %q: %s", r.ResourceRef.Name, err) @@ -70,3 +72,23 @@ func ResolveTaskRun(tr *v1alpha1.TaskRunSpec, getTask GetTask, gr GetResource) ( } return &rtr, nil } + +func getTaskSpec(taskRun *v1alpha1.TaskRun, getTask GetTask) (*v1alpha1.TaskSpec, string, error) { + taskSpec := &v1alpha1.TaskSpec{} + taskName := "" + if taskRun.Spec.TaskRef != nil && taskRun.Spec.TaskRef.Name != "" { + // Get related task for taskrun + t, err := getTask(taskRun.Spec.TaskRef.Name) + if err != nil { + return nil, taskName, fmt.Errorf("error when listing tasks for taskRun %s %v", taskRun.Name, err) + } + taskSpec = &t.Spec + taskName = t.Name + } else if taskRun.Spec.TaskSpec != nil { + taskSpec = taskRun.Spec.TaskSpec + taskName = taskRun.Name + } else { + return taskSpec, taskName, fmt.Errorf("TaskRun %s not providing TaskRef or TaskSpec", taskRun.Name) + } + return taskSpec, taskName, nil +} diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/taskrunresolution_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/taskrunresolution_test.go index 74c6e9dce05..616656fb795 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/taskrunresolution_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/taskrunresolution_test.go @@ -21,128 +21,176 @@ import ( "testing" "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestResolveTaskRun(t *testing.T) { - tr := &v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ - Name: "orchestrate", - }, - Inputs: v1alpha1.TaskRunInputs{ - Resources: []v1alpha1.TaskRunResource{{ - Name: "repoToBuildFrom", - ResourceRef: v1alpha1.PipelineResourceRef{ - Name: "git-repo", - }, - }, { - Name: "clusterToUse", - ResourceRef: v1alpha1.PipelineResourceRef{ - Name: "k8s-cluster", - }, - }}, - }, - Outputs: v1alpha1.TaskRunOutputs{ - Resources: []v1alpha1.TaskRunResource{{ - Name: "imageToBuild", - ResourceRef: v1alpha1.PipelineResourceRef{ - Name: "image", - }, - }, { - Name: "gitRepoToUpdate", - ResourceRef: v1alpha1.PipelineResourceRef{ - Name: "another-git-repo", - }, - }}, - }, + inputs := v1alpha1.TaskRunInputs{ + Resources: []v1alpha1.TaskRunResource{{ + Name: "repoToBuildFrom", + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: "git-repo", + }, + }, { + Name: "clusterToUse", + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: "k8s-cluster", + }, + }}, + } + + outputs := v1alpha1.TaskRunOutputs{ + Resources: []v1alpha1.TaskRunResource{{ + Name: "imageToBuild", + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: "image", + }, + }, { + Name: "gitRepoToUpdate", + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: "another-git-repo", + }, + }}, } task := &v1alpha1.Task{ ObjectMeta: metav1.ObjectMeta{ Name: "orchestrate", }, + Spec: v1alpha1.TaskSpec{ + Steps: []corev1.Container{{ + Name: "step1", + }}, + }, } - gt := func(n string) (*v1alpha1.Task, error) { return task, nil } - resources := []*v1alpha1.PipelineResource{{ - ObjectMeta: metav1.ObjectMeta{ - Name: "git-repo", - }, - }, { - ObjectMeta: metav1.ObjectMeta{ - Name: "k8s-cluster", - }, - }, { - ObjectMeta: metav1.ObjectMeta{ - Name: "image", - }, + tests := []struct { + name string + taskRun *v1alpha1.TaskRun + wantTaskName string + }{{ + name: "using task ref", + taskRun: &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{ + Name: "orchestrate", + }, + Inputs: inputs, + Outputs: outputs, + }}, + wantTaskName: "orchestrate", }, { - ObjectMeta: metav1.ObjectMeta{ - Name: "another-git-repo", + name: "using embedded task spec", + taskRun: &v1alpha1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + }, + Spec: v1alpha1.TaskRunSpec{ + TaskSpec: &v1alpha1.TaskSpec{ + Steps: []corev1.Container{{ + Name: "step1", + }}, + }, + Inputs: inputs, + Outputs: outputs, + }, }, - }} - resourceIndex := 0 - gr := func(n string) (*v1alpha1.PipelineResource, error) { - r := resources[resourceIndex] - resourceIndex++ - return r, nil + wantTaskName: "foo", + }, } + for _, ts := range tests { + t.Run(ts.name, func(t *testing.T) { + gt := func(n string) (*v1alpha1.Task, error) { return task, nil } - rtr, err := ResolveTaskRun(tr, gt, gr) - if err != nil { - t.Fatalf("Did not expect error trying to resolve TaskRun: %s", err) - } + resources := []*v1alpha1.PipelineResource{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "git-repo", + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "k8s-cluster", + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "image", + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "another-git-repo", + }, + }} + resourceIndex := 0 + gr := func(n string) (*v1alpha1.PipelineResource, error) { + r := resources[resourceIndex] + resourceIndex++ + return r, nil + } - if rtr.Task == nil || rtr.Task.Name != "orchestrate" { - t.Errorf("Task not resolved, expected `orchestrate` Task but got: %v", rtr.Task) - } + rtr, err := ResolveTaskRun(ts.taskRun, gt, gr) + if err != nil { + t.Fatalf("Did not expect error trying to resolve TaskRun: %s", err) + } - if len(rtr.Inputs) == 2 { - r, ok := rtr.Inputs["repoToBuildFrom"] - if !ok { - t.Errorf("Expected value present in map for `repoToBuildFrom' but it was missing") - } else { - if r.Name != "git-repo" { - t.Errorf("Expected to use resource `git-repo` for `repoToBuildFrom` but used %s", r.Name) + if rtr.TaskName != ts.wantTaskName { + t.Errorf("Task not resolved, expected `orchestrate` Task but got: %v", rtr.TaskName) } - } - r, ok = rtr.Inputs["clusterToUse"] - if !ok { - t.Errorf("Expected value present in map for `clusterToUse' but it was missing") - } else { - if r.Name != "k8s-cluster" { - t.Errorf("Expected to use resource `k8s-cluster` for `clusterToUse` but used %s", r.Name) + if rtr.TaskSpec == nil || len(rtr.TaskSpec.Steps) != 1 || rtr.TaskSpec.Steps[0].Name != "step1" { + t.Errorf("Task not resolved, expected task's spec to be used but spec was: %v", rtr.TaskSpec) } - } - } else { - t.Errorf("Expected 2 resolved inputs but instead had: %v", rtr.Inputs) - } - - if len(rtr.Outputs) == 2 { - r, ok := rtr.Outputs["imageToBuild"] - if !ok { - t.Errorf("Expected value present in map for `imageToBuild' but it was missing") - } else { - if r.Name != "image" { - t.Errorf("Expected to use resource `image` for `imageToBuild` but used %s", r.Name) + + if len(rtr.Inputs) == 2 { + r, ok := rtr.Inputs["repoToBuildFrom"] + if !ok { + t.Errorf("Expected value present in map for `repoToBuildFrom' but it was missing") + } else { + if r.Name != "git-repo" { + t.Errorf("Expected to use resource `git-repo` for `repoToBuildFrom` but used %s", r.Name) + } + } + r, ok = rtr.Inputs["clusterToUse"] + if !ok { + t.Errorf("Expected value present in map for `clusterToUse' but it was missing") + } else { + if r.Name != "k8s-cluster" { + t.Errorf("Expected to use resource `k8s-cluster` for `clusterToUse` but used %s", r.Name) + } + } + } else { + t.Errorf("Expected 2 resolved inputs but instead had: %v", rtr.Inputs) } - } - r, ok = rtr.Outputs["gitRepoToUpdate"] - if !ok { - t.Errorf("Expected value present in map for `gitRepoToUpdate' but it was missing") - } else { - if r.Name != "another-git-repo" { - t.Errorf("Expected to use resource `another-git-repo` for `gitRepoToUpdate` but used %s", r.Name) + + if len(rtr.Outputs) == 2 { + r, ok := rtr.Outputs["imageToBuild"] + if !ok { + t.Errorf("Expected value present in map for `imageToBuild' but it was missing") + } else { + if r.Name != "image" { + t.Errorf("Expected to use resource `image` for `imageToBuild` but used %s", r.Name) + } + } + r, ok = rtr.Outputs["gitRepoToUpdate"] + if !ok { + t.Errorf("Expected value present in map for `gitRepoToUpdate' but it was missing") + } else { + if r.Name != "another-git-repo" { + t.Errorf("Expected to use resource `another-git-repo` for `gitRepoToUpdate` but used %s", r.Name) + } + } + } else { + t.Errorf("Expected 2 resolved outputs but instead had: %v", rtr.Outputs) } - } - } else { - t.Errorf("Expected 2 resolved outputs but instead had: %v", rtr.Outputs) + + }) } } + func TestResolveTaskRun_missingTask(t *testing.T) { - tr := &v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ - Name: "orchestrate", + tr := &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{ + Name: "orchestrate", + }, }, } @@ -155,17 +203,19 @@ func TestResolveTaskRun_missingTask(t *testing.T) { } } func TestResolveTaskRun_missingOutput(t *testing.T) { - tr := &v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ - Name: "orchestrate", - }, - Outputs: v1alpha1.TaskRunOutputs{ - Resources: []v1alpha1.TaskRunResource{{ - Name: "repoToUpdate", - ResourceRef: v1alpha1.PipelineResourceRef{ - Name: "another-git-repo", - }, + tr := &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{ + Name: "orchestrate", }, + Outputs: v1alpha1.TaskRunOutputs{ + Resources: []v1alpha1.TaskRunResource{{ + Name: "repoToUpdate", + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: "another-git-repo", + }, + }, + }, }, }} @@ -179,17 +229,19 @@ func TestResolveTaskRun_missingOutput(t *testing.T) { } func TestResolveTaskRun_missingInput(t *testing.T) { - tr := &v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ - Name: "orchestrate", - }, - Inputs: v1alpha1.TaskRunInputs{ - Resources: []v1alpha1.TaskRunResource{{ - Name: "repoToBuildFrom", - ResourceRef: v1alpha1.PipelineResourceRef{ - Name: "git-repo", - }, + tr := &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{ + Name: "orchestrate", }, + Inputs: v1alpha1.TaskRunInputs{ + Resources: []v1alpha1.TaskRunResource{{ + Name: "repoToBuildFrom", + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: "git-repo", + }, + }, + }, }, }} @@ -203,16 +255,24 @@ func TestResolveTaskRun_missingInput(t *testing.T) { } func TestResolveTaskRun_noResources(t *testing.T) { - tr := &v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ - Name: "orchestrate", + tr := &v1alpha1.TaskRun{ + Spec: v1alpha1.TaskRunSpec{ + TaskRef: &v1alpha1.TaskRef{ + Name: "orchestrate", + }, }, } task := &v1alpha1.Task{ ObjectMeta: metav1.ObjectMeta{ Name: "orchestrate", }, + Spec: v1alpha1.TaskSpec{ + Steps: []corev1.Container{{ + Name: "step1", + }}, + }, } + gt := func(n string) (*v1alpha1.Task, error) { return task, nil } gr := func(n string) (*v1alpha1.PipelineResource, error) { return &v1alpha1.PipelineResource{}, nil } @@ -221,8 +281,11 @@ func TestResolveTaskRun_noResources(t *testing.T) { t.Fatalf("Did not expect error trying to resolve TaskRun: %s", err) } - if rtr.Task == nil || rtr.Task.Name != "orchestrate" { - t.Errorf("Task not resolved, expected `orchestrate` Task but got: %v", rtr.Task) + if rtr.TaskName != "orchestrate" { + t.Errorf("Task not resolved, expected `orchestrate` Task but got: %v", rtr.TaskName) + } + if rtr.TaskSpec == nil || len(rtr.TaskSpec.Steps) != 1 || rtr.TaskSpec.Steps[0].Name != "step1" { + t.Errorf("Task not resolved, expected task's spec to be used but spec was: %v", rtr.TaskSpec) } if len(rtr.Inputs) != 0 { diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go index 8f2aab68a19..493df203280 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun.go @@ -182,7 +182,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { } func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error { - rtr, err := resources.ResolveTaskRun(&tr.Spec, c.taskLister.Tasks(tr.Namespace).Get, c.resourceLister.PipelineResources(tr.Namespace).Get) + rtr, err := resources.ResolveTaskRun(tr, c.taskLister.Tasks(tr.Namespace).Get, c.resourceLister.PipelineResources(tr.Namespace).Get) if err != nil { c.Logger.Error("Failed to resolve references for taskrun %s with error %v", tr.Name, err) tr.Status.SetCondition(&duckv1alpha1.Condition{ @@ -193,6 +193,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error }) return nil } + if err := ValidateTaskRunAndTask(tr.Spec.Inputs.Params, rtr); err != nil { c.Logger.Error("Failed to validate taskrun %s with error %v", tr.Name, err) tr.Status.SetCondition(&duckv1alpha1.Condition{ @@ -218,9 +219,8 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error c.Logger.Errorf("Failed to reconcile taskrun: %q, failed to get pvc %q: %v", tr.Name, tr.Name, err) return err } - // Build is not present, create build - build, err = c.createBuild(ctx, tr, pvc.Name) + build, err = c.createBuild(ctx, tr, rtr.TaskSpec, rtr.TaskName, pvc.Name) if err != nil { // This Run has failed, so we need to mark it as failed and stop reconciling it var msg string @@ -338,34 +338,9 @@ func createPVC(kc kubernetes.Interface, tr *v1alpha1.TaskRun) (*corev1.Persisten return v, nil } -func (c *Reconciler) getTaskSpec(tr *v1alpha1.TaskRun) (v1alpha1.TaskSpec, string, error) { - taskSpec := v1alpha1.TaskSpec{} - taskName := "" - if tr.Spec.TaskRef != nil && tr.Spec.TaskRef.Name != "" { - // Get related task for taskrun - t, err := c.taskLister.Tasks(tr.Namespace).Get(tr.Spec.TaskRef.Name) - if err != nil { - return taskSpec, taskName, fmt.Errorf("error when listing tasks %v", err) - } - taskSpec = t.Spec - taskName = t.Name - } else if tr.Spec.TaskSpec != nil { - taskSpec = *tr.Spec.TaskSpec - taskName = tr.Name - } else { - return taskSpec, taskName, fmt.Errorf("TaskRun %s not providing TaskRef or TaskSpec", tr.Name) - } - return taskSpec, taskName, nil -} - // createBuild creates a build from the task, using the task's buildspec // with pvcName as a volumeMount -func (c *Reconciler) createBuild(ctx context.Context, tr *v1alpha1.TaskRun, pvcName string) (*buildv1alpha1.Build, error) { - ts, taskName, err := c.getTaskSpec(tr) - if err != nil { - return nil, fmt.Errorf("taskRun %s has nil BuildSpec", tr.Name) - } - +func (c *Reconciler) createBuild(ctx context.Context, tr *v1alpha1.TaskRun, ts *v1alpha1.TaskSpec, taskName, pvcName string) (*buildv1alpha1.Build, error) { // TODO: Preferably use Validate on task.spec to catch validation error bs := ts.GetBuildSpec() if bs == nil { diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index be17e8bc5bb..b1b9a44894b 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -382,7 +382,7 @@ func TestReconcile(t *testing.T) { }}, }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: outputTask.Name, }, Inputs: v1alpha1.TaskRunInputs{ @@ -416,8 +416,29 @@ func TestReconcile(t *testing.T) { Namespace: "foo", }, Spec: v1alpha1.TaskRunSpec{ + Inputs: v1alpha1.TaskRunInputs{ + Params: []v1alpha1.Param{ + { + Name: "myarg", + Value: "foo", + }, + }, + Resources: []v1alpha1.TaskRunResource{ + { + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: gitResource.Name, + APIVersion: "a1", + }, + Name: "workspace", + }, + }, + }, TaskSpec: &v1alpha1.TaskSpec{ Inputs: &v1alpha1.Inputs{ + Resources: []v1alpha1.TaskResource{{ + Type: "git", + Name: "workspace", + }}, Params: []v1alpha1.TaskParam{{ Name: "myarg", Description: "mydesc", @@ -429,11 +450,6 @@ func TestReconcile(t *testing.T) { Image: "myimage", Command: []string{"/mycmd"}, Args: []string{"--my-arg=${inputs.params.myarg}"}, - }, { - Name: "myothercontainer", - Image: "myotherimage", - Command: []string{"/mycmd"}, - Args: []string{"--my-other-arg=${inputs.resources.git-resource.url}"}, }}, }, }, @@ -661,6 +677,36 @@ func TestReconcile(t *testing.T) { resources.GetPVCVolume(taskruns[5].GetPipelineRunPVCName()), }, }, + }, { + name: "taskrun-with-taskspec", + taskRun: taskruns[6], + wantedBuildSpec: buildv1alpha1.BuildSpec{ + Source: &buildv1alpha1.SourceSpec{ + Git: &buildv1alpha1.GitSourceSpec{ + Url: "https://foo.git", + Revision: "master", + }, + }, + Steps: []corev1.Container{ + entrypointCopyStep, + { + Name: "mycontainer", + Image: "myimage", + Command: []string{entrypointLocation}, + Args: []string{}, + Env: []corev1.EnvVar{ + { + Name: "ENTRYPOINT_OPTIONS", + Value: `{"args":["/mycmd","--my-arg=foo"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`, + }, + }, + VolumeMounts: []corev1.VolumeMount{toolsMount}, + }, + }, + Volumes: []corev1.Volume{ + getToolsVolume(taskruns[6].Name), + }, + }, }} for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { @@ -1087,207 +1133,3 @@ func TestCreateRedirectedBuild(t *testing.T) { t.Errorf("services accounts do not match: %s should be %s", b.Spec.ServiceAccountName, tr.Spec.ServiceAccount) } } - -func TestTaskRunWithTaskSpec(t *testing.T) { - taskruns := []*v1alpha1.TaskRun{{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-taskrun-with-taskspec", - Namespace: "foo", - }, - Spec: v1alpha1.TaskRunSpec{ - Inputs: v1alpha1.TaskRunInputs{ - Params: []v1alpha1.Param{ - { - Name: "myarg", - Value: "foo", - }, - }, - Resources: []v1alpha1.TaskRunResource{ - { - ResourceRef: v1alpha1.PipelineResourceRef{ - Name: gitResource.Name, - APIVersion: "a1", - }, - Name: "workspace", - }, - }, - }, - TaskSpec: &v1alpha1.TaskSpec{ - Inputs: &v1alpha1.Inputs{ - Resources: []v1alpha1.TaskResource{{ - Type: "git", - Name: "workspace", - }}, - Params: []v1alpha1.TaskParam{{ - Name: "myarg", - Description: "mydesc", - Default: "mydefault", - }}, - }, - Steps: []corev1.Container{{ - Name: "mycontainer", - Image: "myimage", - Command: []string{"/mycmd"}, - Args: []string{"--my-arg=${inputs.params.myarg}"}, - }}, - }, - }, - }, { - ObjectMeta: metav1.ObjectMeta{ - Name: "test-taskrun-with-sources", - Namespace: "foo", - }, - Spec: v1alpha1.TaskRunSpec{ - TaskSpec: &v1alpha1.TaskSpec{ - Sources: []buildv1alpha1.SourceSpec{{ - Git: &buildv1alpha1.GitSourceSpec{ - Url: "https://foo.git", - Revision: "master", - }, - }}, - Steps: []corev1.Container{{ - Name: "mycontainer", - Image: "myimage", - Command: []string{"/mycmd"}, - Args: []string{"--my-arg=foo"}, - }}, - }, - }, - }} - d := test.Data{ - TaskRuns: taskruns, - Tasks: []*v1alpha1.Task{simpleTask, saTask, templatedTask, defaultTemplatedTask}, - PipelineResources: []*v1alpha1.PipelineResource{gitResource, imageResource}, - } - testcases := []struct { - name string - taskRun *v1alpha1.TaskRun - wantedBuildSpec buildv1alpha1.BuildSpec - }{ - { - name: "success0", - taskRun: taskruns[0], - wantedBuildSpec: buildv1alpha1.BuildSpec{ - Source: &buildv1alpha1.SourceSpec{ - Git: &buildv1alpha1.GitSourceSpec{ - Url: "https://foo.git", - Revision: "master", - }, - }, - Steps: []corev1.Container{ - entrypointCopyStep, - { - Name: "mycontainer", - Image: "myimage", - Command: []string{entrypointLocation}, - Args: []string{}, - Env: []corev1.EnvVar{ - { - Name: "ENTRYPOINT_OPTIONS", - Value: `{"args":["/mycmd","--my-arg=foo"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`, - }, - }, - VolumeMounts: []corev1.VolumeMount{toolsMount}, - }, - }, - Volumes: []corev1.Volume{ - getToolsVolume(taskruns[0].Name), - }, - }, - }, - { - name: "success1", - taskRun: taskruns[1], - wantedBuildSpec: buildv1alpha1.BuildSpec{ - Sources: []buildv1alpha1.SourceSpec{{ - Git: &buildv1alpha1.GitSourceSpec{ - Url: "https://foo.git", - Revision: "master", - }, - }}, - Steps: []corev1.Container{ - entrypointCopyStep, - { - Name: "mycontainer", - Image: "myimage", - Command: []string{entrypointLocation}, - Args: []string{}, - Env: []corev1.EnvVar{ - { - Name: "ENTRYPOINT_OPTIONS", - Value: `{"args":["/mycmd","--my-arg=foo"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`, - }, - }, - VolumeMounts: []corev1.VolumeMount{toolsMount}, - }, - }, - Volumes: []corev1.Volume{ - getToolsVolume(taskruns[1].Name), - }, - }, - }, - } - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - c, _, clients := test.GetTaskRunController(d) - if err := c.Reconciler.Reconcile(context.Background(), getRunName(tc.taskRun)); err != nil { - t.Errorf("expected no error. Got error %v", err) - } - if len(clients.Build.Actions()) == 0 { - t.Errorf("Expected actions to be logged in the buildclient, got none") - } - // check error - build, err := clients.Build.BuildV1alpha1().Builds(tc.taskRun.Namespace).Get(tc.taskRun.Name, metav1.GetOptions{}) - if err != nil { - t.Fatalf("Failed to fetch build: %v", err) - } - if d := cmp.Diff(build.Spec, tc.wantedBuildSpec); d != "" { - t.Errorf("buildspec doesn't match, diff: %s", d) - } - - // This TaskRun is in progress now and the status should reflect that - condition := tc.taskRun.Status.GetCondition(duckv1alpha1.ConditionSucceeded) - if condition == nil || condition.Status != corev1.ConditionUnknown { - t.Errorf("Expected invalid TaskRun to have in progress status, but had %v", condition) - } - if condition != nil && condition.Reason != taskrun.ReasonRunning { - t.Errorf("Expected reason %q but was %s", taskrun.ReasonRunning, condition.Reason) - } - - namespace, name, err := cache.SplitMetaNamespaceKey(tc.taskRun.Name) - if err != nil { - t.Errorf("Invalid resource key: %v", err) - } - //Command, Args, Env, VolumeMounts - if len(clients.Kube.Actions()) == 0 { - t.Fatalf("Expected actions to be logged in the kubeclient, got none") - } - // 3. check that volume was created - pvc, err := clients.Kube.CoreV1().PersistentVolumeClaims(namespace).Get(name, metav1.GetOptions{}) - if err != nil { - t.Errorf("Failed to fetch build: %v", err) - } - - // get related TaskRun to populate expected PVC - tr, err := clients.Pipeline.PipelineV1alpha1().TaskRuns(namespace).Get(name, metav1.GetOptions{}) - if err != nil { - t.Errorf("Failed to fetch build: %v", err) - } - expectedVolume := getExpectedPVC(tr) - if d := cmp.Diff(pvc.Name, expectedVolume.Name); d != "" { - t.Errorf("pvc doesn't match, diff: %s", d) - } - if d := cmp.Diff(pvc.OwnerReferences, expectedVolume.OwnerReferences); d != "" { - t.Errorf("pvc doesn't match, diff: %s", d) - } - if d := cmp.Diff(pvc.Spec.AccessModes, expectedVolume.Spec.AccessModes); d != "" { - t.Errorf("pvc doesn't match, diff: %s", d) - } - if pvc.Spec.Resources.Requests["storage"] != expectedVolume.Spec.Resources.Requests["storage"] { - t.Errorf("pvc doesn't match, got: %v, expected: %v", - pvc.Spec.Resources.Requests["storage"], - expectedVolume.Spec.Resources.Requests["storage"]) - } - }) - } -} diff --git a/pkg/reconciler/v1alpha1/taskrun/validate.go b/pkg/reconciler/v1alpha1/taskrun/validate.go index e19a25c9276..75035114714 100644 --- a/pkg/reconciler/v1alpha1/taskrun/validate.go +++ b/pkg/reconciler/v1alpha1/taskrun/validate.go @@ -32,36 +32,36 @@ func ValidateTaskRunAndTask(params []v1alpha1.Param, rtr *resources.ResolvedTask paramsMapping[param.Name] = "" } - if rtr.Task != nil { - if rtr.Task.Spec.Inputs != nil { - for _, inputResource := range rtr.Task.Spec.Inputs.Resources { + if rtr.TaskSpec != nil { + if rtr.TaskSpec.Inputs != nil { + for _, inputResource := range rtr.TaskSpec.Inputs.Resources { r, ok := rtr.Inputs[inputResource.Name] if !ok { - return fmt.Errorf("input resource %q not provided for task %q", inputResource.Name, rtr.Task.Name) + return fmt.Errorf("input resource %q not provided for task %q", inputResource.Name, rtr.TaskName) } // Validate the type of resource match if inputResource.Type != r.Spec.Type { - return fmt.Errorf("input resource %q for task %q should be type %q but was %q", inputResource.Name, rtr.Task.Name, r.Spec.Type, inputResource.Type) + return fmt.Errorf("input resource %q for task %q should be type %q but was %q", inputResource.Name, rtr.TaskName, r.Spec.Type, inputResource.Type) } } - for _, inputResourceParam := range rtr.Task.Spec.Inputs.Params { + for _, inputResourceParam := range rtr.TaskSpec.Inputs.Params { if _, ok := paramsMapping[inputResourceParam.Name]; !ok { if inputResourceParam.Default == "" { - return fmt.Errorf("input param %q not provided for task %q", inputResourceParam.Name, rtr.Task.Name) + return fmt.Errorf("input param %q not provided for task %q", inputResourceParam.Name, rtr.TaskName) } } } } - if rtr.Task.Spec.Outputs != nil { - for _, outputResource := range rtr.Task.Spec.Outputs.Resources { + if rtr.TaskSpec.Outputs != nil { + for _, outputResource := range rtr.TaskSpec.Outputs.Resources { r, ok := rtr.Outputs[outputResource.Name] if !ok { - return fmt.Errorf("output resource %q not provided for task %q", outputResource.Name, rtr.Task.Name) + return fmt.Errorf("output resource %q not provided for task %q", outputResource.Name, rtr.TaskName) } // Validate the type of resource match if outputResource.Type != r.Spec.Type { - return fmt.Errorf("output resource %q for task %q should be type %q but was %q", outputResource.Name, rtr.Task.Name, r.Spec.Type, outputResource.Type) + return fmt.Errorf("output resource %q for task %q should be type %q but was %q", outputResource.Name, rtr.TaskName, r.Spec.Type, outputResource.Type) } } } diff --git a/pkg/reconciler/v1alpha1/taskrun/validate_test.go b/pkg/reconciler/v1alpha1/taskrun/validate_test.go index b84e038d2a8..b9eb2035a6c 100644 --- a/pkg/reconciler/v1alpha1/taskrun/validate_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/validate_test.go @@ -18,20 +18,15 @@ var validBuildSteps = []corev1.Container{{ func TestValidateTaskRunAndTask(t *testing.T) { rtr := &resources.ResolvedTaskRun{ - Task: &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Name: "task-valid-input", - Namespace: "foo", + TaskSpec: &v1alpha1.TaskSpec{ + Steps: validBuildSteps, + Inputs: &v1alpha1.Inputs{ + Resources: []v1alpha1.TaskResource{{ + Name: "resource-to-build", + Type: v1alpha1.PipelineResourceTypeGit, + }}, }, - Spec: v1alpha1.TaskSpec{ - Steps: validBuildSteps, - Inputs: &v1alpha1.Inputs{ - Resources: []v1alpha1.TaskResource{{ - Name: "resource-to-build", - Type: v1alpha1.PipelineResourceTypeGit, - }}, - }, - }}, + }, Inputs: map[string]*v1alpha1.PipelineResource{ "resource-to-build": { ObjectMeta: metav1.ObjectMeta{ @@ -54,21 +49,16 @@ func TestValidateTaskRunAndTask(t *testing.T) { func Test_ValidParams(t *testing.T) { rtr := &resources.ResolvedTaskRun{ - Task: &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Name: "unit-task-multiple-params", - Namespace: "foo", + TaskSpec: &v1alpha1.TaskSpec{ + Steps: validBuildSteps, + Inputs: &v1alpha1.Inputs{ + Params: []v1alpha1.TaskParam{{ + Name: "foo", + }, { + Name: "bar", + }}, }, - Spec: v1alpha1.TaskSpec{ - Steps: validBuildSteps, - Inputs: &v1alpha1.Inputs{ - Params: []v1alpha1.TaskParam{{ - Name: "foo", - }, { - Name: "bar", - }}, - }, - }}, + }, } p := []v1alpha1.Param{{ Name: "foo", @@ -84,21 +74,16 @@ func Test_ValidParams(t *testing.T) { func Test_InvalidParams(t *testing.T) { rtr := &resources.ResolvedTaskRun{ - Task: &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Name: "unit-task-multiple-params", - Namespace: "foo", + TaskSpec: &v1alpha1.TaskSpec{ + Steps: validBuildSteps, + Inputs: &v1alpha1.Inputs{ + Params: []v1alpha1.TaskParam{{ + Name: "foo", + }, { + Name: "bar", + }}, }, - Spec: v1alpha1.TaskSpec{ - Steps: validBuildSteps, - Inputs: &v1alpha1.Inputs{ - Params: []v1alpha1.TaskParam{{ - Name: "foo", - }, { - Name: "bar", - }}, - }, - }}, + }, } p := []v1alpha1.Param{{ Name: "foobar", @@ -129,74 +114,54 @@ func Test_InvalidTaskRunTask(t *testing.T) { }{{ name: "bad-inputkey", rtr: &resources.ResolvedTaskRun{ - Task: &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Name: "unit-task-wrong-input", - Namespace: "foo", - }, - Spec: v1alpha1.TaskSpec{ - Inputs: &v1alpha1.Inputs{ - Resources: []v1alpha1.TaskResource{{ - Name: "testinput", - }}, - }, + TaskSpec: &v1alpha1.TaskSpec{ + Inputs: &v1alpha1.Inputs{ + Resources: []v1alpha1.TaskResource{{ + Name: "testinput", + }}, }, }, Inputs: map[string]*v1alpha1.PipelineResource{"wrong-resource-name": r}, - }}, { + }, + }, { name: "bad-outputkey", rtr: &resources.ResolvedTaskRun{ - Task: &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Name: "unit-task-wrong-output", - Namespace: "foo", - }, - Spec: v1alpha1.TaskSpec{ - Outputs: &v1alpha1.Outputs{ - Resources: []v1alpha1.TaskResource{{ - Name: "testoutput", - }}, - }, + TaskSpec: &v1alpha1.TaskSpec{ + Outputs: &v1alpha1.Outputs{ + Resources: []v1alpha1.TaskResource{{ + Name: "testoutput", + }}, }, }, Outputs: map[string]*v1alpha1.PipelineResource{"wrong-resource-name": r}, - }}, { + }, + }, { name: "input-resource-mismatch", rtr: &resources.ResolvedTaskRun{ - Task: &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Name: "unit-task-bad-input-resourcetype", - Namespace: "foo", - }, - Spec: v1alpha1.TaskSpec{ - Inputs: &v1alpha1.Inputs{ - Resources: []v1alpha1.TaskResource{{ - Name: "testimageinput", - Type: v1alpha1.PipelineResourceTypeImage, - }}, - }, + TaskSpec: &v1alpha1.TaskSpec{ + Inputs: &v1alpha1.Inputs{ + Resources: []v1alpha1.TaskResource{{ + Name: "testimageinput", + Type: v1alpha1.PipelineResourceTypeImage, + }}, }, }, Inputs: map[string]*v1alpha1.PipelineResource{"testimageinput": r}, - }}, { + }, + }, { name: "output-resource-mismatch", rtr: &resources.ResolvedTaskRun{ - Task: &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Name: "unit-task-bad-output-resourcetype", - Namespace: "foo", - }, - Spec: v1alpha1.TaskSpec{ - Outputs: &v1alpha1.Outputs{ - Resources: []v1alpha1.TaskResource{{ - Name: "testimageoutput", - Type: v1alpha1.PipelineResourceTypeImage, - }}, - }, + TaskSpec: &v1alpha1.TaskSpec{ + Outputs: &v1alpha1.Outputs{ + Resources: []v1alpha1.TaskResource{{ + Name: "testimageoutput", + Type: v1alpha1.PipelineResourceTypeImage, + }}, }, }, Outputs: map[string]*v1alpha1.PipelineResource{"testimageoutput": r}, - }}} + }, + }} for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { From d0671a81833e72344365f8a2b01e91b59cc997c0 Mon Sep 17 00:00:00 2001 From: Nader Ziada Date: Thu, 29 Nov 2018 13:52:13 -0500 Subject: [PATCH 4/4] resolve conflicts with taskref now a pointer Signed-off-by: Nader Ziada --- pkg/reconciler/v1alpha1/taskrun/resources/build_step_test.go | 4 ++-- .../v1alpha1/taskrun/resources/input_resource_test.go | 5 +++-- pkg/reconciler/v1alpha1/taskrun/taskrun_test.go | 5 +++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/build_step_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/build_step_test.go index 7904e79da2f..aca0af59ac0 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/build_step_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/build_step_test.go @@ -34,7 +34,7 @@ func TestPostBuildSteps(t *testing.T) { Namespace: "foo", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "", APIVersion: "a1", }, @@ -129,7 +129,7 @@ func TestPreBuildSteps(t *testing.T) { Namespace: "foo", }, Spec: v1alpha1.TaskRunSpec{ - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "", APIVersion: "a1", }, diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go index e00134c814e..6984712705f 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go @@ -242,12 +242,13 @@ func TestAddResourceToBuild(t *testing.T) { }, }, Spec: buildv1alpha1.BuildSpec{ - Source: &buildv1alpha1.SourceSpec{ + Sources: []buildv1alpha1.SourceSpec{{ + Name: "the-git-with-branch", Git: &buildv1alpha1.GitSourceSpec{ Url: "https://github.com/grafeas/kritis", Revision: "branch", }, - }, + }}, }, }, }, { diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index b1b9a44894b..9633d5c0b13 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -681,12 +681,13 @@ func TestReconcile(t *testing.T) { name: "taskrun-with-taskspec", taskRun: taskruns[6], wantedBuildSpec: buildv1alpha1.BuildSpec{ - Source: &buildv1alpha1.SourceSpec{ + Sources: []buildv1alpha1.SourceSpec{{ + Name: "git-resource", Git: &buildv1alpha1.GitSourceSpec{ Url: "https://foo.git", Revision: "master", }, - }, + }}, Steps: []corev1.Container{ entrypointCopyStep, {