From f2017e5e73b84d6683c4cf3b5caac6ee8be4367e Mon Sep 17 00:00:00 2001 From: Jerop Date: Tue, 28 Mar 2023 13:17:18 -0400 Subject: [PATCH] Split up and refactor `isCustomTask` In this change, we clean up the `isCustomTask` function: - Remove unused `context.Context`. - Remove the check that either `TaskRef` or `TaskSpec` is specified because we check that way before - see [code]. - Define the check that `TaskRef` is a `CustomTask` as a member function of `TaskRef`; and add tests for the member function. - Define the check that `TaskSpec` is a `CustomTask` as a member function of `TaskSpec`; and add tests for the member function. - Update the docstring for `Kind` and `APIVersion` in `TaskRef`. There are no user-facing changes in this commit. [code]: https://github.com/tektoncd/pipeline/blob/b7d815a9d8528547994f65da097050f472bbb8b2/pkg/apis/pipeline/v1beta1/pipeline_types.go#L216-L227 --- docs/pipeline-api.md | 15 ++++-- pkg/apis/pipeline/v1/openapi_generated.go | 4 +- pkg/apis/pipeline/v1/pipeline_types.go | 5 ++ pkg/apis/pipeline/v1/pipeline_types_test.go | 45 ++++++++++++++++ pkg/apis/pipeline/v1/swagger.json | 4 +- pkg/apis/pipeline/v1/taskref_types.go | 10 +++- pkg/apis/pipeline/v1/taskref_types_test.go | 54 +++++++++++++++++++ .../pipeline/v1beta1/openapi_generated.go | 4 +- pkg/apis/pipeline/v1beta1/pipeline_types.go | 5 ++ .../pipeline/v1beta1/pipeline_types_test.go | 45 ++++++++++++++++ pkg/apis/pipeline/v1beta1/swagger.json | 4 +- pkg/apis/pipeline/v1beta1/taskref_types.go | 11 +++- .../pipeline/v1beta1/taskref_types_test.go | 54 +++++++++++++++++++ .../resources/pipelinerunresolution.go | 11 +--- .../resources/pipelinerunresolution_test.go | 15 ------ 15 files changed, 247 insertions(+), 39 deletions(-) create mode 100644 pkg/apis/pipeline/v1/taskref_types_test.go create mode 100644 pkg/apis/pipeline/v1beta1/taskref_types_test.go diff --git a/docs/pipeline-api.md b/docs/pipeline-api.md index 41fa555253c..59e7200d17f 100644 --- a/docs/pipeline-api.md +++ b/docs/pipeline-api.md @@ -4487,7 +4487,9 @@ TaskKind -

TaskKind indicates the kind of the task, namespaced or cluster scoped.

+

TaskKind indicates the Kind of the Task: +1. Namespaced Task when Kind is set to “Task” +2. Custom Task when Kind is non-empty and APIVersion is non-empty

@@ -4499,7 +4501,8 @@ string (Optional) -

API version of the referent

+

API version of the referent +Note: A Task with non-empty APIVersion and Kind is considered a Custom Task

@@ -11953,7 +11956,10 @@ TaskKind -

TaskKind indicates the kind of the task, namespaced or cluster scoped.

+

TaskKind indicates the Kind of the Task: +1. Namespaced Task when Kind is set to “Task” +2. Cluster-Scoped Task when Kind is set to “ClusterTask” +3. Custom Task when Kind is non-empty and APIVersion is non-empty

@@ -11965,7 +11971,8 @@ string (Optional) -

API version of the referent

+

API version of the referent +Note: A Task with non-empty APIVersion and Kind is considered a Custom Task

