From 19070c7b4ad7ddb982ff31315fd5badabf632cbe Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Tue, 5 Nov 2019 11:35:51 +0100 Subject: [PATCH] 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 --- .../v1alpha1/pipelinerun_defaults_test.go | 12 ++++----- .../pipeline/v1alpha1/pipelinerun_types.go | 2 +- .../v1alpha1/pipelinerun_validation.go | 10 +++---- .../v1alpha1/pipelinerun_validation_test.go | 16 +++++------ .../v1alpha1/zz_generated.deepcopy.go | 6 ++++- pkg/reconciler/pipelinerun/metrics.go | 6 ++++- .../pipelinerun/resources/pipelinespec.go | 2 +- .../resources/pipelinespec_test.go | 4 +-- test/builder/pipeline.go | 4 ++- test/builder/pipeline_test.go | 4 +-- third_party/VENDOR-LICENSE | 27 +++++++++++++++++++ 11 files changed, 65 insertions(+), 28 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go b/pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go index 874c1d20127..44c90c83042 100644 --- 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 index 130ac88c670..e5cb686cb26 100644 --- 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 index a4b3734f491..fe08e426e47 100644 --- 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 index 3212f5b8ced..89be44a0915 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go @@ -63,7 +63,7 @@ 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{ @@ -71,7 +71,7 @@ func TestPipelineRun_Invalidate(t *testing.T) { 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 index a6466a997d6..1ef0379f39c 100644 --- a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go @@ -1003,7 +1003,11 @@ func (in *PipelineRunList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PipelineRunSpec) DeepCopyInto(out *PipelineRunSpec) { *out = *in - out.PipelineRef = in.PipelineRef + if in.PipelineRef != nil { + in, out := &in.PipelineRef, &out.PipelineRef + *out = new(PipelineRef) + **out = **in + } if in.PipelineSpec != nil { in, out := &in.PipelineSpec, &out.PipelineSpec *out = new(PipelineSpec) diff --git a/pkg/reconciler/pipelinerun/metrics.go b/pkg/reconciler/pipelinerun/metrics.go index 6c9ea52c233..7620be0c01c 100644 --- 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 index b40861eef4e..70a362b9899 100644 --- 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 index 59b4d6cf4db..2caf9725c26 100644 --- 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 index 4548607e2bc..ccb37c49ff6 100644 --- 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 index 116d32c6cb7..35b76775a59 100644 --- 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 index be452eec13a..f056baf91b3 100644 --- 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