Rename TaskCondition to PipelineTaskCondition

Also, add more comments for condition_types and more tests for condition
validation and status updates

pkg/apis/pipeline/v1alpha1/condition_types.go

@@ -24,26 +24,15 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-// Add validation for TaskConditions?
-type TaskCondition struct {
-	ConditionRef string `json:"conditionRef"`
-	// TODO: Support a ConditionSpec?
-	// +optional
-	Params []Param `json:"params,omitempty"`
-}
-
 // Check that Task may be validated and defaulted.
 var _ apis.Validatable = (*Condition)(nil)
 
 // +genclient
 // +genclient:noStatus
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 
-// Task represents a collection of sequential steps that are run as part of a
-// Pipeline using a set of inputs and producing a set of outputs. Tasks execute
-// when TaskRuns are created that provide the input parameters and resources and
-// output resources the Task requires.
-//
+// Condition declares a step that is used to gate the execution of a Task in a Pipeline.
+// A condition execution (ConditionCheck) evaluates to either true or false
 // +k8s:openapi-gen=true
 type Condition struct {
 	metav1.TypeMeta `json:",inline"`
@@ -55,15 +44,22 @@ type Condition struct {
 	Spec ConditionSpec `json:"spec"`
 }
 
+// ConditionSpec defines the desired state of the Condition
 type ConditionSpec struct {
+	// Check declares container whose exit code determines where a condition is true or false
+	Check corev1.Container `json:"check,omitempty"`
+
+	// Params is an optional set of parameters which must be supplied by the user when a Condition
+	// is evaluated
 	// +optional
 	Params []ParamSpec `json:"params,omitempty"`
-	// Check is a container whose exit code determines where a condition is true or false
-	Check corev1.Container `json:"check,omitempty"`
 }
 
+
+// ConditionCheck represents a single evaluation of a Condition step.
 type ConditionCheck TaskRun
 
+// ConditionCheckStatus is the observed state of a ConditionCheck
 type ConditionCheckStatus TaskRunStatus
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

pkg/apis/pipeline/v1alpha1/condition_validation.go

@@ -18,21 +18,24 @@ package v1alpha1
 
 import (
 	"context"
-
 	"github.com/knative/pkg/apis"
 	"k8s.io/apimachinery/pkg/api/equality"
 )
 
-func (c *Condition) Validate(ctx context.Context) *apis.FieldError {
+func (c Condition) Validate(ctx context.Context) *apis.FieldError {
 	if err := validateObjectMetadata(c.GetObjectMeta()); err != nil {
 		return err.ViaField("metadata")
 	}
-	return c.Spec.Validate(ctx)
+	return c.Spec.Validate(ctx).ViaField("Spec")
 }
 
 func (cs *ConditionSpec) Validate(ctx context.Context) *apis.FieldError {
-	if equality.Semantic.DeepEqual(cs, &ConditionSpec{}) {
+	if equality.Semantic.DeepEqual(cs, ConditionSpec{}) {
 		return apis.ErrMissingField(apis.CurrentField)
 	}
+
+	if cs.Check.Image == "" {
+		return apis.ErrMissingField("Check.Image")
+	}
 	return nil
 }

pkg/apis/pipeline/v1alpha1/condition_validation_test.go

@@ -20,11 +20,13 @@ package v1alpha1
 
 import (
 	"context"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/knative/pkg/apis"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"testing"
 )
-
 func TestCondition_Validate(t *testing.T) {
 	c := Condition{
 		ObjectMeta: metav1.ObjectMeta{
@@ -43,6 +45,49 @@ func TestCondition_Validate(t *testing.T) {
 		},
 	}
 	if err := c.Validate(context.Background()); err != nil {
-		t.Errorf("TaskRun.Validate() error = %v", err)
+		t.Errorf("Condition.Validate()  unexpected error = %v", err)
 	}
 }
+
+func TestCondition_Invalidate(t *testing.T) {
+	tcs := []struct{
+		name string
+		cond Condition
+		expectedError apis.FieldError
+	}{{
+		name: "invalid meta",
+		cond: Condition{
+			ObjectMeta: metav1.ObjectMeta{
+				Name : "invalid.,name",
+			},
+		},
+		expectedError: apis.FieldError{
+			Message: "Invalid resource name: special character . must not be present",
+			Paths: []string{"metadata.name"},
+		},
+	},{
+		name: "no image",
+		cond:Condition{
+			ObjectMeta: metav1.ObjectMeta{Name: "cond"},
+			Spec: ConditionSpec{
+				Check: corev1.Container{},
+			},
+		},
+		expectedError: apis.FieldError{
+			Message: "missing field(s)",
+			Paths: []string{"Spec.Check.Image"},
+		},
+	}}
+
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			err := tc.cond.Validate(context.Background())
+			if err == nil {
+				t.Fatalf("Expected an Error, got nothing for %v", tc)
+			}
+			if d := cmp.Diff(tc.expectedError, *err, cmpopts.IgnoreUnexported(apis.FieldError{})); d != "" {
+				t.Errorf("Condition.Validate() errors diff -want, +got: %v", d)
+			}
+		})
+	}
+}

pkg/apis/pipeline/v1alpha1/pipeline_types.go

@@ -85,7 +85,7 @@ type PipelineTask struct {
 
 	// Conditions is a list of conditions that need to be true for the task to run
 	// +optional
-	Conditions []TaskCondition `json:"conditions,omitempty"`
+	Conditions []PipelineTaskCondition `json:"conditions,omitempty"`
 
 	// Retries represents how many times this task should be retried in case of task failure: ConditionSucceeded set to False
 	// +optional
@@ -111,6 +111,17 @@ type PipelineTaskParam struct {
 	Value string `json:"value"`
 }
 
+// PipelineTaskCondition allows a PipelineTask to declare a Condition to be evaluated before
+// the Task is run.
+type PipelineTaskCondition struct {
+	// ConditionRef is the name of the Condition to use for the conditionCheck
+	ConditionRef string `json:"conditionRef"`
+
+	// Params declare parameters passed to this Condition
+	// +optional
+	Params []Param `json:"params,omitempty"`
+}
+
 // PipelineDeclaredResource is used by a Pipeline to declare the types of the
 // PipelineResources that it will required to run and names which can be used to
 // refer to these PipelineResources in PipelineTaskResourceBindings.

pkg/apis/pipeline/v1alpha1/pipelinerun_types.go

@@ -136,7 +136,7 @@ type PipelineRunTaskRunStatus struct {
 	// Status is the TaskRunStatus for the corresponding TaskRun
 	// +optional
 	Status *TaskRunStatus `json:"status,omitempty"`
-	// ConditionChecks is the Status for the corresponding ConditionCheck
+	// ConditionChecks maps a conditionCheckName to the Status for the corresponding ConditionCheck
 	// +optional
 	ConditionChecks map[string]*PipelineRunConditionCheckStatus `json:"conditionChecks,omitempty"`
 }