Skip to content

Commit

Permalink
Add PipelineRun integration test and refactor unit tests
Browse files Browse the repository at this point in the history
Added the beginnings of a skeleton of an integration test for
PipelineRun for #61. In doing this I realized that I couldn't create
PipelineParam objects with the clients b/c the plural name was wrong -
it needs to be `pipelineparamses` like I'm Smeagol. I tried to work
around this and stumbled on
kubernetes/code-generator#53 so it doesn't
seem to be possible without changing the code-generator code :(

Meanwhile refactored the existing controller code to break some code out
of the Reconciler, so it can be instantiated without an entire
controller (instead depends only on the objects it needs). The decision about
what TaskRuns to create has been separated from the logic to retrieve
existing ones, and the logic to create them. (`GetTasks`,
`getNextPipelineRunTaskRun`)

The tests were refactored such that the success cases are in separate
tests from the failure cases, so the table driven tests dont have to
handle both and it's more clear what the tests are doing.
  • Loading branch information
bobcatfish authored and knative-prow-robot committed Oct 10, 2018
1 parent 62713cc commit ecf71f6
Show file tree
Hide file tree
Showing 14 changed files with 631 additions and 263 deletions.
4 changes: 2 additions & 2 deletions config/300-pipelineparams.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ metadata:
creationTimestamp: null
labels:
controller-tools.k8s.io: "1.0"
name: pipelineparams.pipeline.knative.dev
name: pipelineparamses.pipeline.knative.dev
spec:
group: pipeline.knative.dev
names:
kind: PipelineParams
plural: pipelineparams
plural: pipelineparamses
scope: Namespaced
version: v1alpha1
status:
Expand Down
39 changes: 39 additions & 0 deletions pkg/reconciler/v1alpha1/pipeline/tasks.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
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 pipeline

import (
"fmt"

"github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1"
listers "github.com/knative/build-pipeline/pkg/client/listers/pipeline/v1alpha1"
)

// GetTasks retrieves all Tasks instances which the pipeline p references, using
// lister l. If it is unable to retrieve an instance of a references Task, it will return
// an error, otherwise it returns a map from the name of the Task in the Pipeline to the
// name of the Task object itself.
func GetTasks(l listers.TaskLister, p *v1alpha1.Pipeline) (map[string]*v1alpha1.Task, error) {
tasks := map[string]*v1alpha1.Task{}
for _, pt := range p.Spec.Tasks {
t, err := l.Tasks(p.Namespace).Get(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)
}
tasks[pt.Name] = t
}
return tasks, nil
}
79 changes: 79 additions & 0 deletions pkg/reconciler/v1alpha1/pipeline/tasks_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
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 pipeline

import (
"reflect"
"testing"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/knative/build-pipeline/pkg/apis/pipeline/v1alpha1"
fakepipelineclientset "github.com/knative/build-pipeline/pkg/client/clientset/versioned/fake"
informers "github.com/knative/build-pipeline/pkg/client/informers/externalversions"
informersv1alpha1 "github.com/knative/build-pipeline/pkg/client/informers/externalversions/pipeline/v1alpha1"
)

func getFakeInformer() informersv1alpha1.TaskInformer {
pipelineClient := fakepipelineclientset.NewSimpleClientset()
sharedInfomer := informers.NewSharedInformerFactory(pipelineClient, 0)
return sharedInfomer.Pipeline().V1alpha1().Tasks()
}

var p = &v1alpha1.Pipeline{
ObjectMeta: metav1.ObjectMeta{
Namespace: "namespace",
Name: "pipeline",
},
Spec: v1alpha1.PipelineSpec{
Tasks: []v1alpha1.PipelineTask{{
Name: "mytask1",
TaskRef: v1alpha1.TaskRef{Name: "task"},
}, {
Name: "mytask2",
TaskRef: v1alpha1.TaskRef{Name: "task"},
}},
},
}

func TestGetTasks(t *testing.T) {
task := &v1alpha1.Task{
ObjectMeta: metav1.ObjectMeta{
Namespace: "namespace",
Name: "task",
},
Spec: v1alpha1.TaskSpec{},
}
i := getFakeInformer()
i.Informer().GetIndexer().Add(task)

tasks, err := GetTasks(i.Lister(), p)
if err != nil {
t.Fatalf("Error getting tasks for fake pipeline %s: %s", p.ObjectMeta.Name, err)
}
expectedTasks := map[string]*v1alpha1.Task{
"mytask1": task,
"mytask2": task,
}
if !reflect.DeepEqual(tasks, expectedTasks) {
t.Fatalf("Expected to get map of tasks %v but got %v instead", expectedTasks, tasks)
}
}

