From c6c149ef690d331367482b6561cd05b1928d38b9 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Wed, 30 Oct 2019 10:15:33 +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 ++++++------ pkg/apis/pipeline/v1alpha1/pipelinerun_types.go | 2 +- .../pipeline/v1alpha1/pipelinerun_validation.go | 14 +++++++------- .../v1alpha1/pipelinerun_validation_test.go | 16 ++++++++-------- .../pipelinerun/resources/pipelinespec.go | 2 +- .../pipelinerun/resources/pipelinespec_test.go | 4 ++-- test/builder/pipeline.go | 4 +++- test/builder/pipeline_test.go | 4 ++-- 8 files changed, 30 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..a5a9ddb1f4e 100644 --- a/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go @@ -38,17 +38,17 @@ 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") + // Check that one of PipelineRef and TaskSpec is present + 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 + // Validate TaskSpec if it's present if ps.PipelineSpec != nil { if err := ps.PipelineSpec.Validate(ctx); err != nil { return err 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/reconciler/pipelinerun/resources/pipelinespec.go b/pkg/reconciler/pipelinerun/resources/pipelinespec.go index b40861eef4e..c63304b77ad 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: // 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 c805f31003f..8185dcb94e1 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 b8242b0e87d..ce9fcc45d20 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{{