Fix re-applying v1beta1 TaskRun failing 🚢

Re-apply a v1beta1 resource would failed because of `inputs` (or `outputs`) not a valid field. This is mainly because we are using `v1alpha1` as stored version and thus getting the struct as v1alpha1 internally. This fixes this issue by marking `TaskRun.Spec.Inputs` and `TaskRun.Spec.Outputs` as pointers. It doesn't change the API, but fixes the problem as a `nil` pointer will not be unmarshall where the struct was. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
jagathprakash · Mar 25, 2020 · c53d549 · c53d549
1 parent 9c26b91
commit c53d549
Show file tree

Hide file tree

Showing 8 changed files with 65 additions and 50 deletions.
diff --git a/go.mod b/go.mod
@@ -4,7 +4,6 @@ go 1.13
 
 require (
 	cloud.google.com/go v0.47.0 // indirect
-	cloud.google.com/go/storage v1.0.0
 	contrib.go.opencensus.io/exporter/stackdriver v0.12.8 // indirect
 	github.com/GoogleCloudPlatform/cloud-builders/gcs-fetcher v0.0.0-20191203181535-308b93ad1f39
 	github.com/cloudevents/sdk-go/v2 v2.0.0-preview6
@@ -43,7 +42,6 @@ require (
 	golang.org/x/time v0.0.0-20191024005414-555d28b269f0 // indirect
 	golang.org/x/tools v0.0.0-20200214144324-88be01311a71 // indirect
 	gomodules.xyz/jsonpatch/v2 v2.1.0 // indirect
-	google.golang.org/api v0.15.0
 	google.golang.org/appengine v1.6.5 // indirect
 	k8s.io/api v0.17.3
 	k8s.io/apiextensions-apiserver v0.17.3 // indirect

diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_conversion.go b/pkg/apis/pipeline/v1alpha1/taskrun_conversion.go
@@ -58,37 +58,40 @@ func (source *TaskRunSpec) ConvertTo(ctx context.Context, sink *v1beta1.TaskRunS
 	sink.Params = source.Params
 	sink.Resources = source.Resources
 	// Deprecated fields
-	if len(source.Inputs.Params) > 0 && len(source.Params) > 0 {
-		// This shouldn't happen as it shouldn't pass validation
-		return apis.ErrMultipleOneOf("inputs.params", "params")
-	}
-	if len(source.Inputs.Params) > 0 {
-		sink.Params = make([]v1beta1.Param, len(source.Inputs.Params))
-		for i, param := range source.Inputs.Params {
-			sink.Params[i] = *param.DeepCopy()
-		}
-	}
-	if len(source.Inputs.Resources) > 0 {
-		if sink.Resources == nil {
-			sink.Resources = &v1beta1.TaskRunResources{}
+	if source.Inputs != nil {
+		if len(source.Inputs.Params) > 0 && len(source.Params) > 0 {
+			// This shouldn't happen as it shouldn't pass validation
+			return apis.ErrMultipleOneOf("inputs.params", "params")
 		}
-		if len(source.Inputs.Resources) > 0 && source.Resources != nil && len(source.Resources.Inputs) > 0 {
-			// This shouldn't happen as it shouldn't pass validation but just in case
-			return apis.ErrMultipleOneOf("inputs.resources", "resources.inputs")
+		if len(source.Inputs.Params) > 0 {
+			sink.Params = make([]v1beta1.Param, len(source.Inputs.Params))
+			for i, param := range source.Inputs.Params {
+				sink.Params[i] = *param.DeepCopy()
+			}
 		}
-		sink.Resources.Inputs = make([]v1beta1.TaskResourceBinding, len(source.Inputs.Resources))
-		for i, resource := range source.Inputs.Resources {
-			sink.Resources.Inputs[i] = v1beta1.TaskResourceBinding{
-				PipelineResourceBinding: v1beta1.PipelineResourceBinding{
-					Name:         resource.Name,
-					ResourceRef:  resource.ResourceRef,
-					ResourceSpec: resource.ResourceSpec,
-				},
-				Paths: resource.Paths,
+		if len(source.Inputs.Resources) > 0 {
+			if sink.Resources == nil {
+				sink.Resources = &v1beta1.TaskRunResources{}
+			}
+			if len(source.Inputs.Resources) > 0 && source.Resources != nil && len(source.Resources.Inputs) > 0 {
+				// This shouldn't happen as it shouldn't pass validation but just in case
+				return apis.ErrMultipleOneOf("inputs.resources", "resources.inputs")
+			}
+			sink.Resources.Inputs = make([]v1beta1.TaskResourceBinding, len(source.Inputs.Resources))
+			for i, resource := range source.Inputs.Resources {
+				sink.Resources.Inputs[i] = v1beta1.TaskResourceBinding{
+					PipelineResourceBinding: v1beta1.PipelineResourceBinding{
+						Name:         resource.Name,
+						ResourceRef:  resource.ResourceRef,
+						ResourceSpec: resource.ResourceSpec,
+					},
+					Paths: resource.Paths,
+				}
 			}
 		}
 	}
-	if len(source.Outputs.Resources) > 0 {
+
+	if source.Outputs != nil && len(source.Outputs.Resources) > 0 {
 		if sink.Resources == nil {
 			sink.Resources = &v1beta1.TaskRunResources{}
 		}

diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_conversion_test.go b/pkg/apis/pipeline/v1alpha1/taskrun_conversion_test.go
@@ -173,7 +173,7 @@ func TestTaskRunConversion(t *testing.T) {
 					Name:  "p1",
 					Value: v1beta1.ArrayOrString{StringVal: "baz"},
 				}},
-				Inputs: TaskRunInputs{
+				Inputs: &TaskRunInputs{
 					Params: []Param{{
 						Name:  "p2",
 						Value: v1beta1.ArrayOrString{StringVal: "bar"}},
@@ -215,7 +215,7 @@ func TestTaskRunConversion(t *testing.T) {
 						Paths: []string{"foo", "bar"},
 					}},
 				},
-				Inputs: TaskRunInputs{
+				Inputs: &TaskRunInputs{
 					Resources: []TaskResourceBinding{{
 						PipelineResourceBinding: PipelineResourceBinding{
 							Name:        "i1",
@@ -260,7 +260,7 @@ func TestTaskRunConversion(t *testing.T) {
 						Paths: []string{"foo", "bar"},
 					}},
 				},
-				Outputs: TaskRunOutputs{
+				Outputs: &TaskRunOutputs{
 					Resources: []TaskResourceBinding{{
 						PipelineResourceBinding: PipelineResourceBinding{
 							Name:        "o1",
@@ -314,7 +314,7 @@ func TestTaskRunConversionFromDeprecated(t *testing.T) {
 				Generation: 1,
 			},
 			Spec: TaskRunSpec{
-				Inputs: TaskRunInputs{
+				Inputs: &TaskRunInputs{
 					Params: []Param{{
 						Name:  "p2",
 						Value: v1beta1.ArrayOrString{StringVal: "bar"}},
@@ -344,7 +344,7 @@ func TestTaskRunConversionFromDeprecated(t *testing.T) {
 				Generation: 1,
 			},
 			Spec: TaskRunSpec{
-				Inputs: TaskRunInputs{
+				Inputs: &TaskRunInputs{
 					Resources: []TaskResourceBinding{{
 						PipelineResourceBinding: PipelineResourceBinding{
 							Name:        "i1",
@@ -382,7 +382,7 @@ func TestTaskRunConversionFromDeprecated(t *testing.T) {
 				Generation: 1,
 			},
 			Spec: TaskRunSpec{
-				Outputs: TaskRunOutputs{
+				Outputs: &TaskRunOutputs{
 					Resources: []TaskResourceBinding{{
 						PipelineResourceBinding: PipelineResourceBinding{
 							Name:        "o1",

diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_types.go b/pkg/apis/pipeline/v1alpha1/taskrun_types.go
@@ -56,9 +56,9 @@ type TaskRunSpec struct {
 	Resources *v1beta1.TaskRunResources `json:"resources,omitempty"`
 	// Deprecated
 	// +optional
-	Inputs TaskRunInputs `json:"inputs,omitempty"`
+	Inputs *TaskRunInputs `json:"inputs,omitempty"`
 	// +optional
-	Outputs TaskRunOutputs `json:"outputs,omitempty"`
+	Outputs *TaskRunOutputs `json:"outputs,omitempty"`
 }
 
 // TaskRunSpecStatus defines the taskrun spec status the user can provide

diff --git a/pkg/apis/pipeline/v1alpha1/taskrun_validation.go b/pkg/apis/pipeline/v1alpha1/taskrun_validation.go
@@ -61,14 +61,18 @@ func (ts *TaskRunSpec) Validate(ctx context.Context) *apis.FieldError {
 
 	// Deprecated
 	// check for input resources
-	if err := ts.Inputs.Validate(ctx, "spec.Inputs"); err != nil {
-		return err
+	if ts.Inputs != nil {
+		if err := ts.Inputs.Validate(ctx, "spec.Inputs"); err != nil {
+			return err
+		}
 	}
 
 	// Deprecated
 	// check for output resources
-	if err := ts.Outputs.Validate(ctx, "spec.Outputs"); err != nil {
-		return err
+	if ts.Outputs != nil {
+		if err := ts.Outputs.Validate(ctx, "spec.Outputs"); err != nil {
+			return err
+		}
 	}
 
 	// Validate Resources

diff --git a/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/pipeline/v1alpha1/zz_generated.deepcopy.go
diff --git a/test/builder/task.go b/test/builder/task.go
@@ -774,11 +774,12 @@ func TaskRunResourcesOutput(name string, ops ...TaskResourceBindingOp) TaskRunRe
 // Any number of TaskRunInputs modifier can be passed to transform it.
 func TaskRunInputs(ops ...TaskRunInputsOp) TaskRunSpecOp {
 	return func(spec *v1alpha1.TaskRunSpec) {
-		inputs := &spec.Inputs
+		if spec.Inputs == nil {
+			spec.Inputs = &v1alpha1.TaskRunInputs{}
+		}
 		for _, op := range ops {
-			op(inputs)
+			op(spec.Inputs)
 		}
-		spec.Inputs = *inputs
 	}
 }
 
@@ -843,11 +844,12 @@ func TaskResourceBindingPaths(paths ...string) TaskResourceBindingOp {
 // Any number of TaskRunOutputs modifier can be passed to transform it.
 func TaskRunOutputs(ops ...TaskRunOutputsOp) TaskRunSpecOp {
 	return func(spec *v1alpha1.TaskRunSpec) {
-		outputs := &spec.Outputs
+		if spec.Outputs == nil {
+			spec.Outputs = &v1alpha1.TaskRunOutputs{}
+		}
 		for _, op := range ops {
-			op(outputs)
+			op(spec.Outputs)
 		}
-		spec.Outputs = *outputs
 	}
 }
 

diff --git a/test/builder/task_test.go b/test/builder/task_test.go
@@ -231,7 +231,7 @@ func TestTaskRunWithTaskRef(t *testing.T) {
 			Annotations: map[string]string{},
 		},
 		Spec: v1alpha1.TaskRunSpec{
-			Inputs: v1alpha1.TaskRunInputs{
+			Inputs: &v1alpha1.TaskRunInputs{
 				Resources: []v1alpha1.TaskResourceBinding{{
 					PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
 						Name: "git-resource",
@@ -256,7 +256,7 @@ func TestTaskRunWithTaskRef(t *testing.T) {
 					Value: *tb.ArrayOrString("array", "values"),
 				}},
 			},
-			Outputs: v1alpha1.TaskRunOutputs{
+			Outputs: &v1alpha1.TaskRunOutputs{
 				Resources: []v1alpha1.TaskResourceBinding{{
 					PipelineResourceBinding: v1alpha1.PipelineResourceBinding{
 						Name: "git-resource",