Task, ClusterTask and Pipeline auto-conversion from v1alpha1 to v1alpha2 🎋 #2002

vdemeester · 2020-02-05T11:19:53Z

Changes

This adds:

auto-conversion methods and tests for Task types in v1alpha1
and v1alpha2. In order to reduce the number of duplicated fields, we
are using v1alpha2.TaskSpec embedded in v1alpha1.TaskSpec so that both
types are compatible.
auto-conversion methods and tests for ClusterTask
auto-conversion methods and tests for Pipeline

/cc @sbwsg @bobcatfish @afrittoli

Signed-off-by: Vincent Demeester [email protected]

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

vdemeester · 2020-02-05T11:20:26Z

pkg/apis/pipeline/v1alpha1/pipeline_conversion.go

+	switch sink := obj.(type) {
+	case *v1alpha2.Pipeline:
+		sink.ObjectMeta = source.ObjectMeta
+		return nil // source.Spec.ConvertUp(ctx, &sink.Spec)


This is a placeholder 👼 I needed to define at least 2 Convertible types to some tests 😝

vdemeester · 2020-02-05T11:21:48Z

pkg/apis/pipeline/v1alpha1/task_types.go

@@ -46,6 +45,8 @@ func (t *Task) Copy() TaskInterface {

 // TaskSpec defines the desired state of Task.
 type TaskSpec struct {
+	v1alpha2.TaskSpec `json:",inline"`


This is not required per-se, it just simplify the conversion using embedded struct and it doesn't change on the api side (except user of the go library)

If you look at Pipeline, it is not using embedding as we can't really do that (same task field targeting different type)

aha, so this is how TaskSpec.ConvertDown is able to copy into the v1alpha1.Params and Resources fields?

👍

tekton-robot · 2020-02-05T11:29:11Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha2/pipeline_conversion.go	Do not exist	0.0%
pkg/apis/pipeline/v1alpha2/task_conversion.go	Do not exist	100.0%

tekton-robot · 2020-02-05T11:58:10Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha2/pipeline_conversion.go	Do not exist	0.0%
pkg/apis/pipeline/v1alpha2/task_conversion.go	Do not exist	100.0%

tekton-robot · 2020-02-05T14:10:42Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha2/pipeline_conversion.go	Do not exist	0.0%
pkg/apis/pipeline/v1alpha2/task_conversion.go	Do not exist	100.0%

tekton-robot · 2020-02-05T15:22:42Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha2/pipeline_conversion.go	Do not exist	0.0%
pkg/apis/pipeline/v1alpha2/task_conversion.go	Do not exist	100.0%

tekton-robot · 2020-02-05T15:24:21Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha2/pipeline_conversion.go	Do not exist	0.0%
pkg/apis/pipeline/v1alpha2/task_conversion.go	Do not exist	100.0%

ghost · 2020-02-05T18:40:43Z

pkg/apis/pipeline/v1alpha1/task_conversion.go

+				sink.Resources = &v1alpha2.TaskResources{}
+			}
+			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


ghost · 2020-02-05T18:40:55Z

pkg/apis/pipeline/v1alpha1/task_conversion.go

+				// 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([]v1alpha2.TaskResource, len(source.Inputs.Resources))


ghost · 2020-02-05T18:44:25Z

pkg/apis/pipeline/v1alpha1/task_types.go

@@ -46,6 +45,8 @@ func (t *Task) Copy() TaskInterface {

 // TaskSpec defines the desired state of Task.
 type TaskSpec struct {
+	v1alpha2.TaskSpec `json:",inline"`


aha, so this is how TaskSpec.ConvertDown is able to copy into the v1alpha1.Params and Resources fields?

👍

tekton-robot · 2020-02-05T18:50:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbwsg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ghost · 2020-02-05T18:50:37Z

I don't totally understand the need for v1alpha1 ConvertDown funcs. What is their use?

vdemeester · 2020-02-06T07:52:42Z

I don't totally understand the need for v1alpha1 ConvertDown funcs. What is their use?

I did write about this in the initial pull request #1529 :

Only one version of the CRD can be stored, so v1alpha1 is the one
stored
All the version of the CRD must be schema compatible (before 1.16)
- v1alpha1 gets all the fields, meaning :
  - the new ones (from v1alpha2, …)
  - the old deprecated ones (that will be removed from v1alpha2)
- we may only document the "frozen" v1alpha1 field (and not the
  new one that are coming from the other api versions)

The main reason to ConvertDown is because we do store it in v1alpha1. So if a user send a v1alpha1 struct, it will convert it up and down, to be v1alpha2 but still stored as a v1alpha1 struct (kinda).

tekton-robot · 2020-02-06T09:05:37Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1alpha2/cluster_task_conversion.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha2/pipeline_conversion.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha2/task_conversion.go	Do not exist	100.0%

tekton-robot · 2020-02-06T09:45:17Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1alpha2/cluster_task_conversion.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha2/pipeline_conversion.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha2/task_conversion.go	Do not exist	100.0%

ghost · 2020-02-06T13:00:43Z

I did write about this in the initial pull request #1529 :

👼

The main reason to ConvertDown is because we do store it in v1alpha1. So if a user send a v1alpha1 struct, it will convert it up and down, to be v1alpha2 but still stored as a v1alpha1 struct (kinda).

🤯 Brilliant. Thanks!

chmouel · 2020-02-06T13:58:35Z

the golang ci test is fallaciously green since it is actually failing but exit 0

0206 09:56:42.536] ===============================
I0206 09:56:42.536] ==== RUNNING GOLANGCI-LINT ====
I0206 09:56:42.536] ===============================
I0206 09:57:44.392] level=warning msg="[runner] Can't run linter goanalysis_metalinter: ctrlflow: failed prerequisites: [email protected]/tektoncd/pipeline/pkg/artifacts [github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1.test]"

