Skip to content

Commit

Permalink
Replace ValidateFrom with PipelineSpec.Validate
Browse files Browse the repository at this point in the history 
While working on #1256 I noticed that we had two different functions
to validate that the `from` field. One was being used by the webhook,
and the other in the reconciler. I replaced the `ValidateFrom` in the
reconciler with the more general `Validate` that validates the entire
pipeline spec, not just one field. In addition, I fixed some of the
reconciler tests that were now failing due to having an invalid pipeline
spec e.g. pipeline tasks with no name.

Signed-off-by: Dibyo Mukherjee <[email protected]>
  • Loading branch information
@dibyom @tekton-robot
dibyom authored and tekton-robot committed Oct 30, 2019
1 parent ee806d2 commit fe47335
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 314 deletions.
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1alpha1/pipeline_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ type PipelineTaskInputResource struct {
type PipelineTaskOutputResource struct {
// Name is the name of the PipelineResource as declared by the Task.
Name string `json:"name"`
// Resource is the name of the DeclaredPipelienResource to use.
// Resource is the name of the DeclaredPipelineResource to use.
Resource string `json:"resource"`
}

Expand Down
25 changes: 13 additions & 12 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,19 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
})
return nil
}

if err := pipelineSpec.Validate(ctx); err != nil {
// This Run has failed, so we need to mark it as failed and stop reconciling it
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonFailedValidation,
Message: fmt.Sprintf("Pipeline %s can't be Run; it has an invalid spec: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pr.Name), err),
})
return nil
}

