Consolidate Results validation + add Results to Run status

examples/run/output-pipeline-run.yaml

@@ -10,10 +10,8 @@ spec:
     type: manual
   serviceAccount: 'default'
   results:
-    logs:
-      name: 'logBucket'
-      type: 'gcs'
-      url: 'gcs://somebucket/results/logs'    
+    type: 'gcs'
+    url: 'gcs://somebucket/results/logs'
   resources:
   - name: first-create-file
     inputs:

examples/run/pipeline-run.yaml

@@ -10,10 +10,8 @@ spec:
     type: manual
   serviceAccount: 'default'
   results:
-    logs:
-      name: 'logBucket'
-      type: 'gcs'
-      url: 'gcs://somebucket/results/logs'
+    type: 'gcs'
+    url: 'gcs://somebucket/results/logs'
   resources:
   - name: build-skaffold-web
     inputs:

examples/run/task-run.yaml

@@ -8,6 +8,9 @@ spec:
   trigger:
     triggerRef:
       type: manual
+  results:
+    type: 'gcs'
+    url: 'gcs://somebucket/results/logs'
   inputs:
     resources:
     - name: workspace

pkg/apis/pipeline/v1alpha1/pipelinerun_types.go

@@ -47,7 +47,7 @@ type PipelineRunSpec struct {
 	// +optional
 	ServiceAccount string `json:"serviceAccount"`
 	// +optional
-	Results    *Results `json:"results"`
+	Results    *Results `json:"results,omitempty"`
 	Generation int64    `json:"generation,omitempty"`
 }
 
@@ -92,32 +92,6 @@ type PipelineRef struct {
 	APIVersion string `json:"apiVersion,omitempty"`
 }
 
-// AllResultTargetTypes is a list of all ResultTargetTypes, used for validation
-var AllResultTargetTypes = []ResultTargetType{ResultTargetTypeGCS}
-
-// ResultTargetType represents the type of endpoint that this result target is,
-// so that the controller will know how to write results to it.
-type ResultTargetType string
-
-const (
-	// ResultTargetTypeGCS indicates that the URL endpoint is a GCS bucket.
-	ResultTargetTypeGCS = "gcs"
-)
-
-// Results tells a pipeline where to persist the results of runnign the pipeline.
-type Results struct {
-	// Logs will store all logs output from running a task.
-	Logs ResultTarget `json:"logs"`
-}
-
-// ResultTarget is used to identify an endpoint where results can be uploaded. The
-// serviceaccount used for the pipeline must have access to this endpoint.
-type ResultTarget struct {
-	Name string           `json:"name"`
-	Type ResultTargetType `json:"type"`
-	URL  string           `json:"url"`
-}
-
 // PipelineTriggerType indicates the mechanism by which this PipelineRun was created.
 type PipelineTriggerType string
 