This seems a recurring errors in golang-ci which i have updated the bug with our issue in here :

golangci/golangci-lint#827 (comment)

tekton-robot · 2020-02-06T14:02:28Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1alpha2/cluster_task_conversion.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha2/pipeline_conversion.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha2/task_conversion.go	Do not exist	100.0%

tekton-robot · 2020-02-07T14:26:29Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1alpha2/cluster_task_conversion.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha2/pipeline_conversion.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha2/task_conversion.go	Do not exist	100.0%

vdemeester · 2020-02-07T14:56:32Z

/test pull-tekton-pipeline-integration-tests

ernado · 2020-02-08T13:32:44Z

Hey, can you please check what actual golangci version are you using?
I suspect that is something like v1.21, but there is already v1.23.3 with important fixes.

vdemeester · 2020-02-10T09:34:55Z

Hey, can you please check what actual golangci version are you using?
I suspect that is something like v1.21, but there is already v1.23.3 with important fixes.

See tektoncd/plumbing#216 👼

chmouel · 2020-02-11T09:33:14Z

/lgtm

This adds auto-conversion methods and tests for Task types in v1alpha1 and v1alpha2. In order to reduce the number of duplicated fields, we are using v1alpha2.TaskSpec embedded in v1alpha1.TaskSpec so that both types are compatible. Signed-off-by: Vincent Demeester <[email protected]>

This adds auto-conversion methods and tests for Pipeline types in v1alpha1 and v1alpha2. Signed-off-by: Vincent Demeester <[email protected]>

This adds auto-conversion methods and tests for ClusterTask types in v1alpha1 and v1alpha2. Signed-off-by: Vincent Demeester <[email protected]>

The main reason is to not run into error forcing us to use the same receiver name for all function. The receiver name are different for a good reason. Signed-off-by: Vincent Demeester <[email protected]>

tekton-robot · 2020-02-11T09:56:04Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage
pkg/apis/pipeline/v1alpha2/cluster_task_conversion.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha2/pipeline_conversion.go	Do not exist	100.0%
pkg/apis/pipeline/v1alpha2/task_conversion.go	Do not exist	100.0%

chmouel · 2020-02-11T09:58:26Z

/lgtm

tekton-robot requested review from afrittoli, bobcatfish and a user February 5, 2020 11:19

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Feb 5, 2020

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 5, 2020

vdemeester commented Feb 5, 2020

View reviewed changes

vdemeester force-pushed the v1alpha2-auto-conversion branch from 15a7bd5 to f71d174 Compare February 5, 2020 11:26

vdemeester force-pushed the v1alpha2-auto-conversion branch from f71d174 to eb8a6f2 Compare February 5, 2020 11:55

vdemeester force-pushed the v1alpha2-auto-conversion branch from eb8a6f2 to 5371f99 Compare February 5, 2020 14:08

vdemeester force-pushed the v1alpha2-auto-conversion branch from 90e16b2 to 71fc0d5 Compare February 5, 2020 15:21

vdemeester changed the title ~~Task auto-conversion from v1alpha1 to v1alpha2 🎋~~ Task and Pipeline auto-conversion from v1alpha1 to v1alpha2 🎋 Feb 5, 2020

ghost approved these changes Feb 5, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2020

vdemeester force-pushed the v1alpha2-auto-conversion branch from 71fc0d5 to dbe103c Compare February 6, 2020 09:01

vdemeester changed the title ~~Task and Pipeline auto-conversion from v1alpha1 to v1alpha2 🎋~~ Task, ClusterTask and Pipeline auto-conversion from v1alpha1 to v1alpha2 🎋 Feb 6, 2020

vdemeester force-pushed the v1alpha2-auto-conversion branch from dbe103c to 01901bd Compare February 6, 2020 09:42

chmouel mentioned this pull request Feb 6, 2020

