diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go index c98870bd8b5..463a4eb03ad 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go @@ -48,12 +48,24 @@ func (ps *PipelineSpec) Validate() *apis.FieldError { taskNames[t.Name] = struct{}{} } - // providedBy should match other tasks. - for _, t := range ps.Tasks { + // providedBy should match future tasks + // TODO(#168) when pipelines don't just execute linearly this will need to be more sophisticated + for i, t := range ps.Tasks { if t.Resources != nil { for _, rd := range t.Resources.Inputs { for _, pb := range rd.ProvidedBy { - if _, ok := taskNames[pb]; !ok { + if i == 0 { + // First Task can't depend on anything before it (b/c there is nothing) + return apis.ErrInvalidKeyName(pb, fmt.Sprintf("spec.tasks.resources.%s", pb)) + } + found := false + // Look for previous Task that satisfies constraint + for j := i - 1; j >= 0; j-- { + if ps.Tasks[j].Name == pb { + found = true + } + } + if !found { return apis.ErrInvalidKeyName(pb, fmt.Sprintf("spec.tasks.resources.%s", pb)) } } diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go index 9c3a5bce004..200e39de648 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go @@ -40,7 +40,7 @@ func TestPipelineSpec_Validate_Error(t *testing.T) { }, }, { - name: "invalid constraint tasks", + name: "providedby task doesnt exist", fields: fields{ Tasks: []PipelineTask{{ Name: "foo", @@ -52,6 +52,21 @@ func TestPipelineSpec_Validate_Error(t *testing.T) { }}, }, }, + { + name: "providedby task is afterward", + fields: fields{ + Tasks: []PipelineTask{{ + Name: "foo", + Resources: &PipelineTaskResources{ + Inputs: []PipelineTaskInputResource{{ + ProvidedBy: []string{"bar"}, + }}, + }, + }, { + Name: "bar", + }}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -89,14 +104,14 @@ func TestPipelineSpec_Validate_Valid(t *testing.T) { name: "valid constraint tasks", fields: fields{ Tasks: []PipelineTask{{ + Name: "bar", + }, { Name: "foo", Resources: &PipelineTaskResources{ Inputs: []PipelineTaskInputResource{{ ProvidedBy: []string{"bar"}, }}, }, - }, { - Name: "bar", }}, }, }, diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go index 3047224a72a..f5a8a9fea19 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go @@ -60,7 +60,7 @@ func GetNextTask(prName string, state []*ResolvedPipelineRunTask, logger *zap.Su logger.Infof("TaskRun %s is still running so we shouldn't start more for PipelineRun %s", prtr.TaskRunName, prName) return nil } - } else if canTaskRun(prtr.PipelineTask, state) { + } else { logger.Infof("TaskRun %s should be started for PipelineRun %s", prtr.TaskRunName, prName) return prtr } @@ -69,37 +69,6 @@ func GetNextTask(prName string, state []*ResolvedPipelineRunTask, logger *zap.Su return nil } -func canTaskRun(pt *v1alpha1.PipelineTask, state []*ResolvedPipelineRunTask) bool { - // Check if Task can run now. Go through all the input constraints - if pt.Resources != nil { - for _, rd := range pt.Resources.Inputs { - if len(rd.ProvidedBy) > 0 { - for _, constrainingTaskName := range rd.ProvidedBy { - for _, prtr := range state { - // the constraining task must have a successful task run to allow this task to run - if prtr.PipelineTask.Name == constrainingTaskName { - if prtr.TaskRun == nil { - return false - } - c := prtr.TaskRun.Status.GetCondition(duckv1alpha1.ConditionSucceeded) - if c == nil { - return false - } - switch c.Status { - case corev1.ConditionFalse: - return false - case corev1.ConditionUnknown: - return false - } - } - } - } - } - } - } - return true -} - // ResolvedPipelineRunTask contains a Task and its associated TaskRun, if it // exists. TaskRun can be nil to represent there being no TaskRun. type ResolvedPipelineRunTask struct { diff --git a/pkg/reconciler/v1alpha1/pipelinerun/resources/providedby_test.go b/pkg/reconciler/v1alpha1/pipelinerun/resources/providedby_test.go deleted file mode 100644 index a7a52a7385f..00000000000 --- a/pkg/reconciler/v1alpha1/pipelinerun/resources/providedby_test.go +++ /dev/null @@ -1,153 +0,0 @@ -/* -Copyright 2018 The Knative Authors - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package resources - -import ( - "testing" - - "github.com/google/go-cmp/cmp" - - "github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -var mytask1 = &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace", - Name: "mytask1", - }, - Spec: v1alpha1.TaskSpec{ - Inputs: &v1alpha1.Inputs{ - Resources: []v1alpha1.TaskResource{{ - Name: "myresource1", - Type: v1alpha1.PipelineResourceTypeGit, - }}, - }, - }, -} - -var mytask2 = &v1alpha1.Task{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace", - Name: "mytask2", - }, - Spec: v1alpha1.TaskSpec{ - Inputs: &v1alpha1.Inputs{ - Resources: []v1alpha1.TaskResource{{ - Name: "myresource1", - Type: v1alpha1.PipelineResourceTypeGit, - }}, - }, - }, -} - -var mypipelinetasks = []v1alpha1.PipelineTask{{ - Name: "mypipelinetask1", - TaskRef: v1alpha1.TaskRef{Name: "mytask1"}, -}, { - Name: "mypipelinetask2", - TaskRef: v1alpha1.TaskRef{Name: "mytask2"}, - Resources: &v1alpha1.PipelineTaskResources{ - Inputs: []v1alpha1.PipelineTaskInputResource{{ - Name: "myresource1", - ProvidedBy: []string{"mypipelinetask1"}, - }}, - }, -}} - -var mytaskruns = []v1alpha1.TaskRun{{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace", - Name: "pipelinerun-mytask1", - }, - Spec: v1alpha1.TaskRunSpec{}, -}, { - ObjectMeta: metav1.ObjectMeta{ - Namespace: "namespace", - Name: "pipelinerun-mytask2", - }, - Spec: v1alpha1.TaskRunSpec{}, -}} - -func TestCanTaskRun(t *testing.T) { - tcs := []struct { - name string - state []*ResolvedPipelineRunTask - canSecondTaskRun bool - }{ - { - name: "first-task-not-started", - state: []*ResolvedPipelineRunTask{{ - PipelineTask: &mypipelinetasks[0], - TaskRunName: "pipelinerun-mytask1", - TaskRun: nil, - }, { - PipelineTask: &mypipelinetasks[1], - TaskRunName: "pipelinerun-mytask2", - TaskRun: nil, - }}, - canSecondTaskRun: false, - }, - { - name: "first-task-running", - state: []*ResolvedPipelineRunTask{{ - PipelineTask: &mypipelinetasks[0], - TaskRunName: "pipelinerun-mytask1", - TaskRun: makeStarted(mytaskruns[0]), - }, { - PipelineTask: &mypipelinetasks[1], - TaskRunName: "pipelinerun-mytask2", - TaskRun: nil, - }}, - canSecondTaskRun: false, - }, - { - name: "first-task-failed", - state: []*ResolvedPipelineRunTask{{ - PipelineTask: &mypipelinetasks[0], - TaskRunName: "pipelinerun-mytask1", - TaskRun: makeFailed(mytaskruns[0]), - }, { - PipelineTask: &mypipelinetasks[1], - TaskRunName: "pipelinerun-mytask2", - TaskRun: nil, - }}, - canSecondTaskRun: false, - }, - { - name: "first-task-finished", - state: []*ResolvedPipelineRunTask{{ - PipelineTask: &mypipelinetasks[0], - TaskRunName: "pipelinerun-mytask1", - TaskRun: makeSucceeded(mytaskruns[0]), - }, { - PipelineTask: &mypipelinetasks[1], - TaskRunName: "pipelinerun-mytask2", - TaskRun: nil, - }}, - canSecondTaskRun: true, - }, - } - for _, tc := range tcs { - t.Run(tc.name, func(t *testing.T) { - cantaskrun := canTaskRun(&mypipelinetasks[1], tc.state) - if d := cmp.Diff(cantaskrun, tc.canSecondTaskRun); d != "" { - t.Fatalf("Expected second task availability to run should be %t, but different state returned: %s", tc.canSecondTaskRun, d) - } - }) - } -}