diff --git a/pkg/apis/pipeline/v1/openapi_generated.go b/pkg/apis/pipeline/v1/openapi_generated.go index 62a5fcd92ec..916b68da440 100644 --- a/pkg/apis/pipeline/v1/openapi_generated.go +++ b/pkg/apis/pipeline/v1/openapi_generated.go @@ -3234,14 +3234,14 @@ func schema_pkg_apis_pipeline_v1_TaskRef(ref common.ReferenceCallback) common.Op }, "kind": { SchemaProps: spec.SchemaProps{ - Description: "TaskKind indicates the kind of the task, namespaced or cluster scoped.", + Description: "TaskKind indicates the Kind of the Task: 1. Namespaced Task when Kind is set to \"Task\" 2. Custom Task when Kind is non-empty and APIVersion is non-empty", Type: []string{"string"}, Format: "", }, }, "apiVersion": { SchemaProps: spec.SchemaProps{ - Description: "API version of the referent", + Description: "API version of the referent Note: A Task with non-empty APIVersion and Kind is considered a Custom Task", Type: []string{"string"}, Format: "", }, diff --git a/pkg/apis/pipeline/v1/pipeline_types.go b/pkg/apis/pipeline/v1/pipeline_types.go index 3fbebee7839..77c46b14f14 100644 --- a/pkg/apis/pipeline/v1/pipeline_types.go +++ b/pkg/apis/pipeline/v1/pipeline_types.go @@ -203,6 +203,11 @@ type PipelineTask struct { Timeout *metav1.Duration `json:"timeout,omitempty"` } +// IsCustomTask checks whether an embedded TaskSpec is a Custom Task +func (et *EmbeddedTask) IsCustomTask() bool { + return et != nil && et.APIVersion != "" && et.Kind != "" +} + // validateRefOrSpec validates at least one of taskRef or taskSpec is specified func (pt PipelineTask) validateRefOrSpec() (errs *apis.FieldError) { // can't have both taskRef and taskSpec at the same time diff --git a/pkg/apis/pipeline/v1/pipeline_types_test.go b/pkg/apis/pipeline/v1/pipeline_types_test.go index 283bd3732db..e26815d0230 100644 --- a/pkg/apis/pipeline/v1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1/pipeline_types_test.go @@ -965,3 +965,48 @@ func TestPipelineTask_IsMatrixed(t *testing.T) { }) } } + +func TestEmbeddedTask_IsCustomTask(t *testing.T) { + tests := []struct { + name string + et *EmbeddedTask + want bool + }{{ + name: "not a custom task - APIVersion and Kind are not set", + et: &EmbeddedTask{}, + want: false, + }, { + name: "not a custom task - APIVersion is not set", + et: &EmbeddedTask{ + TypeMeta: runtime.TypeMeta{ + Kind: "Example", + }, + }, + want: false, + }, { + name: "not a custom task - Kind is not set", + et: &EmbeddedTask{ + TypeMeta: runtime.TypeMeta{ + APIVersion: "example/v0", + }, + }, + want: false, + }, { + name: "custom task - APIVersion and Kind are set", + et: &EmbeddedTask{ + TypeMeta: runtime.TypeMeta{ + Kind: "Example", + APIVersion: "example/v0", + }, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.et.IsCustomTask(); got != tt.want { + t.Errorf("IsCustomTask() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1/swagger.json b/pkg/apis/pipeline/v1/swagger.json index a08d40d8846..93587d6ac89 100644 --- a/pkg/apis/pipeline/v1/swagger.json +++ b/pkg/apis/pipeline/v1/swagger.json @@ -1654,11 +1654,11 @@ "type": "object", "properties": { "apiVersion": { - "description": "API version of the referent", + "description": "API version of the referent Note: A Task with non-empty APIVersion and Kind is considered a Custom Task", "type": "string" }, "kind": { - "description": "TaskKind indicates the kind of the task, namespaced or cluster scoped.", + "description": "TaskKind indicates the Kind of the Task: 1. Namespaced Task when Kind is set to \"Task\" 2. Custom Task when Kind is non-empty and APIVersion is non-empty", "type": "string" }, "name": { diff --git a/pkg/apis/pipeline/v1/taskref_types.go b/pkg/apis/pipeline/v1/taskref_types.go index 74a319dd713..54361f2791b 100644 --- a/pkg/apis/pipeline/v1/taskref_types.go +++ b/pkg/apis/pipeline/v1/taskref_types.go @@ -20,9 +20,12 @@ package v1 type TaskRef struct { // Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names Name string `json:"name,omitempty"` - // TaskKind indicates the kind of the task, namespaced or cluster scoped. + // TaskKind indicates the Kind of the Task: + // 1. Namespaced Task when Kind is set to "Task" + // 2. Custom Task when Kind is non-empty and APIVersion is non-empty Kind TaskKind `json:"kind,omitempty"` // API version of the referent + // Note: A Task with non-empty APIVersion and Kind is considered a Custom Task // +optional APIVersion string `json:"apiVersion,omitempty"` @@ -40,3 +43,8 @@ const ( // NamespacedTaskKind indicates that the task type has a namespaced scope. NamespacedTaskKind TaskKind = "Task" ) + +// IsCustomTask checks whether the reference is to a Custom Task +func (tr *TaskRef) IsCustomTask() bool { + return tr != nil && tr.APIVersion != "" && tr.Kind != "" +} diff --git a/pkg/apis/pipeline/v1/taskref_types_test.go b/pkg/apis/pipeline/v1/taskref_types_test.go new file mode 100644 index 00000000000..cf822578722 --- /dev/null +++ b/pkg/apis/pipeline/v1/taskref_types_test.go @@ -0,0 +1,54 @@ +package v1 + +import "testing" + +func TestTaskRef_IsCustomTask(t *testing.T) { + tests := []struct { + name string + tr *TaskRef + want bool + }{{ + name: "not a custom task - apiVersion and Kind are not set", + tr: &TaskRef{ + Name: "foo", + }, + want: false, + }, { + name: "not a custom task - apiVersion is not set", + tr: &TaskRef{ + Name: "foo", + Kind: "Example", + }, + want: false, + }, { + name: "not a custom task - kind is not set", + tr: &TaskRef{ + Name: "foo", + APIVersion: "example/v0", + }, + want: false, + }, { + name: "custom task with name", + tr: &TaskRef{ + Name: "foo", + Kind: "Example", + APIVersion: "example/v0", + }, + want: true, + }, { + name: "custom task without name", + tr: &TaskRef{ + Kind: "Example", + APIVersion: "example/v0", + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.tr.IsCustomTask(); got != tt.want { + t.Errorf("IsCustomTask() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/openapi_generated.go b/pkg/apis/pipeline/v1beta1/openapi_generated.go index 92bbcf4afd3..f5f23b48027 100644 --- a/pkg/apis/pipeline/v1beta1/openapi_generated.go +++ b/pkg/apis/pipeline/v1beta1/openapi_generated.go @@ -3909,14 +3909,14 @@ func schema_pkg_apis_pipeline_v1beta1_TaskRef(ref common.ReferenceCallback) comm }, "kind": { SchemaProps: spec.SchemaProps{ - Description: "TaskKind indicates the kind of the task, namespaced or cluster scoped.", + Description: "TaskKind indicates the Kind of the Task: 1. Namespaced Task when Kind is set to \"Task\" 2. Cluster-Scoped Task when Kind is set to \"ClusterTask\" 3. Custom Task when Kind is non-empty and APIVersion is non-empty", Type: []string{"string"}, Format: "", }, }, "apiVersion": { SchemaProps: spec.SchemaProps{ - Description: "API version of the referent", + Description: "API version of the referent Note: A Task with non-empty APIVersion and Kind is considered a Custom Task", Type: []string{"string"}, Format: "", }, diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types.go b/pkg/apis/pipeline/v1beta1/pipeline_types.go index 2ea11155148..f38389b9abf 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types.go @@ -209,6 +209,11 @@ type PipelineTask struct { Timeout *metav1.Duration `json:"timeout,omitempty"` } +// IsCustomTask checks whether an embedded TaskSpec is a Custom Task +func (et *EmbeddedTask) IsCustomTask() bool { + return et != nil && et.APIVersion != "" && et.Kind != "" +} + // validateRefOrSpec validates at least one of taskRef or taskSpec is specified func (pt PipelineTask) validateRefOrSpec() (errs *apis.FieldError) { // can't have both taskRef and taskSpec at the same time diff --git a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go index a0e1d8d9102..1598df2a4e4 100644 --- a/pkg/apis/pipeline/v1beta1/pipeline_types_test.go +++ b/pkg/apis/pipeline/v1beta1/pipeline_types_test.go @@ -939,3 +939,48 @@ func TestPipelineTask_IsMatrixed(t *testing.T) { }) } } + +func TestEmbeddedTask_IsCustomTask(t *testing.T) { + tests := []struct { + name string + et *EmbeddedTask + want bool + }{{ + name: "not a custom task - APIVersion and Kind are not set", + et: &EmbeddedTask{}, + want: false, + }, { + name: "not a custom task - APIVersion is not set", + et: &EmbeddedTask{ + TypeMeta: runtime.TypeMeta{ + Kind: "Example", + }, + }, + want: false, + }, { + name: "not a custom task - Kind is not set", + et: &EmbeddedTask{ + TypeMeta: runtime.TypeMeta{ + APIVersion: "example/v0", + }, + }, + want: false, + }, { + name: "custom task - APIVersion and Kind are set", + et: &EmbeddedTask{ + TypeMeta: runtime.TypeMeta{ + Kind: "Example", + APIVersion: "example/v0", + }, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.et.IsCustomTask(); got != tt.want { + t.Errorf("IsCustomTask() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/apis/pipeline/v1beta1/swagger.json b/pkg/apis/pipeline/v1beta1/swagger.json index e8171739b5e..7da1c2c47ba 100644 --- a/pkg/apis/pipeline/v1beta1/swagger.json +++ b/pkg/apis/pipeline/v1beta1/swagger.json @@ -2168,7 +2168,7 @@ "type": "object", "properties": { "apiVersion": { - "description": "API version of the referent", + "description": "API version of the referent Note: A Task with non-empty APIVersion and Kind is considered a Custom Task", "type": "string" }, "bundle": { @@ -2176,7 +2176,7 @@ "type": "string" }, "kind": { - "description": "TaskKind indicates the kind of the task, namespaced or cluster scoped.", + "description": "TaskKind indicates the Kind of the Task: 1. Namespaced Task when Kind is set to \"Task\" 2. Cluster-Scoped Task when Kind is set to \"ClusterTask\" 3. Custom Task when Kind is non-empty and APIVersion is non-empty", "type": "string" }, "name": { diff --git a/pkg/apis/pipeline/v1beta1/taskref_types.go b/pkg/apis/pipeline/v1beta1/taskref_types.go index 54521a64afd..cfaa9089f38 100644 --- a/pkg/apis/pipeline/v1beta1/taskref_types.go +++ b/pkg/apis/pipeline/v1beta1/taskref_types.go @@ -20,9 +20,13 @@ package v1beta1 type TaskRef struct { // Name of the referent; More info: http://kubernetes.io/docs/user-guide/identifiers#names Name string `json:"name,omitempty"` - // TaskKind indicates the kind of the task, namespaced or cluster scoped. + // TaskKind indicates the Kind of the Task: + // 1. Namespaced Task when Kind is set to "Task" + // 2. Cluster-Scoped Task when Kind is set to "ClusterTask" + // 3. Custom Task when Kind is non-empty and APIVersion is non-empty Kind TaskKind `json:"kind,omitempty"` // API version of the referent + // Note: A Task with non-empty APIVersion and Kind is considered a Custom Task // +optional APIVersion string `json:"apiVersion,omitempty"` // Bundle url reference to a Tekton Bundle. @@ -49,3 +53,8 @@ const ( // ClusterTaskKind indicates that task type has a cluster scope. ClusterTaskKind TaskKind = "ClusterTask" ) + +// IsCustomTask checks whether the reference is to a Custom Task +func (tr *TaskRef) IsCustomTask() bool { + return tr != nil && tr.APIVersion != "" && tr.Kind != "" +} diff --git a/pkg/apis/pipeline/v1beta1/taskref_types_test.go b/pkg/apis/pipeline/v1beta1/taskref_types_test.go new file mode 100644 index 00000000000..145f4e08dfb --- /dev/null +++ b/pkg/apis/pipeline/v1beta1/taskref_types_test.go @@ -0,0 +1,54 @@ +package v1beta1 + +import "testing" + +func TestTaskRef_IsCustomTask(t *testing.T) { + tests := []struct { + name string + tr *TaskRef + want bool + }{{ + name: "not a custom task - apiVersion and Kind are not set", + tr: &TaskRef{ + Name: "foo", + }, + want: false, + }, { + name: "not a custom task - apiVersion is not set", + tr: &TaskRef{ + Name: "foo", + Kind: "Example", + }, + want: false, + }, { + name: "not a custom task - kind is not set", + tr: &TaskRef{ + Name: "foo", + APIVersion: "example/v0", + }, + want: false, + }, { + name: "custom task with name", + tr: &TaskRef{ + Name: "foo", + Kind: "Example", + APIVersion: "example/v0", + }, + want: true, + }, { + name: "custom task without name", + tr: &TaskRef{ + Kind: "Example", + APIVersion: "example/v0", + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.tr.IsCustomTask(); got != tt.want { + t.Errorf("IsCustomTask() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go index 7524c980600..63e06ff429d 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go @@ -574,15 +574,6 @@ func ValidateTaskRunSpecs(p *v1beta1.PipelineSpec, pr *v1beta1.PipelineRun) erro return nil } -func isCustomTask(ctx context.Context, rpt ResolvedPipelineTask) bool { - invalidSpec := rpt.PipelineTask.TaskRef != nil && rpt.PipelineTask.TaskSpec != nil - isTaskRefCustomTask := rpt.PipelineTask.TaskRef != nil && rpt.PipelineTask.TaskRef.APIVersion != "" && - rpt.PipelineTask.TaskRef.Kind != "" - isTaskSpecCustomTask := rpt.PipelineTask.TaskSpec != nil && rpt.PipelineTask.TaskSpec.APIVersion != "" && - rpt.PipelineTask.TaskSpec.Kind != "" - return !invalidSpec && (isTaskRefCustomTask || isTaskSpecCustomTask) -} - // ResolvePipelineTask retrieves a single Task's instance using the getTask to fetch // the spec. If it is unable to retrieve an instance of a referenced Task, it will return // an error, otherwise it returns a list of all the Tasks retrieved. It will retrieve @@ -598,7 +589,7 @@ func ResolvePipelineTask( rpt := ResolvedPipelineTask{ PipelineTask: &pipelineTask, } - rpt.CustomTask = isCustomTask(ctx, rpt) + rpt.CustomTask = rpt.PipelineTask.TaskRef.IsCustomTask() || rpt.PipelineTask.TaskSpec.IsCustomTask() switch { case rpt.IsCustomTask() && rpt.PipelineTask.IsMatrixed(): rpt.RunObjectNames = getNamesOfRuns(pipelineRun.Status.ChildReferences, pipelineTask.Name, pipelineRun.Name, pipelineTask.Matrix.CountCombinations()) diff --git a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go index e69a9e5d44d..53ab1fb75c6 100644 --- a/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go +++ b/pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go @@ -2284,21 +2284,6 @@ func TestIsCustomTask(t *testing.T) { }, }, want: true, - }, { - name: "custom both taskRef and taskSpec", - pt: v1beta1.PipelineTask{ - TaskRef: &v1beta1.TaskRef{ - APIVersion: "example.dev/v0", - Kind: "Sample", - }, - TaskSpec: &v1beta1.EmbeddedTask{ - TypeMeta: runtime.TypeMeta{ - APIVersion: "example.dev/v0", - Kind: "Sample", - }, - }, - }, - want: false, }, { name: "custom taskRef missing kind", pt: v1beta1.PipelineTask{