@@ -137,6 +111,8 @@ type PipelineTriggerRef struct {
 // PipelineRunStatus defines the observed state of PipelineRun
 type PipelineRunStatus struct {
 	Conditions duckv1alpha1.Conditions `json:"conditions"`
+	// In #107 should be updated to hold the location logs have been uploaded to
+	Results Results `json:"results"`
 	// map of TaskRun Status with the taskRun name as the key
 	//+optional
 	TaskRuns map[string]TaskRunStatus `json:"taskRuns,omitempty"`

pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go

@@ -17,8 +17,6 @@ limitations under the License.
 package v1alpha1
 
 import (
-	"net/url"
-
 	"github.com/knative/pkg/apis"
 	"k8s.io/apimachinery/pkg/api/equality"
 )
@@ -43,36 +41,12 @@ func (ps *PipelineRunSpec) Validate() *apis.FieldError {
 	if ps.PipelineTriggerRef.Type != PipelineTriggerTypeManual {
 		return apis.ErrInvalidValue(string(ps.PipelineTriggerRef.Type), "pipelinerun.spec.triggerRef.type")
 	}
-
+	// check for results
 	if ps.Results != nil {
-		// Results.Logs should have a valid URL and ResultTargetType
-		if err := validateURL(ps.Results.Logs.URL, "pipelinerun.spec.Results.Logs.URL"); err != nil {
-			return err
-		}
-		if err := validateResultTargetType(ps.Results.Logs.Type, "pipelinerun.spec.Results.Logs.Type"); err != nil {
+		if err := ps.Results.Validate("spec.results"); err != nil {
 			return err
 		}
 	}
 
 	return nil
 }
-
-func validateURL(u, path string) *apis.FieldError {
-	if u == "" {
-		return nil
-	}
-	_, err := url.ParseRequestURI(u)
-	if err != nil {
-		return apis.ErrInvalidValue(u, path)
-	}
-	return nil
-}
-
-func validateResultTargetType(r ResultTargetType, path string) *apis.FieldError {
-	for _, a := range AllResultTargetTypes {
-		if a == r {
-			return nil
-		}
-	}
-	return apis.ErrInvalidValue(string(r), path)
-}

pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go

@@ -23,15 +23,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-var validURL = "http://www.google.com"
-
-func validResultTarget(name string) ResultTarget {
-	return ResultTarget{
-		URL:  validURL,
-		Name: name,
-		Type: "gcs",
-	}
-}
 func TestPipelineRun_Invalidate(t *testing.T) {
 	tests := []struct {
 		name string
@@ -87,29 +78,6 @@ func TestPipelineRun_Invalidate(t *testing.T) {
 				},
 			},
 			want: apis.ErrInvalidValue("badtype", "pipelinerun.spec.triggerRef.type"),
-		}, {
-			name: "invalid results",
-			pr: PipelineRun{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "pipelinelineName",
-				},
-				Spec: PipelineRunSpec{
-					PipelineRef: PipelineRef{
-						Name: "prname",
-					},
-					PipelineTriggerRef: PipelineTriggerRef{
-						Type: PipelineTriggerTypeManual,
-					},
-					Results: &Results{
-						Logs: ResultTarget{
-							Name: "runs",
-							URL:  "badurl",
-							Type: "gcs",
-						},
-					},
-				},
-			},
-			want: apis.ErrInvalidValue("badurl", "pipelinerun.spec.Results.Logs.URL"),
 		},
 	}
 
@@ -136,7 +104,8 @@ func TestPipelineRun_Validate(t *testing.T) {
 				Type: "manual",
 			},
 			Results: &Results{
-				Logs: validResultTarget("logs"),
+				URL:  "http://www.google.com",
+				Type: "gcs",
 			},
 		},
 	}

pkg/apis/pipeline/v1alpha1/result_types.go

