Skip to content

Commit

Permalink
Validate that extra params + resources aren't provided
Browse files Browse the repository at this point in the history
While working on creating an end to end test for tektoncd#168, @dlorenc and I
had a very hard time determining why our pipeline wasn't working as
expected - it turned out that we were using the wrong resource name in
our `providedBy` clauses.

This is the first of a couple follow up commits to add more validation:
this one makes sure that extra resources are not provided (e.g. if you
typo-ed a param with a default value, you may never know your param was
being ignored).

(Shout out to @vdemeester - I updated the reconciler tests both before
and after his test refactoring, and it was nearly impossible to
understand the tests before his builders were added: the builders made
it waaaaay easier! 🙏

Follow up on #124
  • Loading branch information
bobcatfish committed Dec 19, 2018
1 parent 24449cf commit 1bb816f
Show file tree
Hide file tree
Showing 10 changed files with 397 additions and 251 deletions.
4 changes: 2 additions & 2 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func TestReconcile(t *testing.T) {
tb.Task("unit-test-task", "foo", tb.TaskSpec(
tb.TaskInputs(
tb.InputsResource("workspace", v1alpha1.PipelineResourceTypeGit),
tb.InputsParam("foo"), tb.InputsParam("bar"),
tb.InputsParam("foo"), tb.InputsParam("bar"), tb.InputsParam("templatedparam"),
),
tb.TaskOutputs(
tb.OutputsResource("image-to-use", v1alpha1.PipelineResourceTypeImage),
Expand All @@ -120,7 +120,7 @@ func TestReconcile(t *testing.T) {
tb.ClusterTask("unit-test-cluster-task", tb.ClusterTaskSpec(
tb.TaskInputs(
tb.InputsResource("workspace", v1alpha1.PipelineResourceTypeGit),
tb.InputsParam("foo"), tb.InputsParam("bar"),
tb.InputsParam("foo"), tb.InputsParam("bar"), tb.InputsParam("templatedparam"),
),
tb.TaskOutputs(
tb.OutputsResource("image-to-use", v1alpha1.PipelineResourceTypeImage),
Expand Down
35 changes: 35 additions & 0 deletions pkg/reconciler/v1alpha1/taskrun/list/diff.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
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 extress or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package list

// DiffLeft will return all strings which are in the left list of stirngs but
// not in the right.
func DiffLeft(left, right []string) []string {
extra := []string{}
for _, s := range left {
found := false
for _, s2 := range right {
if s == s2 {
found = true
}
}
if !found {
extra = append(extra, s)
}
}
return extra
}
52 changes: 52 additions & 0 deletions pkg/reconciler/v1alpha1/taskrun/list/diff_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
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 extress or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package list

import (
"reflect"
"testing"
)

func TestDiffLeft_same(t *testing.T) {
left := []string{"elsa", "anna", "olaf", "kristoff"}
right := []string{"elsa", "anna", "olaf", "kristoff"}
extraLeft := DiffLeft(left, right)

if !reflect.DeepEqual(extraLeft, []string{}) {
t.Errorf("Didn't expect extra strings in left list but got %v", extraLeft)
}
}

func TestDiffLeft_extraLeft(t *testing.T) {
left := []string{"elsa", "anna", "olaf", "kristoff", "hans"}
right := []string{"elsa", "anna", "olaf", "kristoff"}
extraLeft := DiffLeft(left, right)

if !reflect.DeepEqual(extraLeft, []string{"hans"}) {
t.Errorf("Should have identified extra string in left list but got %v", extraLeft)
}
}

func TestDiffLeft_extraRight(t *testing.T) {
left := []string{"elsa", "anna", "olaf", "kristoff"}
right := []string{"elsa", "anna", "olaf", "kristoff", "hans"}
extraLeft := DiffLeft(left, right)

if !reflect.DeepEqual(extraLeft, []string{}) {
t.Errorf("Shouldn't have noticed extra item in right list but got %v", extraLeft)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestResolveTaskRun(t *testing.T) {
}}

taskName := "orchestrate"
taskSpec := &v1alpha1.TaskSpec{
taskSpec := v1alpha1.TaskSpec{
Steps: []corev1.Container{{
Name: "step1",
}}}
Expand Down Expand Up @@ -80,7 +80,7 @@ func TestResolveTaskRun(t *testing.T) {
return r, nil
}

rtr, err := ResolveTaskResources(taskSpec, taskName, inputs, outputs, gr)
rtr, err := ResolveTaskResources(&taskSpec, taskName, inputs, outputs, gr)
if err != nil {
t.Fatalf("Did not expect error trying to resolve TaskRun: %s", err)
}
Expand Down Expand Up @@ -166,14 +166,14 @@ func TestResolveTaskRun_missingInput(t *testing.T) {
}

func TestResolveTaskRun_noResources(t *testing.T) {
taskSpec := &v1alpha1.TaskSpec{
taskSpec := v1alpha1.TaskSpec{
Steps: []corev1.Container{{
Name: "step1",
}}}

gr := func(n string) (*v1alpha1.PipelineResource, error) { return &v1alpha1.PipelineResource{}, nil }

rtr, err := ResolveTaskResources(taskSpec, "orchestrate", []v1alpha1.TaskResourceBinding{}, []v1alpha1.TaskResourceBinding{}, gr)
rtr, err := ResolveTaskResources(&taskSpec, "orchestrate", []v1alpha1.TaskResourceBinding{}, []v1alpha1.TaskResourceBinding{}, gr)
if err != nil {
t.Fatalf("Did not expect error trying to resolve TaskRun: %s", err)
}
Expand Down
91 changes: 21 additions & 70 deletions pkg/reconciler/v1alpha1/taskrun/taskrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,25 +79,22 @@ var (
saTask = tb.Task("test-with-sa", "foo", tb.TaskSpec(tb.Step("sa-step", "foo", tb.Command("/mycmd"))))

templatedTask = tb.Task("test-task-with-templating", "foo", tb.TaskSpec(
tb.TaskInputs(tb.InputsResource("workspace", v1alpha1.PipelineResourceTypeGit)),
tb.TaskInputs(
tb.InputsResource("workspace", v1alpha1.PipelineResourceTypeGit),
tb.InputsParam("myarg"), tb.InputsParam("myarghasdefault", tb.ParamDefault("dont see me")),
tb.InputsParam("myarghasdefault2", tb.ParamDefault("thedefault")),
),
tb.TaskOutputs(tb.OutputsResource("myimage", v1alpha1.PipelineResourceTypeImage)),
tb.Step("mycontainer", "myimage", tb.Command("/mycmd"), tb.Args(
"--my-arg=${inputs.params.myarg}",
"--my-arg-with-default=${inputs.params.myarghasdefault}",
"--my-arg-with-default2=${inputs.params.myarghasdefault2}",
"--my-additional-arg=${outputs.resources.myimage.url}",
)),
tb.Step("myothercontainer", "myotherimage", tb.Command("/mycmd"), tb.Args(
"--my-other-arg=${inputs.resources.workspace.url}",
)),
))
defaultTemplatedTask = tb.Task("test-task-with-default-templating", "foo", tb.TaskSpec(
tb.TaskInputs(tb.InputsParam("myarg", tb.ParamDefault("mydefault"))),
tb.Step("mycontainer", "myimage", tb.Command("/mycmd"), tb.Args(
"--my-arg=${inputs.params.myarg}",
)),
tb.Step("myothercontainer", "myotherimage", tb.Command("/mycmd"), tb.Args(
"--my-other-arg=${inputs.resources.git-resource.url}",
)),
))

gitResource = tb.PipelineResource("git-resource", "foo", tb.PipelineResourceSpec(
v1alpha1.PipelineResourceTypeGit, tb.PipelineResourceSpecParam("URL", "https://foo.git"),
Expand Down Expand Up @@ -185,23 +182,11 @@ func TestReconcile(t *testing.T) {
tb.TaskRunTaskRef(templatedTask.Name, tb.TaskRefAPIVersion("a1")),
tb.TaskRunInputs(
tb.TaskRunInputsParam("myarg", "foo"),
tb.TaskRunInputsParam("myarghasdefault", "bar"),
tb.TaskRunInputsResource("workspace", tb.ResourceBindingRef(gitResource.Name)),
),
tb.TaskRunOutputs(tb.TaskRunOutputsResource("myimage", tb.ResourceBindingRef("image-resource"))),
))
taskRunOverrivdesDefaultTemplating := tb.TaskRun("test-taskrun-overrides-default-templating", "foo", tb.TaskRunSpec(
tb.TaskRunTaskRef(defaultTemplatedTask.Name, tb.TaskRefAPIVersion("a1")),
tb.TaskRunInputs(
tb.TaskRunInputsParam("myarg", "foo"),
tb.TaskRunInputsResource(gitResource.Name),
),
))
taskRunDefaultTemplating := tb.TaskRun("test-taskrun-default-templating", "foo", tb.TaskRunSpec(
tb.TaskRunTaskRef(defaultTemplatedTask.Name, tb.TaskRefAPIVersion("a1")),
tb.TaskRunInputs(
tb.TaskRunInputsResource(gitResource.Name),
),
))
taskRunInputOutput := tb.TaskRun("test-taskrun-input-output", "foo",
tb.TaskRunOwnerReference("PipelineRun", "test"),
tb.TaskRunSpec(
Expand Down Expand Up @@ -241,13 +226,12 @@ func TestReconcile(t *testing.T) {
)
taskruns := []*v1alpha1.TaskRun{
taskRunSuccess, taskRunWithSaSuccess,
taskRunTemplating, taskRunOverrivdesDefaultTemplating, taskRunDefaultTemplating,
taskRunInputOutput, taskRunWithTaskSpec, taskRunWithClusterTask,
taskRunTemplating, taskRunInputOutput, taskRunWithTaskSpec, taskRunWithClusterTask,
}

d := test.Data{
TaskRuns: taskruns,
Tasks: []*v1alpha1.Task{simpleTask, saTask, templatedTask, defaultTemplatedTask, outputTask},
Tasks: []*v1alpha1.Task{simpleTask, saTask, templatedTask, outputTask},
ClusterTasks: []*v1alpha1.ClusterTask{clustertask},
PipelineResources: []*v1alpha1.PipelineResource{gitResource, anotherGitResource, imageResource},
}
Expand Down Expand Up @@ -283,7 +267,7 @@ func TestReconcile(t *testing.T) {
tb.BuildSource("git-resource", tb.BuildSourceGit("https://foo.git", "master")),
entrypointCopyStep,
tb.BuildStep("mycontainer", "myimage", tb.Command(entrypointLocation),
tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-arg=foo","--my-additional-arg=gcr.io/kristoff/sven"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`),
tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-arg=foo","--my-arg-with-default=bar","--my-arg-with-default2=thedefault","--my-additional-arg=gcr.io/kristoff/sven"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`),
tb.VolumeMount(toolsMount),
),
tb.BuildStep("myothercontainer", "myotherimage", tb.Command(entrypointLocation),
Expand All @@ -292,36 +276,6 @@ func TestReconcile(t *testing.T) {
),
tb.BuildVolume(getToolsVolume(taskRunTemplating.Name)),
),
}, {
name: "input-overrides-default-params",
taskRun: taskRunOverrivdesDefaultTemplating,
wantBuildSpec: tb.BuildSpec(
entrypointCopyStep,
tb.BuildStep("mycontainer", "myimage", tb.Command(entrypointLocation),
tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-arg=foo"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`),
tb.VolumeMount(toolsMount),
),
tb.BuildStep("myothercontainer", "myotherimage", tb.Command(entrypointLocation),
tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-other-arg=https://foo.git"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`),
tb.VolumeMount(toolsMount),
),
tb.BuildVolume(getToolsVolume(taskRunOverrivdesDefaultTemplating.Name)),
),
}, {
name: "default-params",
taskRun: taskRunDefaultTemplating,
wantBuildSpec: tb.BuildSpec(
entrypointCopyStep,
tb.BuildStep("mycontainer", "myimage", tb.Command(entrypointLocation),
tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-arg=mydefault"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`),
tb.VolumeMount(toolsMount),
),
tb.BuildStep("myothercontainer", "myotherimage", tb.Command(entrypointLocation),
tb.EnvVar("ENTRYPOINT_OPTIONS", `{"args":["/mycmd","--my-other-arg=https://foo.git"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`),
tb.VolumeMount(toolsMount),
),
tb.BuildVolume(getToolsVolume(taskRunDefaultTemplating.Name)),
),
}, {
name: "wrap-steps",
taskRun: taskRunInputOutput,
Expand Down Expand Up @@ -397,15 +351,23 @@ func TestReconcile(t *testing.T) {
if err != nil {
t.Errorf("Invalid resource key: %v", err)
}

tr, err := clients.Pipeline.PipelineV1alpha1().TaskRuns(namespace).Get(name, metav1.GetOptions{})
if err != nil {
t.Fatalf("getting updated taskrun: %v", err)
}
condition := tr.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 != reasonRunning {
t.Errorf("Expected reason %q but was %s", reasonRunning, condition.Reason)
}

if tr.Status.PodName == "" {
t.Fatalf("Reconcile didn't set pod name")
}

// check error
pod, err := clients.Kube.CoreV1().Pods(tr.Namespace).Get(tr.Status.PodName, metav1.GetOptions{})
if err != nil {
t.Fatalf("Failed to fetch build pod: %v", err)
Expand All @@ -420,26 +382,15 @@ func TestReconcile(t *testing.T) {
if d := cmp.Diff(pod.Spec, wantPod.Spec); d != "" {
t.Errorf("pod spec doesn't match, diff: %s", d)
}

// This TaskRun is in progress now and the status should reflect that.
condition := tr.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 != reasonRunning {
t.Errorf("Expected reason %q but was %s", reasonRunning, condition.Reason)
}

if len(clients.Kube.Actions()) == 0 {
t.Fatalf("Expected actions to be logged in the kubeclient, got none")
}
// 3. check that PVC 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
expectedVolume := getExpectedPVC(tr)
if d := cmp.Diff(pvc.Name, expectedVolume.Name); d != "" {
t.Errorf("pvc doesn't match, diff: %s", d)
Expand Down
70 changes: 0 additions & 70 deletions pkg/reconciler/v1alpha1/taskrun/validate.go

This file was deleted.

Loading

0 comments on commit 1bb816f

Please sign in to comment.