Skip to content

Commit

Permalink
Stop reconciling invalid PipelineRun
Browse files Browse the repository at this point in the history
If a PipelineRun references a Pipeline that uses Tasks which don't
exist, we should immediately stop trying to Reconcile it. To fix this,
the user/trigger should create a new PipelineRun after creating the
Tasks needed.
  • Loading branch information
bobcatfish committed Oct 11, 2018
1 parent 0cc7f60 commit 33036c9
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 8 deletions.
9 changes: 8 additions & 1 deletion pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,14 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
p, pr.Name,
)
if err != nil {
return fmt.Errorf("error getting Tasks for Pipeline %s, Pipeline may be invalid!: %s", p.Name, err)
if errors.IsNotFound(err) {
c.Logger.Infof("PipelineRun %s's Pipeline %s can't be Run; it contains Tasks that don't exist: %s",
fmt.Sprintf("%s/%s", p.Namespace, p.Name),
fmt.Sprintf("%s/%s", p.Namespace, pr.Name), err)
// The PipelineRun is Invalid so we want to stop trying to Reconcile it
return nil
}
return fmt.Errorf("error getting Tasks and/or TaskRuns for Pipeline %s, Pipeline may be invalid!: %s", p.Name, err)
}
prtr := resources.GetNextTask(pr.Name, state, c.Logger)
if prtr != nil {
Expand Down
37 changes: 35 additions & 2 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ func TestReconcile(t *testing.T) {
},
},
}}

ps := []*v1alpha1.Pipeline{{
ObjectMeta: metav1.ObjectMeta{
Name: "test-pipeline",
Expand Down Expand Up @@ -118,7 +117,7 @@ func TestReconcile(t *testing.T) {
}
}

func TestReconcileInvalid(t *testing.T) {
func TestReconcile_InvalidPipeline(t *testing.T) {
prs := []*v1alpha1.PipelineRun{{
ObjectMeta: metav1.ObjectMeta{
Name: "invalid-pipeline",
Expand Down Expand Up @@ -160,6 +159,40 @@ func TestReconcileInvalid(t *testing.T) {
}
}

func TestReconcile_MissingTasks(t *testing.T) {
ps := []*v1alpha1.Pipeline{{
ObjectMeta: metav1.ObjectMeta{
Name: "pipeline-missing-tasks",
Namespace: "foo",
},
Spec: v1alpha1.PipelineSpec{Tasks: []v1alpha1.PipelineTask{{
Name: "myspecialtask",
TaskRef: v1alpha1.TaskRef{Name: "sometask"},
}},
}},
}
prs := []*v1alpha1.PipelineRun{{
ObjectMeta: metav1.ObjectMeta{
Name: "pipelinerun-missing-tasks",
Namespace: "foo",
},
Spec: v1alpha1.PipelineRunSpec{
PipelineRef: v1alpha1.PipelineRef{
Name: "pipeline-missing-tasks",
},
}},
}
d := testData{
prs: prs,
ps: ps,
}
c, _, _ := getController(d)
err := c.Reconciler.Reconcile(context.Background(), "foo/pipelinerun-missing-tasks")
if err != nil {
t.Errorf("When Pipeline's Tasks can't be found, expected no error to be returned (i.e. controller should stop trying to reconcile) but got: %s", err)
}
}

func getLogMessages(logs *observer.ObservedLogs) []string {
messages := []string{}
for _, l := range logs.All() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,9 @@ func GetPipelineState(getTask GetTask, getTaskRun GetTaskRun, p *v1alpha1.Pipeli
pt := p.Spec.Tasks[i]
t, err := getTask(p.Namespace, pt.TaskRef.Name)
if err != nil {
return nil, fmt.Errorf("failed to get tasks for Pipeline %q: Error getting task %q : %s",
fmt.Sprintf("%s/%s", p.Namespace, p.Name),
fmt.Sprintf("%s/%s", p.Namespace, pt.TaskRef.Name), err)
// If the Task can't be found, it means the PipelineRun is invalid. Return the same error
// type so it can be used by the caller.
return nil, err
}
prtr := PipelineRunTaskRun{
Task: t,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package resources

import (
"fmt"
"testing"

"go.uber.org/zap"
Expand Down Expand Up @@ -255,7 +254,7 @@ func TestGetPipelineState(t *testing.T) {

func TestGetPipelineState_TaskDoesntExist(t *testing.T) {
getTask := func(namespace, name string) (*v1alpha1.Task, error) {
return nil, fmt.Errorf("Task %s doesn't exist", name)
return nil, errors.NewNotFound(v1alpha1.Resource("task"), name)
}
getTaskRun := func(namespace, name string) (*v1alpha1.TaskRun, error) {
return nil, nil
Expand All @@ -264,6 +263,9 @@ func TestGetPipelineState_TaskDoesntExist(t *testing.T) {
if err == nil {
t.Fatalf("Expected error getting non-existent Tasks for Pipeline %s but got none", p.Name)
}
if !errors.IsNotFound(err) {
t.Fatalf("Expected same error type returned by func for non-existent Task for Pipeline %s but got %s", p.Name, err)
}
}

func TestGetPipelineConditionStatus(t *testing.T) {
Expand Down

0 comments on commit 33036c9

Please sign in to comment.