Skip to content

Commit

Permalink
Make PipelineSpec/PipelineRef in PipelineRun consistent with TaskRun
Browse files Browse the repository at this point in the history 
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 <vdemeest@redhat.com>
  • Loading branch information
@vdemeester
vdemeester committed Oct 30, 2019
1 parent ee806d2 commit c6c149e
Show file tree
Hide file tree
Showing 8 changed files with 30 additions and 28 deletions.
12 changes: 6 additions & 6 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_defaults_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
},
},
Expand All @@ -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},
},
},
Expand All @@ -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",
},
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/pipeline/v1alpha1/pipelinerun_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 7 additions & 7 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 8 additions & 8 deletions pkg/apis/pipeline/v1alpha1/pipelinerun_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -103,7 +103,7 @@ func TestPipelineRun_Validate(t *testing.T) {
Name: "pipelinelineName",
},
Spec: v1alpha1.PipelineRunSpec{
PipelineRef: v1alpha1.PipelineRef{
PipelineRef: &v1alpha1.PipelineRef{
Name: "prname",
},
},
Expand All @@ -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},
Expand Down Expand Up @@ -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{
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/reconciler/pipelinerun/resources/pipelinespec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions pkg/reconciler/pipelinerun/resources/pipelinespec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestGetPipelineSpec_Ref(t *testing.T) {
Name: "mypipelinerun",
},
Spec: v1alpha1.PipelineRunSpec{
PipelineRef: v1alpha1.PipelineRef{
PipelineRef: &v1alpha1.PipelineRef{
Name: "orchestrate",
},
},
Expand Down Expand Up @@ -115,7 +115,7 @@ func TestGetPipelineSpec_Error(t *testing.T) {
Name: "mypipelinerun",
},
Spec: v1alpha1.PipelineRunSpec{
PipelineRef: v1alpha1.PipelineRef{
PipelineRef: &v1alpha1.PipelineRef{
Name: "orchestrate",
},
},
Expand Down
4 changes: 3 additions & 1 deletion test/builder/pipeline.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}

Expand Down
4 changes: 2 additions & 2 deletions test/builder/pipeline_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{{
Expand Down Expand Up @@ -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{{
Expand Down

0 comments on commit c6c149e

Please sign in to comment.