if err := resources.ValidateResourceBindings(pipelineSpec, pr); err != nil {
// This Run has failed, so we need to mark it as failed and stop reconciling it
pr.Status.SetCondition(&apis.Condition{
Expand Down Expand Up @@ -372,18 +385,6 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
return nil
}

if err := resources.ValidateFrom(pipelineState); err != nil {
// This Run has failed, so we need to mark it as failed and stop reconciling it
pr.Status.SetCondition(&apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonFailedValidation,
Message: fmt.Sprintf("Pipeline %s can't be Run; it invalid input/output linkages: %s",
fmt.Sprintf("%s/%s", pipelineMeta.Namespace, pr.Name), err),
})
return nil
}

for _, rprt := range pipelineState {
err := taskrun.ValidateResolvedTaskResources(rprt.PipelineTask.Params, rprt.ResolvedTaskResources)
if err != nil {
Expand Down
131 changes: 52 additions & 79 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ func TestReconcile(t *testing.T) {
tb.PipelineDeclaredResource("best-image", "image"),
tb.PipelineParamSpec("pipeline-param", v1alpha1.ParamTypeString, tb.ParamSpecDefault("somethingdifferent")),
tb.PipelineParamSpec("rev-param", v1alpha1.ParamTypeString, tb.ParamSpecDefault("revision")),
tb.PipelineParamSpec("bar", v1alpha1.ParamTypeString),
// unit-test-3 uses runAfter to indicate it should run last
tb.PipelineTask("unit-test-3", "unit-test-task",
funParam, moreFunParam, templatedParam,
Expand Down Expand Up @@ -876,93 +877,65 @@ func TestReconcileCancelledPipelineRun(t *testing.T) {

func TestReconcilePropagateLabels(t *testing.T) {
names.TestingSeed()
taskName := "hello-world-1"

tests := []struct {
name string
taskName string
expected *v1alpha1.TaskRun
}{
{
name: "with pipelinetask name",
taskName: "hello-world-1",
expected: tb.TaskRun("test-pipeline-run-with-labels-hello-world-1-9l9zj", "foo",
tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-with-labels",
tb.OwnerReferenceAPIVersion("tekton.dev/v1alpha1"),
tb.Controller, tb.BlockOwnerDeletion,
),
tb.TaskRunLabel("tekton.dev/pipeline", "test-pipeline"),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineTaskLabelKey, "hello-world-1"),
tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-with-labels"),
tb.TaskRunLabel("PipelineRunLabel", "PipelineRunValue"),
tb.TaskRunSpec(
tb.TaskRunTaskRef("hello-world"),
tb.TaskRunServiceAccountName("test-sa"),
),
),
}, {
name: "without pipelinetask name",
expected: tb.TaskRun("test-pipeline-run-with-labels--mz4c7", "foo",
tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-with-labels",
tb.OwnerReferenceAPIVersion("tekton.dev/v1alpha1"),
tb.Controller, tb.BlockOwnerDeletion,
),
tb.TaskRunLabel("tekton.dev/pipeline", "test-pipeline"),
tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-with-labels"),
tb.TaskRunLabel("PipelineRunLabel", "PipelineRunValue"),
tb.TaskRunSpec(
tb.TaskRunTaskRef("hello-world"),
tb.TaskRunServiceAccountName("test-sa"),
),
),
},
}
ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", "foo", tb.PipelineSpec(
tb.PipelineTask(taskName, "hello-world"),
))}
prs := []*v1alpha1.PipelineRun{tb.PipelineRun("test-pipeline-run-with-labels", "foo",
tb.PipelineRunLabel("PipelineRunLabel", "PipelineRunValue"),
tb.PipelineRunLabel("tekton.dev/pipeline", "WillNotBeUsed"),
tb.PipelineRunSpec("test-pipeline",
tb.PipelineRunServiceAccountName("test-sa"),
),
)}
ts := []*v1alpha1.Task{tb.Task("hello-world", "foo")}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ps := []*v1alpha1.Pipeline{tb.Pipeline("test-pipeline", "foo", tb.PipelineSpec(
tb.PipelineTask(tt.taskName, "hello-world"),
))}
prs := []*v1alpha1.PipelineRun{tb.PipelineRun("test-pipeline-run-with-labels", "foo",
tb.PipelineRunLabel("PipelineRunLabel", "PipelineRunValue"),
tb.PipelineRunLabel("tekton.dev/pipeline", "WillNotBeUsed"),
tb.PipelineRunSpec("test-pipeline",
tb.PipelineRunServiceAccountName("test-sa"),
),
)}
ts := []*v1alpha1.Task{tb.Task("hello-world", "foo")}
d := test.Data{
PipelineRuns: prs,
Pipelines: ps,
Tasks: ts,
}

d := test.Data{
PipelineRuns: prs,
Pipelines: ps,
Tasks: ts,
}
expected := tb.TaskRun("test-pipeline-run-with-labels-hello-world-1-9l9zj", "foo",
tb.TaskRunOwnerReference("PipelineRun", "test-pipeline-run-with-labels",
tb.OwnerReferenceAPIVersion("tekton.dev/v1alpha1"),
tb.Controller, tb.BlockOwnerDeletion,
),
tb.TaskRunLabel("tekton.dev/pipeline", "test-pipeline"),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineTaskLabelKey, "hello-world-1"),
tb.TaskRunLabel("tekton.dev/pipelineRun", "test-pipeline-run-with-labels"),
tb.TaskRunLabel("PipelineRunLabel", "PipelineRunValue"),
tb.TaskRunSpec(
tb.TaskRunTaskRef("hello-world"),
tb.TaskRunServiceAccountName("test-sa"),
),
)

testAssets, cancel := getPipelineRunController(t, d)
defer cancel()
c := testAssets.Controller
clients := testAssets.Clients
testAssets, cancel := getPipelineRunController(t, d)
defer cancel()
c := testAssets.Controller
clients := testAssets.Clients

err := c.Reconciler.Reconcile(context.Background(), "foo/test-pipeline-run-with-labels")
if err != nil {
t.Errorf("Did not expect to see error when reconciling completed PipelineRun but saw %s", err)
}
err := c.Reconciler.Reconcile(context.Background(), "foo/test-pipeline-run-with-labels")
if err != nil {
t.Errorf("Did not expect to see error when reconciling completed PipelineRun but saw %s", err)
}

