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 #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).

Some minor changes:
* Combined 3 test cases in the taskrun reconciler into one test (testing
  templating with default params).
* Reorganized logic in some reconciler test failures so that `Fatalf`
  won't prevent the dev from seeing informative failure info
  (specifically the condition tends to have a reason that explains why
  the case is failing)

(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 authored and knative-prow-robot committed Jan 9, 2019
1 parent e2f4e40 commit 93074e1
Show file tree
Hide file tree
Showing 10 changed files with 395 additions and 250 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 93074e1

Please sign in to comment.