Can't run linter goanalysis_metalinter: failed prerequisites golangci/golangci-lint#827

Closed

3 tasks

vdemeester force-pushed the v1alpha2-auto-conversion branch from 01901bd to 50cf5e7 Compare February 6, 2020 13:59

vdemeester force-pushed the v1alpha2-auto-conversion branch from 50cf5e7 to 1c2273e Compare February 7, 2020 14:24

vdemeester mentioned this pull request Feb 10, 2020

TaskRun, and PipelineRun auto-conversion from v1alpha1 to v1alpha2 🎋 #2025

Merged

3 tasks

tekton-robot assigned chmouel Feb 11, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2020

vdemeester added 4 commits February 11, 2020 10:50

Pipeline auto-conversion from v1alpha1 to v1alpha2 🎋

bc0413d

This adds auto-conversion methods and tests for Pipeline types in v1alpha1 and v1alpha2. Signed-off-by: Vincent Demeester <[email protected]>

ClusterTask auto-conversion from v1alpha1 to v1alpha2 🎋

c815d35

This adds auto-conversion methods and tests for ClusterTask types in v1alpha1 and v1alpha2. Signed-off-by: Vincent Demeester <[email protected]>

Add nolint: golint to conversion file 👃

620c476

The main reason is to not run into error forcing us to use the same receiver name for all function. The receiver name are different for a good reason. Signed-off-by: Vincent Demeester <[email protected]>

vdemeester force-pushed the v1alpha2-auto-conversion branch from 1c2273e to 620c476 Compare February 11, 2020 09:53

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2020

tekton-robot merged commit 94d18e6 into tektoncd:master Feb 11, 2020

vdemeester deleted the v1alpha2-auto-conversion branch February 11, 2020 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task, ClusterTask and Pipeline auto-conversion from v1alpha1 to v1alpha2 🎋 #2002

Task, ClusterTask and Pipeline auto-conversion from v1alpha1 to v1alpha2 🎋 #2002

vdemeester commented Feb 5, 2020 •

edited

Loading

vdemeester Feb 5, 2020

vdemeester Feb 5, 2020

vdemeester Feb 5, 2020

ghost Feb 5, 2020

tekton-robot commented Feb 5, 2020

tekton-robot commented Feb 5, 2020

tekton-robot commented Feb 5, 2020

tekton-robot commented Feb 5, 2020

tekton-robot commented Feb 5, 2020

ghost Feb 5, 2020

ghost Feb 5, 2020

ghost Feb 5, 2020

tekton-robot commented Feb 5, 2020

ghost commented Feb 5, 2020

vdemeester commented Feb 6, 2020

tekton-robot commented Feb 6, 2020

tekton-robot commented Feb 6, 2020

ghost commented Feb 6, 2020

chmouel commented Feb 6, 2020

tekton-robot commented Feb 6, 2020

tekton-robot commented Feb 7, 2020

vdemeester commented Feb 7, 2020

ernado commented Feb 8, 2020

vdemeester commented Feb 10, 2020

chmouel commented Feb 11, 2020

tekton-robot commented Feb 11, 2020

chmouel commented Feb 11, 2020

Task, ClusterTask and Pipeline auto-conversion from v1alpha1 to v1alpha2 🎋 #2002

Task, ClusterTask and Pipeline auto-conversion from v1alpha1 to v1alpha2 🎋 #2002

Conversation

vdemeester commented Feb 5, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

vdemeester Feb 5, 2020

Choose a reason for hiding this comment

vdemeester Feb 5, 2020

Choose a reason for hiding this comment

vdemeester Feb 5, 2020

Choose a reason for hiding this comment

ghost Feb 5, 2020

Choose a reason for hiding this comment

tekton-robot commented Feb 5, 2020

tekton-robot commented Feb 5, 2020

tekton-robot commented Feb 5, 2020

tekton-robot commented Feb 5, 2020

tekton-robot commented Feb 5, 2020

ghost Feb 5, 2020

Choose a reason for hiding this comment

ghost Feb 5, 2020

Choose a reason for hiding this comment

ghost Feb 5, 2020

Choose a reason for hiding this comment

tekton-robot commented Feb 5, 2020

ghost commented Feb 5, 2020

vdemeester commented Feb 6, 2020

tekton-robot commented Feb 6, 2020

tekton-robot commented Feb 6, 2020

ghost commented Feb 6, 2020

chmouel commented Feb 6, 2020

tekton-robot commented Feb 6, 2020

tekton-robot commented Feb 7, 2020

vdemeester commented Feb 7, 2020

ernado commented Feb 8, 2020

vdemeester commented Feb 10, 2020

chmouel commented Feb 11, 2020

tekton-robot commented Feb 11, 2020

chmouel commented Feb 11, 2020

vdemeester commented Feb 5, 2020 •

edited

Loading