// Check that the PipelineRun was reconciled correctly
_, err = clients.Pipeline.Tekton().PipelineRuns("foo").Get("test-pipeline-run-with-labels", metav1.GetOptions{})
if err != nil {
t.Fatalf("Somehow had error getting completed reconciled run out of fake client: %s", err)
}
// Check that the PipelineRun was reconciled correctly
_, err = clients.Pipeline.Tekton().PipelineRuns("foo").Get("test-pipeline-run-with-labels", metav1.GetOptions{})
if err != nil {
t.Fatalf("Somehow had error getting completed reconciled run out of fake client: %s", err)
}

// Check that the expected TaskRun was created
actual := clients.Pipeline.Actions()[0].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
if actual == nil {
t.Errorf("Expected a TaskRun to be created, but it wasn't.")
}
// Check that the expected TaskRun was created
actual := clients.Pipeline.Actions()[0].(ktesting.CreateAction).GetObject().(*v1alpha1.TaskRun)
if actual == nil {
t.Errorf("Expected a TaskRun to be created, but it wasn't.")
}

if d := cmp.Diff(actual, tt.expected); d != "" {
t.Errorf("expected to see TaskRun %v created. Diff %s", tt.expected, d)
}
})
if d := cmp.Diff(actual, expected); d != "" {
t.Errorf("expected to see TaskRun %v created. Diff %s", expected, d)
}
}

Expand Down
46 changes: 0 additions & 46 deletions pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,52 +405,6 @@ func isSkipped(rprt *ResolvedPipelineRunTask, stateMap map[string]*ResolvedPipel
return false
}

func findReferencedTask(pb string, state []*ResolvedPipelineRunTask) *ResolvedPipelineRunTask {
for _, rprtRef := range state {
if rprtRef.PipelineTask.Name == pb {
return rprtRef
}
}
return nil
}

// ValidateFrom will look at any `from` clauses in the resolved PipelineRun state
// and validate it: the `from` must specify an input of the current `Task`. The `PipelineTask`
// it corresponds to must actually exist in the `Pipeline`. The `PipelineResource` that is bound to the input
// must be the same `PipelineResource` that was bound to the output of the previous `Task`. If the state is
// not valid, it will return an error.
func ValidateFrom(state PipelineRunState) error {
for _, rprt := range state {
if rprt.PipelineTask.Resources != nil {
for _, dep := range rprt.PipelineTask.Resources.Inputs {
inputBinding := rprt.ResolvedTaskResources.Inputs[dep.Name]
for _, pb := range dep.From {
if pb == rprt.PipelineTask.Name {
return xerrors.Errorf("PipelineTask %s is trying to depend on a PipelineResource from itself", pb)
}
depTask := findReferencedTask(pb, state)
if depTask == nil {
return xerrors.Errorf("pipelineTask %s is trying to depend on previous Task %q but it does not exist", rprt.PipelineTask.Name, pb)
}

sameBindingExists := false
for _, output := range depTask.ResolvedTaskResources.Outputs {
if output.Name == inputBinding.Name {
sameBindingExists = true
}
}
if !sameBindingExists {
return xerrors.Errorf("from is ambiguous: input %q for PipelineTask %q is bound to %q but no outputs in PipelineTask %q are bound to same resource",
dep.Name, rprt.PipelineTask.Name, inputBinding.Name, depTask.PipelineTask.Name)
}
}
}
}
}

return nil
}

func resolveConditionChecks(pt *v1alpha1.PipelineTask, taskRunStatus map[string]*v1alpha1.PipelineRunTaskRunStatus, taskRunName string, getTaskRun resources.GetTaskRun, getCondition GetCondition, providedResources map[string]*v1alpha1.PipelineResource) ([]*ResolvedConditionCheck, error) {
rccs := []*ResolvedConditionCheck{}
for _, ptc := range pt.Conditions {
Expand Down
Loading

0 comments on commit fe47335

Please sign in to comment.