Skip to content

Commit

Permalink
Handle case where no resources are bound
Browse files Browse the repository at this point in the history
This was actually panicing b/c the ResolvedTaskResources was `nil` :O
Caught by an end to end test!
  • Loading branch information
bobcatfish authored and knative-prow-robot committed Dec 4, 2018
1 parent aa8ab5a commit f70944e
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 7 deletions.
2 changes: 1 addition & 1 deletion pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
}
err = resources.ResolveTaskRuns(c.taskRunLister.TaskRuns(pr.Namespace).Get, pipelineState)
if err != nil {
return fmt.Errorf("error getting TaskRunss for Pipeline %s: %s", p.Name, err)
return fmt.Errorf("error getting TaskRuns for Pipeline %s: %s", p.Name, err)
}

serviceAccount := pr.Spec.ServiceAccount
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,15 +109,15 @@ type ResolvedPipelineRunTask struct {
// GetTaskRun is a function that will retrieve the TaskRun name.
type GetTaskRun func(name string) (*v1alpha1.TaskRun, error)

func getPipelineRunTask(pipelineTaskName string, pr *v1alpha1.PipelineRun) *v1alpha1.PipelineTaskResource {
func getPipelineRunTaskResources(pipelineTaskName string, pr *v1alpha1.PipelineRun) ([]v1alpha1.TaskResourceBinding, []v1alpha1.TaskResourceBinding) {
for _, ptr := range pr.Spec.PipelineTaskResources {
if ptr.Name == pipelineTaskName {
return &ptr
return ptr.Inputs, ptr.Outputs
}
}
// It's not an error to not find corresponding resources, because a Task may not need resources.
// Validation should occur later once resolution is done.
return nil
return []v1alpha1.TaskResourceBinding{}, []v1alpha1.TaskResourceBinding{}
}

// ResolvePipelineRun retrieves all Tasks instances which the pipeline p references, getting
Expand All @@ -142,9 +142,9 @@ func ResolvePipelineRun(getTask resources.GetTask, getResource resources.GetReso
return nil, err
}

// Get all the resources that this task will be using
ptr := getPipelineRunTask(pt.Name, pr)
rtr, err := resources.ResolveTaskResources(&t.Spec, t.Name, ptr.Inputs, ptr.Outputs, getResource)
// Get all the resources that this task will be using, if any
inputs, outputs := getPipelineRunTaskResources(pt.Name, pr)
rtr, err := resources.ResolveTaskResources(&t.Spec, t.Name, inputs, outputs, getResource)
if err != nil {
return nil, fmt.Errorf("couldn't resolve task resources for task %q: %v", t.Name, err)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,44 @@ func TestResolvePipelineRun(t *testing.T) {
}
}

func TestResolvePipelineRun_PipelineTaskHasNoResources(t *testing.T) {
pr := &v1alpha1.PipelineRun{
ObjectMeta: metav1.ObjectMeta{
Name: "pipelinerun",
},
Spec: v1alpha1.PipelineRunSpec{
PipelineRef: v1alpha1.PipelineRef{
Name: "pipeline",
},
// We don't bind any Resources here
},
}
getTask := func(name string) (*v1alpha1.Task, error) {
return task, nil
}
getResource := func(name string) (*v1alpha1.PipelineResource, error) { return nil, fmt.Errorf("should not get called") }

pipelineState, err := ResolvePipelineRun(getTask, getResource, p, pr)
if err != nil {
t.Fatalf("Did not expect error when resolving PipelineRun without Resources: %v", err)
}
if len(pipelineState) != 2 {
t.Fatalf("Expected only 2 resolved PipelineTasks but got %d", len(pipelineState))
}
expectedTaskResources := &resources.ResolvedTaskResources{
TaskName: task.Name,
TaskSpec: &task.Spec,
Inputs: map[string]*v1alpha1.PipelineResource{},
Outputs: map[string]*v1alpha1.PipelineResource{},
}
if d := cmp.Diff(pipelineState[0].ResolvedTaskResources, expectedTaskResources, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" {
t.Fatalf("Expected resources where only Tasks were resolved but but actual differed: %s", d)
}
if d := cmp.Diff(pipelineState[1].ResolvedTaskResources, expectedTaskResources, cmpopts.IgnoreUnexported(v1alpha1.TaskRunSpec{})); d != "" {
t.Fatalf("Expected resources where only Tasks were resolved but but actual differed: %s", d)
}
}

func TestResolvePipelineRun_TaskDoesntExist(t *testing.T) {
pr := &v1alpha1.PipelineRun{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -335,6 +373,7 @@ func TestResolvePipelineRun_TaskDoesntExist(t *testing.T) {
},
},
}
// Return an error when the Task is retrieved, as if it didn't exist
getTask := func(name string) (*v1alpha1.Task, error) {
return nil, errors.NewNotFound(v1alpha1.Resource("task"), name)
}
Expand Down

0 comments on commit f70944e

Please sign in to comment.