Make PipelineSpec/PipelineRef in PipelineRun consistent with TaskRun

In TaskRun, TaskRef can be nil, which was not the case for PipelineRun (and PipelineRef). This fixes that by making the two consistent. Signed-off-by: Vincent Demeester <[email protected]>
tektoncd · Nov 5, 2019 · f15a0f3 · f15a0f3
1 parent 199ffdd
commit f15a0f3
Show file tree

Hide file tree

Showing 11 changed files with 65 additions and 28 deletions.
diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go
@@ -88,12 +88,12 @@ func TestPipelineRunDefaulting(t *testing.T) {
 		name: "PipelineRef upgrade context",
 		in: &v1alpha1.PipelineRun{
 			Spec: v1alpha1.PipelineRunSpec{
-				PipelineRef: v1alpha1.PipelineRef{Name: "foo"},
+				PipelineRef: &v1alpha1.PipelineRef{Name: "foo"},
 			},
 		},
 		want: &v1alpha1.PipelineRun{
 			Spec: v1alpha1.PipelineRunSpec{
-				PipelineRef: v1alpha1.PipelineRef{Name: "foo"},
+				PipelineRef: &v1alpha1.PipelineRef{Name: "foo"},
 				Timeout:     &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute},
 			},
 		},
@@ -102,12 +102,12 @@ func TestPipelineRunDefaulting(t *testing.T) {
 		name: "PipelineRef default config context",
 		in: &v1alpha1.PipelineRun{
 			Spec: v1alpha1.PipelineRunSpec{
-				PipelineRef: v1alpha1.PipelineRef{Name: "foo"},
+				PipelineRef: &v1alpha1.PipelineRef{Name: "foo"},
 			},
 		},
 		want: &v1alpha1.PipelineRun{
 			Spec: v1alpha1.PipelineRunSpec{
-				PipelineRef: v1alpha1.PipelineRef{Name: "foo"},
+				PipelineRef: &v1alpha1.PipelineRef{Name: "foo"},
 				Timeout:     &metav1.Duration{Duration: 5 * time.Minute},
 			},
 		},
@@ -127,12 +127,12 @@ func TestPipelineRunDefaulting(t *testing.T) {
 		name: "PipelineRef default config context with sa",
 		in: &v1alpha1.PipelineRun{
 			Spec: v1alpha1.PipelineRunSpec{
-				PipelineRef: v1alpha1.PipelineRef{Name: "foo"},
+				PipelineRef: &v1alpha1.PipelineRef{Name: "foo"},
 			},
 		},
 		want: &v1alpha1.PipelineRun{
 			Spec: v1alpha1.PipelineRunSpec{
-				PipelineRef:        v1alpha1.PipelineRef{Name: "foo"},
+				PipelineRef:        &v1alpha1.PipelineRef{Name: "foo"},
 				Timeout:            &metav1.Duration{Duration: 5 * time.Minute},
 				ServiceAccountName: "tekton",
 			},

diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go
@@ -45,7 +45,7 @@ var _ apis.Defaultable = (*PipelineRun)(nil)
 // PipelineRunSpec defines the desired state of PipelineRun
 type PipelineRunSpec struct {
 	// +optional
-	PipelineRef PipelineRef `json:"pipelineRef,omitempty"`
+	PipelineRef *PipelineRef `json:"pipelineRef,omitempty"`
 	// +optional
 	PipelineSpec *PipelineSpec `json:"pipelineSpec,omitempty"`
 	// Resources is a list of bindings specifying which actual instances of

diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go
@@ -38,14 +38,14 @@ func (ps *PipelineRunSpec) Validate(ctx context.Context) *apis.FieldError {
 		return apis.ErrMissingField("spec")
 	}
 
-	// can't have both pipelinekRef and pipelineSpec at the same time
-	if ps.PipelineRef.Name != "" && ps.PipelineSpec != nil {
-		return apis.ErrDisallowedFields("spec.pipelineRef", "spec.pipelineSpec")
+	// can't have both pipelineRef and pipelineSpec at the same time
+	if (ps.PipelineRef != nil && ps.PipelineRef.Name != "") && ps.PipelineSpec != nil {
+		return apis.ErrDisallowedFields("spec.pipelineref", "spec.pipelinespec")
 	}
 
 	// Check that one of PipelineRef and PipelineSpec is present
-	if ps.PipelineRef.Name == "" && ps.PipelineSpec == nil {
-		return apis.ErrMissingField("spec.pipelineRef.name", "spec.pipelineSpec")
+	if (ps.PipelineRef == nil || (ps.PipelineRef != nil && ps.PipelineRef.Name == "")) && ps.PipelineSpec == nil {
+		return apis.ErrMissingField("spec.pipelineref.name", "spec.pipelinespec")
 	}
 
 	// Validate PipelineSpec if it's present

diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go
@@ -63,15 +63,15 @@ func TestPipelineRun_Invalidate(t *testing.T) {
 					ServiceAccountName: "foo",
 				},
 			},
-			want: apis.ErrMissingField("spec.pipelineRef.name, spec.pipelineSpec"),
+			want: apis.ErrMissingField("spec.pipelineref.name, spec.pipelinespec"),
 		}, {
 			name: "negative pipeline timeout",
 			pr: v1alpha1.PipelineRun{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "pipelinelineName",
 				},
 				Spec: v1alpha1.PipelineRunSpec{
-					PipelineRef: v1alpha1.PipelineRef{
+					PipelineRef: &v1alpha1.PipelineRef{
 						Name: "prname",
 					},
 					Timeout: &metav1.Duration{Duration: -48 * time.Hour},
@@ -103,7 +103,7 @@ func TestPipelineRun_Validate(t *testing.T) {
 					Name: "pipelinelineName",
 				},
 				Spec: v1alpha1.PipelineRunSpec{
-					PipelineRef: v1alpha1.PipelineRef{
+					PipelineRef: &v1alpha1.PipelineRef{
 						Name: "prname",
 					},
 				},
@@ -115,7 +115,7 @@ func TestPipelineRun_Validate(t *testing.T) {
 					Name: "pipelinelineName",
 				},
 				Spec: v1alpha1.PipelineRunSpec{
-					PipelineRef: v1alpha1.PipelineRef{
+					PipelineRef: &v1alpha1.PipelineRef{
 						Name: "prname",
 					},
 					Timeout: &metav1.Duration{Duration: 0},
@@ -145,13 +145,13 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) {
 	}, {
 		name: "pipelineRef without Pipeline Name",
 		spec: v1alpha1.PipelineRunSpec{
-			PipelineRef: v1alpha1.PipelineRef{},
+			PipelineRef: &v1alpha1.PipelineRef{},
 		},
-		wantErr: apis.ErrMissingField("spec"),
+		wantErr: apis.ErrMissingField("spec.pipelineref.name", "spec.pipelinespec"),
 	}, {
 		name: "pipelineRef and pipelineSpec together",
 		spec: v1alpha1.PipelineRunSpec{
-			PipelineRef: v1alpha1.PipelineRef{
+			PipelineRef: &v1alpha1.PipelineRef{
 				Name: "pipelinerefname",
 			},
 			PipelineSpec: &v1alpha1.PipelineSpec{
@@ -162,7 +162,7 @@ func TestPipelineRunSpec_Invalidate(t *testing.T) {
 					},
 				}}},
 		},
-		wantErr: apis.ErrDisallowedFields("spec.pipelineSpec", "spec.pipelineRef"),
+		wantErr: apis.ErrDisallowedFields("spec.pipelinespec", "spec.pipelineref"),
 	}}
 	for _, ps := range tests {
 		t.Run(ps.name, func(t *testing.T) {

diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go
diff --git a/pkg/reconciler/pipelinerun/metrics.go b/pkg/reconciler/pipelinerun/metrics.go
@@ -134,9 +134,13 @@ func (r *Recorder) DurationAndCount(pr *v1alpha1.PipelineRun) error {
 		status = "failed"
 	}
 
+	pipelineName := "anonymous"
+	if pr.Spec.PipelineRef != nil && pr.Spec.PipelineRef.Name != "" {
+		pipelineName = pr.Spec.PipelineRef.Name
+	}
 	ctx, err := tag.New(
 		context.Background(),
-		tag.Insert(r.pipeline, pr.Spec.PipelineRef.Name),
+		tag.Insert(r.pipeline, pipelineName),
 		tag.Insert(r.pipelineRun, pr.Name),
 		tag.Insert(r.namespace, pr.Namespace),
 		tag.Insert(r.status, status),

diff --git a/pkg/reconciler/pipelinerun/resources/pipelinespec.go b/pkg/reconciler/pipelinerun/resources/pipelinespec.go
@@ -32,7 +32,7 @@ func GetPipelineData(pipelineRun *v1alpha1.PipelineRun, getPipeline GetPipeline)
 	pipelineMeta := metav1.ObjectMeta{}
 	pipelineSpec := v1alpha1.PipelineSpec{}
 	switch {
-	case pipelineRun.Spec.PipelineRef.Name != "":
+	case pipelineRun.Spec.PipelineRef != nil && pipelineRun.Spec.PipelineRef.Name != "":
 		// Get related pipeline for pipelinerun
 		t, err := getPipeline(pipelineRun.Spec.PipelineRef.Name)
 		if err != nil {

diff --git a/pkg/reconciler/pipelinerun/resources/pipelinespec_test.go b/pkg/reconciler/pipelinerun/resources/pipelinespec_test.go
@@ -43,7 +43,7 @@ func TestGetPipelineSpec_Ref(t *testing.T) {
 			Name: "mypipelinerun",
 		},
 		Spec: v1alpha1.PipelineRunSpec{
-			PipelineRef: v1alpha1.PipelineRef{
+			PipelineRef: &v1alpha1.PipelineRef{
 				Name: "orchestrate",
 			},
 		},
@@ -115,7 +115,7 @@ func TestGetPipelineSpec_Error(t *testing.T) {
 			Name: "mypipelinerun",
 		},
 		Spec: v1alpha1.PipelineRunSpec{
-			PipelineRef: v1alpha1.PipelineRef{
+			PipelineRef: &v1alpha1.PipelineRef{
 				Name: "orchestrate",
 			},
 		},

diff --git a/test/builder/pipeline.go b/test/builder/pipeline.go
@@ -282,7 +282,9 @@ func PipelineRunSpec(name string, ops ...PipelineRunSpecOp) PipelineRunOp {
 	return func(pr *v1alpha1.PipelineRun) {
 		prs := &pr.Spec
 
-		prs.PipelineRef.Name = name
+		prs.PipelineRef = &v1alpha1.PipelineRef{
+			Name: name,
+		}
 		// Set a default timeout
 		prs.Timeout = &metav1.Duration{Duration: config.DefaultTimeoutMinutes * time.Minute}
 

diff --git a/test/builder/pipeline_test.go b/test/builder/pipeline_test.go
@@ -152,7 +152,7 @@ func TestPipelineRun(t *testing.T) {
 			},
 		},
 		Spec: v1alpha1.PipelineRunSpec{
-			PipelineRef:         v1alpha1.PipelineRef{Name: "tomatoes"},
+			PipelineRef:         &v1alpha1.PipelineRef{Name: "tomatoes"},
 			ServiceAccountName:  "sa",
 			ServiceAccountNames: []v1alpha1.PipelineRunSpecServiceAccountName{{TaskName: "foo", ServiceAccountName: "sa-2"}},
 			Params: []v1alpha1.Param{{
@@ -220,7 +220,7 @@ func TestPipelineRunWithResourceSpec(t *testing.T) {
 			},
 		},
 		Spec: v1alpha1.PipelineRunSpec{
-			PipelineRef:         v1alpha1.PipelineRef{Name: "tomatoes"},
+			PipelineRef:         &v1alpha1.PipelineRef{Name: "tomatoes"},
 			ServiceAccountName:  "sa",
 			ServiceAccountNames: []v1alpha1.PipelineRunSpecServiceAccountName{{TaskName: "foo", ServiceAccountName: "sa-2"}},
 			Params: []v1alpha1.Param{{

diff --git a/third_party/VENDOR-LICENSE b/third_party/VENDOR-LICENSE
@@ -5429,6 +5429,33 @@ SOFTWARE.
 
 
 
+===========================================================
+Import: github.com/tektoncd/pipeline/vendor/github.com/mohae/deepcopy
+
+The MIT License (MIT)
+
+Copyright (c) 2014 Joel
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in all
+copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+SOFTWARE.
+
+
+
 ===========================================================
 Import: github.com/tektoncd/pipeline/vendor/github.com/peterbourgon/diskv