From 232c5b269253c1161573b25864a19843b0f90674 Mon Sep 17 00:00:00 2001
From: Eli Zucker <thezuck@google.com>
Date: Tue, 2 Jul 2019 16:45:29 -0400
Subject: [PATCH] Refactor TaskParam and PipelineParam.

Make a new ParamSpec type and place it in its own file along with the
already existing Param type. ParamSpec replaces TaskParam and
PipelineParam.

TaskParam and PipelineParam are identical types, and with future
features including array/string parameters, consolidating
parameter-related code in param_types.go makes sense for better
code-reusability and logical structure.

Additionally, I think "ParamSpec" is a better name; TaskParam and
PipelineParam currently represent a name and description (with
potentially more information in the future such as whether the parameter
is an array or string), while the Param type represents the parameter
value itself. Previously, it was not as intuitive that Param contains
the actual parameter value, not TaskParam/PipelineParam.
---
 pkg/apis/pipeline/v1alpha1/param_types.go     | 40 ++++++++++++++
 pkg/apis/pipeline/v1alpha1/pipeline_types.go  | 17 +-----
 .../pipeline/v1alpha1/pipeline_validation.go  |  2 +-
 pkg/apis/pipeline/v1alpha1/task_types.go      | 27 +---------
 .../pipeline/v1alpha1/task_validation_test.go |  6 +--
 .../v1alpha1/zz_generated.deepcopy.go         | 52 +++++++------------
 .../v1alpha1/taskrun/resources/apply.go       |  2 +-
 .../v1alpha1/taskrun/resources/apply_test.go  |  6 +--
 pkg/reconciler/v1alpha1/taskrun/taskrun.go    |  2 +-
 test/builder/pipeline.go                      | 18 +++----
 test/builder/pipeline_test.go                 |  2 +-
 test/builder/task.go                          | 16 +++---
 test/builder/task_test.go                     |  2 +-
 13 files changed, 88 insertions(+), 104 deletions(-)
 create mode 100644 pkg/apis/pipeline/v1alpha1/param_types.go