func TestGetTasksDoesntExist(t *testing.T) {
i := getFakeInformer()
_, err := GetTasks(i.Lister(), p)
if err == nil {
t.Fatalf("Expected error getting non-existent Tasks for Pipeline %s but got none", p.ObjectMeta.Name)
}
}
66 changes: 15 additions & 51 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,13 @@ import (
"go.uber.org/zap"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/tools/cache"

informers "github.com/knative/build-pipeline/pkg/client/informers/externalversions/pipeline/v1alpha1"
listers "github.com/knative/build-pipeline/pkg/client/listers/pipeline/v1alpha1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"github.com/knative/build-pipeline/pkg/reconciler/v1alpha1/pipeline"
)

const (
Expand Down Expand Up @@ -141,58 +142,26 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
fmt.Sprintf("%s/%s", pr.Namespace, pr.Spec.PipelineRef.Name))
return nil
}
if err := c.createPipelineRunTaskRuns(p, pr); err != nil {
return err
}
// TODO fetch the taskruns status for this pipeline run.

// TODO check status of tasks and update status of PipelineRuns

return nil
}

// createPipelineRunTaskRuns goes through all the `Task` in the `PipelineSpec`
// and creates a `TaskRun` for the first `Task` for which no corresponding
// `TaskRun` exist.
func (c *Reconciler) createPipelineRunTaskRuns(p *v1alpha1.Pipeline, pr *v1alpha1.PipelineRun) error {
taskRefMap, err := c.getTaskMap(p)
pipelineTasks, err := pipeline.GetTasks(c.taskLister, p)
if err != nil {
return errors.NewBadRequest(fmt.Sprintf("pipeline %q is not valid due to %v", p.Name, err))
return fmt.Errorf("error getting map of created TaskRuns for Pipeline %s: %s", p.Name, err)
}
for _, pt := range p.Spec.Tasks {
trName := getTaskRunName(pr.Name, &pt)
_, listErr := c.taskRunLister.TaskRuns(p.Namespace).Get(trName)
if errors.IsNotFound(listErr) && c.canTaskRun(&pt) {
if _, err := c.createTaskRun(taskRefMap[pt.Name], trName, pr); err != nil {
return err
}
} else if err != nil {
return err
}
pipelineTaskName, trName, err := getNextPipelineRunTaskRun(c.taskRunLister, p, pr.Name)
if err != nil {
return fmt.Errorf("error getting next TaskRun to create for PipelineRun %s: %s", pr.Name, err)
}
return nil
}

// getTaskMap returns a map of task in pipeline spec to `Task` mentioned in taskRef.
func (c *Reconciler) getTaskMap(p *v1alpha1.Pipeline) (map[string]*v1alpha1.Task, error) {
tMap := make(map[string]*v1alpha1.Task, 0)
for _, pt := range p.Spec.Tasks {
tObj, err := c.taskLister.Tasks(p.Namespace).Get(pt.TaskRef.Name)
if pipelineTaskName != "" {
_, err = c.createTaskRun(pipelineTasks[pipelineTaskName], trName, pr)
if err != nil {
c.Logger.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)
return nil, err
return fmt.Errorf("error creating TaskRun called %s for PipelineTask %s from PipelineRun %s: %s", trName, pipelineTaskName, pr.Name, err)
}
tMap[pt.Name] = tObj
}
return tMap, nil
}

func (c *Reconciler) canTaskRun(pt *v1alpha1.PipelineTask) bool {
// Check if Task can run now. Go through all the input constraints and see if
// the upstream tasks have completed successfully and inputs are available.
return true
// TODO fetch the taskruns status for this pipeline run.

// TODO check status of tasks and update status of PipelineRuns

return nil
}

func (c *Reconciler) createTaskRun(t *v1alpha1.Task, trName string, pr *v1alpha1.PipelineRun) (*v1alpha1.TaskRun, error) {
Expand All @@ -209,11 +178,6 @@ func (c *Reconciler) createTaskRun(t *v1alpha1.Task, trName string, pr *v1alpha1
return c.PipelineClientSet.PipelineV1alpha1().TaskRuns(t.Namespace).Create(tr)
}

// getTaskRunName should return a uniquie name for a `TaskRun`.
func getTaskRunName(prName string, pt *v1alpha1.PipelineTask) string {
return fmt.Sprintf("%s-%s", prName, pt.Name)
}

func (c *Reconciler) updateStatus(pr *v1alpha1.PipelineRun) (*v1alpha1.PipelineRun, error) {
newPr, err := c.pipelineRunLister.PipelineRuns(pr.Namespace).Get(pr.Name)
if err != nil {
Expand Down
Loading

0 comments on commit ecf71f6

Please sign in to comment.