@@ -0,0 +1,85 @@
+/*
+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 v1alpha1
+
+import (
+	"fmt"
+	"net/url"
+
+	"github.com/knative/pkg/apis"
+)
+
+// AllResultTargetTypes is a list of all ResultTargetTypes, used for validation
+var AllResultTargetTypes = []ResultTargetType{ResultTargetTypeGCS}
+
+// ResultTargetType represents the type of endpoint that this result target is,
+// so that the controller will know how to write results to it.
+type ResultTargetType string
+
+const (
+	// ResultTargetTypeGCS indicates that the URL endpoint is a GCS bucket.
+	ResultTargetTypeGCS = "gcs"
+)
+
+// Results is used to identify an endpoint where results can be uploaded. The
+// serviceaccount used for the pipeline must have access to this endpoint.
+type Results struct {
+	Type ResultTargetType `json:"type"`
+	URL  string           `json:"url"`
+}
+
+// Validate will validate the result configuration. The path is the path at which
+// we found this instance of `Results` (since it is probably a member of another
+// structure) and will be used to report any errors.
+func (r *Results) Validate(path string) *apis.FieldError {
+	if r.Type != ResultTargetTypeGCS {
+		return apis.ErrInvalidValue(string(r.Type), fmt.Sprintf("%s.Type", path))
+	}
+
+	if err := validateResultTargetType(r.Type, fmt.Sprintf("%s.Type", path)); err != nil {
+		return err
+	}
+
+	if r.URL == "" {
+		return apis.ErrMissingField(fmt.Sprintf("%s.URL", path))
+	}
+
+	if err := validateURL(r.URL, fmt.Sprintf("%s.URL", path)); err != nil {
+		return err
+	}
+	return nil
+}
+
+func validateURL(u, path string) *apis.FieldError {
+	if u == "" {
+		return nil
+	}
+	_, err := url.ParseRequestURI(u)
+	if err != nil {
+		return apis.ErrInvalidValue(u, path)
+	}
+	return nil
+}
+
+func validateResultTargetType(r ResultTargetType, path string) *apis.FieldError {
+	for _, a := range AllResultTargetTypes {
+		if a == r {
+			return nil
+		}
+	}
+	return apis.ErrInvalidValue(string(r), path)
+}

pkg/apis/pipeline/v1alpha1/result_types_test.go

@@ -0,0 +1,85 @@
+/*
+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 v1alpha1
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/knative/pkg/apis"
+)
+
+func TestValidate(t *testing.T) {
+	results := Results{
+		URL:  "http://google.com",
+		Type: "gcs",
+	}
+	err := results.Validate("somepath")
+	if err != nil {
+		t.Fatalf("Did not expect error when validating valid Results but got %s", err)
+	}
+}
+
+func TestValidate_Invalid(t *testing.T) {
+	tests := []struct {
+		name    string
+		results *Results
+		want    *apis.FieldError
+	}{
+		{
+			name: "invalid task type result",
+			results: &Results{
+				URL:  "http://www.google.com",
+				Type: "wrongtype",
+			},
+			want: apis.ErrInvalidValue("wrongtype", "spec.results.Type"),
+		},
+		{
+			name: "invalid task type results missing url",
+			results: &Results{
+				Type: ResultTargetTypeGCS,
+				URL:  "",
+			},
+			want: apis.ErrMissingField("spec.results.URL"),
+		},
+		{
+			name: "invalid task type results bad url",
+			results: &Results{
+				Type: ResultTargetTypeGCS,
+				URL:  "badurl",
+			},
+			want: apis.ErrInvalidValue("badurl", "spec.results.URL"),
+		},
+		{
+			name: "invalid task type results type",
+			results: &Results{
+				Type: "badtype",
+				URL:  "http://www.google.com",
+			},
+			want: apis.ErrInvalidValue("badtype", "spec.results.Type"),
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			path := "spec.results"
+			err := tc.results.Validate(path)
+			if d := cmp.Diff(err.Error(), tc.want.Error()); d != "" {
+				t.Errorf("Results.Validate/%s (-want, +got) = %v", tc.name, d)
+			}
+		})
+	}
+}

pkg/apis/pipeline/v1alpha1/taskrun_types.go

@@ -40,7 +40,8 @@ type TaskRunSpec struct {
 	Inputs TaskRunInputs `json:"inputs,omitempty"`
 	// +optional
 	Outputs TaskRunOutputs `json:"outputs,omitempty"`
-	Results Results        `json:"results"`
+	// +optional
+	Results *Results `json:"results,omitempty"`
 	// +optional
 	Generation int64 `json:"generation,omitempty"`
 	// +optional
@@ -103,6 +104,9 @@ type TaskRunStatus struct {
 	// +optional
 	Conditions duckv1alpha1.Conditions `json:"conditions,omitempty"`
 
+	// In #107 should be updated to hold the location logs have been uploaded to
+	Results Results `json:"results"`
+
 	// PodName is the name of the pod responsible for executing this task's steps.
 	PodName string `json:"podName"`