diff --git a/pkg/apis/pipeline/v1alpha1/param_types.go b/pkg/apis/pipeline/v1alpha1/param_types.go
new file mode 100644
index 00000000000..48c6e7b4bc9
--- /dev/null
+++ b/pkg/apis/pipeline/v1alpha1/param_types.go
@@ -0,0 +1,40 @@
+/*
+Copyright 2019 The Tekton Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1
+
+// ParamSpec defines arbitrary parameters needed beyond typed inputs (such as
+// resources). Parameter values are provided by users as inputs on a TaskRun
+// or PipelineRun.
+type ParamSpec struct {
+	// Name declares the name by which a parameter is referenced.
+	Name string `json:"name"`
+	// Description is a user-facing description of the parameter that may be
+	// used to populate a UI.
+	// +optional
+	Description string `json:"description,omitempty"`
+	// Default is the value a parameter takes if no input value is supplied. If
+	// default is set, a Task may be executed without a supplied value for the
+	// parameter.
+	// +optional
+	Default string `json:"default,omitempty"`
+}
+
+// Param declares a value to use for the Param called Name.
+type Param struct {
+	Name  string `json:"name"`
+	Value string `json:"value"`
+}
\ No newline at end of file
diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_types.go b/pkg/apis/pipeline/v1alpha1/pipeline_types.go
index 2f89faf7a36..59d1f83cd3a 100644
--- a/pkg/apis/pipeline/v1alpha1/pipeline_types.go
+++ b/pkg/apis/pipeline/v1alpha1/pipeline_types.go
@@ -30,7 +30,7 @@ type PipelineSpec struct {
 	Tasks []PipelineTask `json:"tasks,omitempty"`
 	// Params declares a list of input parameters that must be supplied when
 	// this Pipeline is run.
-	Params []PipelineParam `json:"params,omitempty"`
+	Params []ParamSpec `json:"params,omitempty"`
 }
 
 // PipelineStatus does not contain anything because Pipelines on their own
@@ -107,21 +107,6 @@ type PipelineTaskParam struct {
 	Value string `json:"value"`
 }
 
-// PipelineParam defines an arbitrary parameter needed by a Pipeline beyond typed inputs
-// such as resources.
-type PipelineParam struct {
-	// Name is the name of the parameter.
-	Name string `json:"name"`
-	// Description is an informational description of what the parameter
-	// represents.
-	// +optional
-	Description string `json:"description,omitempty"`
-	// Default specifies the value that this parameter should take if a value is
-	// not specified in a PipelineRun.
-	// +optional
-	Default string `json:"default,omitempty"`
-}
-
 // PipelineDeclaredResource is used by a Pipeline to declare the types of the
 // PipelineResources that it will required to run and names which can be used to
 // refer to these PipelineResources in PipelineTaskResourceBindings.
diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go
index e243677e5be..ce6f6171d8b 100644
--- a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go
+++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go
@@ -149,7 +149,7 @@ func (ps *PipelineSpec) Validate(ctx context.Context) *apis.FieldError {
 	return nil
 }
 
-func validatePipelineParameterVariables(tasks []PipelineTask, params []PipelineParam) *apis.FieldError {
+func validatePipelineParameterVariables(tasks []PipelineTask, params []ParamSpec) *apis.FieldError {
 	parameterNames := map[string]struct{}{}
 	for _, p := range params {
 		parameterNames[p.Name] = struct{}{}
diff --git a/pkg/apis/pipeline/v1alpha1/task_types.go b/pkg/apis/pipeline/v1alpha1/task_types.go
index f2662879b28..00a0dc874c2 100644
--- a/pkg/apis/pipeline/v1alpha1/task_types.go
+++ b/pkg/apis/pipeline/v1alpha1/task_types.go
@@ -103,7 +103,7 @@ type Inputs struct {
 	// must be supplied as inputs in TaskRuns unless they declare a default
 	// value.
 	// +optional
-	Params []TaskParam `json:"params,omitempty"`
+	Params []ParamSpec `json:"params,omitempty"`
 }
 
 // TaskResource defines an input or output Resource declared as a requirement
@@ -127,31 +127,6 @@ type TaskResource struct {
 	OutputImageDir string `json:"outputImageDir"`
 }
 
-// TaskParam defines arbitrary parameters needed by a task beyond typed inputs
-// such as resources. Parameter values are provided by users as inputs on a
-// TaskRun.
-type TaskParam struct {
-	// Name declares the name by which a parameter is referenced in the Task's
-	// definition. Parameters may be referenced by name in the definition of a
-	// Task's steps.
-	Name string `json:"name"`
-	// Description is a user-facing description of the parameter that may be
-	// used to populate a UI.
-	// +optional
-	Description string `json:"description,omitempty"`
-	// Default is the value a parameter takes if no input value is supplied. If
-	// default is set, a Task maybe be executed by a TaskRun that does not
-	// supply a value for the parameter.
-	// +optional
-	Default string `json:"default,omitempty"`
-}
-
-// Param declares a value to use for the Param called Name.
-type Param struct {
-	Name  string `json:"name"`
-	Value string `json:"value"`
-}
-
 // Outputs allow a task to declare what data the Build/Task will be producing,
 // i.e. results such as logs and artifacts such as images.
 type Outputs struct {
diff --git a/pkg/apis/pipeline/v1alpha1/task_validation_test.go b/pkg/apis/pipeline/v1alpha1/task_validation_test.go
index 96c184f9052..e396a9e5dbe 100644
--- a/pkg/apis/pipeline/v1alpha1/task_validation_test.go
+++ b/pkg/apis/pipeline/v1alpha1/task_validation_test.go
@@ -62,7 +62,7 @@ func TestTaskSpecValidate(t *testing.T) {
 		fields: fields{
 			Inputs: &Inputs{
 				Resources: []TaskResource{validResource},
-				Params: []TaskParam{
+				Params: []ParamSpec{
 					{
 						Name:        "task",
 						Description: "param",
@@ -110,7 +110,7 @@ func TestTaskSpecValidate(t *testing.T) {
 					Name: "foo",
 					Type: PipelineResourceTypeImage,
 				}},
-				Params: []TaskParam{{
+				Params: []ParamSpec{{
 					Name: "baz",
 				}, {
 					Name: "foo-is-baz",
@@ -367,7 +367,7 @@ func TestTaskSpecValidateError(t *testing.T) {
 		name: "Inexistent param variable with existing",
 		fields: fields{
 			Inputs: &Inputs{
-				Params: []TaskParam{
+				Params: []ParamSpec{
 					{
 						Name:        "foo",
 						Description: "param",
diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go
index 8768c69d018..d95226a4571 100644
--- a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go
+++ b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go
@@ -265,7 +265,7 @@ func (in *Inputs) DeepCopyInto(out *Inputs) {
 	}
 	if in.Params != nil {
 		in, out := &in.Params, &out.Params
-		*out = make([]TaskParam, len(*in))
+		*out = make([]ParamSpec, len(*in))
 		copy(*out, *in)
 	}
 	return
@@ -364,6 +364,22 @@ func (in *Param) DeepCopy() *Param {
 	return out
 }
 
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
+func (in *ParamSpec) DeepCopyInto(out *ParamSpec) {
+	*out = *in
+	return
+}
+
+// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ParamSpec.
+func (in *ParamSpec) DeepCopy() *ParamSpec {
+	if in == nil {
+		return nil
+	}
+	out := new(ParamSpec)
+	in.DeepCopyInto(out)
+	return out
+}
+
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *Pipeline) DeepCopyInto(out *Pipeline) {
 	*out = *in
@@ -441,22 +457,6 @@ func (in *PipelineList) DeepCopyObject() runtime.Object {
 	return nil
 }
 
-// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
-func (in *PipelineParam) DeepCopyInto(out *PipelineParam) {
-	*out = *in
-	return
-}
-
-// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PipelineParam.
-func (in *PipelineParam) DeepCopy() *PipelineParam {
-	if in == nil {
-		return nil
-	}
-	out := new(PipelineParam)
-	in.DeepCopyInto(out)
-	return out
-}
-
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *PipelineRef) DeepCopyInto(out *PipelineRef) {
 	*out = *in
@@ -873,7 +873,7 @@ func (in *PipelineSpec) DeepCopyInto(out *PipelineSpec) {
 	}
 	if in.Params != nil {
 		in, out := &in.Params, &out.Params
-		*out = make([]PipelineParam, len(*in))
+		*out = make([]ParamSpec, len(*in))
 		copy(*out, *in)
 	}
 	return
@@ -1168,22 +1168,6 @@ func (in *TaskList) DeepCopyObject() runtime.Object {
 	return nil
 }
 
-// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
-func (in *TaskParam) DeepCopyInto(out *TaskParam) {
-	*out = *in
-	return
-}
-
-// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new TaskParam.
-func (in *TaskParam) DeepCopy() *TaskParam {
-	if in == nil {
-		return nil
-	}
-	out := new(TaskParam)
-	in.DeepCopyInto(out)
-	return out
-}
-
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *TaskRef) DeepCopyInto(out *TaskRef) {
 	*out = *in
diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/apply.go b/pkg/reconciler/v1alpha1/taskrun/resources/apply.go
index 9d7d79b6478..967ba3f0a3e 100644
--- a/pkg/reconciler/v1alpha1/taskrun/resources/apply.go
+++ b/pkg/reconciler/v1alpha1/taskrun/resources/apply.go
@@ -25,7 +25,7 @@ import (
 )
 
 // ApplyParameters applies the params from a TaskRun.Input.Parameters to a TaskSpec
-func ApplyParameters(spec *v1alpha1.TaskSpec, tr *v1alpha1.TaskRun, defaults ...v1alpha1.TaskParam) *v1alpha1.TaskSpec {
+func ApplyParameters(spec *v1alpha1.TaskSpec, tr *v1alpha1.TaskRun, defaults ...v1alpha1.ParamSpec) *v1alpha1.TaskSpec {
 	// This assumes that the TaskRun inputs have been validated against what the Task requests.
 	replacements := map[string]string{}
 	// Set all the default replacements
diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/apply_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/apply_test.go
index 75622f2f6db..a6398d4a216 100644
--- a/pkg/reconciler/v1alpha1/taskrun/resources/apply_test.go
+++ b/pkg/reconciler/v1alpha1/taskrun/resources/apply_test.go
@@ -205,7 +205,7 @@ func TestApplyParameters(t *testing.T) {
 	type args struct {
 		ts *v1alpha1.TaskSpec
 		tr *v1alpha1.TaskRun
-		dp []v1alpha1.TaskParam
+		dp []v1alpha1.ParamSpec
 	}
 	tests := []struct {
 		name string
@@ -282,7 +282,7 @@ func TestApplyParameters(t *testing.T) {
 					},
 				},
 			},
-			dp: []v1alpha1.TaskParam{
+			dp: []v1alpha1.ParamSpec{
 				{
 					Name:    "myimage",
 					Default: "replaced-image-name",
@@ -298,7 +298,7 @@ func TestApplyParameters(t *testing.T) {
 		args: args{
 			ts: simpleTaskSpec,
 			tr: &v1alpha1.TaskRun{},
-			dp: []v1alpha1.TaskParam{
+			dp: []v1alpha1.ParamSpec{
 				{
 					Name:    "myimage",
 					Default: "mydefault",
diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go
index 1b889f0a1c2..d403c4bc601 100644
--- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go
+++ b/pkg/reconciler/v1alpha1/taskrun/taskrun.go
@@ -461,7 +461,7 @@ func (c *Reconciler) createPod(tr *v1alpha1.TaskRun, rtr *resources.ResolvedTask
 		return nil, xerrors.Errorf("couldn't create redirected TaskSpec: %w", err)
 	}
 
-	var defaults []v1alpha1.TaskParam
+	var defaults []v1alpha1.ParamSpec
 	if ts.Inputs != nil {
 		defaults = append(defaults, ts.Inputs.Params...)
 	}
diff --git a/test/builder/pipeline.go b/test/builder/pipeline.go
index 730080f31f2..1610c11ed6a 100644
--- a/test/builder/pipeline.go
+++ b/test/builder/pipeline.go
@@ -28,8 +28,8 @@ type PipelineOp func(*v1alpha1.Pipeline)
 // PipelineSpecOp is an operation which modify a PipelineSpec struct.
 type PipelineSpecOp func(*v1alpha1.PipelineSpec)
 
-// PipelineParamOp is an operation which modify a PipelineParam struct.
-type PipelineParamOp func(*v1alpha1.PipelineParam)
+// PipelineParamOp is an operation which modify a ParamSpec struct.
+type PipelineParamOp func(*v1alpha1.ParamSpec)
 
 // PipelineTaskOp is an operation which modify a PipelineTask struct.
 type PipelineTaskOp func(*v1alpha1.PipelineTask)
@@ -110,11 +110,11 @@ func PipelineDeclaredResource(name string, t v1alpha1.PipelineResourceType) Pipe
 	}
 }
 
-// PipelineParam adds a param, with specified name, to the Spec.
-// Any number of PipelineParam modifiers can be passed to transform it.
+// ParamSpec adds a param, with specified name, to the Spec.
+// Any number of ParamSpec modifiers can be passed to transform it.
 func PipelineParam(name string, ops ...PipelineParamOp) PipelineSpecOp {
 	return func(ps *v1alpha1.PipelineSpec) {
-		pp := &v1alpha1.PipelineParam{Name: name}
+		pp := &v1alpha1.ParamSpec{Name: name}
 		for _, op := range ops {
 			op(pp)
 		}
@@ -122,16 +122,16 @@ func PipelineParam(name string, ops ...PipelineParamOp) PipelineSpecOp {
 	}
 }
 
-// PipelineParamDescription sets the description to the PipelineParam.
+// PipelineParamDescription sets the description to the ParamSpec.
 func PipelineParamDescription(desc string) PipelineParamOp {
-	return func(pp *v1alpha1.PipelineParam) {
+	return func(pp *v1alpha1.ParamSpec) {
 		pp.Description = desc
 	}
 }
 
-// PipelineParamDefault sets the default value to the PipelineParam.
+// PipelineParamDefault sets the default value to the ParamSpec.
 func PipelineParamDefault(value string) PipelineParamOp {
-	return func(pp *v1alpha1.PipelineParam) {
+	return func(pp *v1alpha1.ParamSpec) {
 		pp.Default = value
 	}
 }
diff --git a/test/builder/pipeline_test.go b/test/builder/pipeline_test.go
index cfa2c630740..0de6130654f 100644
--- a/test/builder/pipeline_test.go
+++ b/test/builder/pipeline_test.go
@@ -59,7 +59,7 @@ func TestPipeline(t *testing.T) {
 				Name: "my-only-image-resource",
 				Type: "image",
 			}},
-			Params: []v1alpha1.PipelineParam{{
+			Params: []v1alpha1.ParamSpec{{
 				Name:        "first-param",
 				Default:     "default-value",
 				Description: "default description",
diff --git a/test/builder/task.go b/test/builder/task.go
index 0c1b6c137e6..6b7dd9c9caa 100644
--- a/test/builder/task.go
+++ b/test/builder/task.go
@@ -38,8 +38,8 @@ type InputsOp func(*v1alpha1.Inputs)
 // OutputsOp is an operation which modify an Outputs struct.
 type OutputsOp func(*v1alpha1.Outputs)
 
-// TaskParamOp is an operation which modify a TaskParam struct.
-type TaskParamOp func(*v1alpha1.TaskParam)
+// TaskParamOp is an operation which modify a ParamSpec struct.
+type TaskParamOp func(*v1alpha1.ParamSpec)
 
 // TaskRunOp is an operation which modify a TaskRun struct.
 type TaskRunOp func(*v1alpha1.TaskRun)
@@ -249,10 +249,10 @@ func OutputsResource(name string, resourceType v1alpha1.PipelineResourceType) Ou
 }
 
 // InputsParam adds a param, with specified name, to the Inputs.
-// Any number of TaskParam modifier can be passed to transform it.
+// Any number of ParamSpec modifier can be passed to transform it.
 func InputsParam(name string, ops ...TaskParamOp) InputsOp {
 	return func(i *v1alpha1.Inputs) {
-		tp := &v1alpha1.TaskParam{Name: name}
+		tp := &v1alpha1.ParamSpec{Name: name}
 		for _, op := range ops {
 			op(tp)
 		}
@@ -260,16 +260,16 @@ func InputsParam(name string, ops ...TaskParamOp) InputsOp {
 	}
 }
 
-// ParamDescripiton sets the description to the TaskParam.
+// ParamDescripiton sets the description to the ParamSpec.
 func ParamDescription(desc string) TaskParamOp {
-	return func(tp *v1alpha1.TaskParam) {
+	return func(tp *v1alpha1.ParamSpec) {
 		tp.Description = desc
 	}
 }
 
-// ParamDefault sets the default value to the TaskParam.
+// ParamDefault sets the default value to the ParamSpec.
 func ParamDefault(value string) TaskParamOp {
-	return func(tp *v1alpha1.TaskParam) {
+	return func(tp *v1alpha1.ParamSpec) {
 		tp.Default = value
 	}
 }
diff --git a/test/builder/task_test.go b/test/builder/task_test.go
index 838b367f9f8..fef1b1e24b2 100644
--- a/test/builder/task_test.go
+++ b/test/builder/task_test.go
@@ -72,7 +72,7 @@ func TestTask(t *testing.T) {
 					Type:       v1alpha1.PipelineResourceTypeGit,
 					TargetPath: "/foo/bar",
 				}},
-				Params: []v1alpha1.TaskParam{{Name: "param", Description: "mydesc", Default: "default"}},
+				Params: []v1alpha1.ParamSpec{{Name: "param", Description: "mydesc", Default: "default"}},
 			},
 			Outputs: &v1alpha1.Outputs{
 				Resources: []v1alpha1.TaskResource{{