introduce v1alpha2 api with auto conversion #1529

vdemeester · 2019-11-06T14:42:21Z

Changes

This is related to #1526 and #1379.

This introduces a new api version v1alpha2 while still allowing user
to use v1alpha1 API.

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)
kubectl get returns the highest priority version, aka the most
recent one, here v1alpha2. To make sure any object created from
any version are valid (while getting), we convert them using the
admission webhook (#blackmagic 🧙).
- Object created using v1alpha1 API will automatically be
  converted (see below)
- This has the downsize of not being able to get v1alpha1 object
  anymore (although it should be possible using the client-go and
  some even more #blackmagic 🧙 hack on the webhook side probably)
One benefit of this approach (apis.Convertible) is that once
we only support 1.16 (the release with conversion webhook), this
will be usable almost as is (for the conversion webhook) 👼.

This is done in a quick and dirty way:

Lot's of code is duplicated where it would not be needed, it was
just quicker to do so at that point in time
I completely skipped the variable interpolation validation for
Parameters as those values would/will need to be automatically
transformed too (but it was a bit complicated for what I wanted to
experiment on)
I did not implement the reconcilier part of this. It should be
simple as:
- it will use only one object (the store one, aka v1alpha1)
- it will make the assumption that the objects are fully
  transformed (either by the webhook or in-memory by the
  controller), meaning there will be no handling
  backward-compatible piece of code in the reconcilier (all is
  contained in the Validable, Defaultable and Convertible impl.).
I did not run/fix the tests 😉

Possible next steps are:

Split this into smaller PR.
Introduce a v1alpha2 package early but not exposing it before we
are ready to. This will allow us to not have to rush this for
0.9, and to increment over this in smaller step.
Decide what v1alpha2 should look like — I did this experiment with
TaskSpec.Inputs.Params to TaskSpec.Params but we should decide
what we want to change and target that.
Reduce duplication by reusing anything that didn't change from
v1alpha1 to v1alpha2 in v1alpha1 (aka refering the same type
in go)
Related to this experiment, implement the variable interpolation
validation thing and the reconcilier part too.
Need to figure out if the definition types (Task, Pipeline, …)
need a controller (I don't think so but…)

Example of conversion

#This task deploys with kubectl apply -f <filename>
apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: demo-deploy-kubectl
spec:
  inputs:
    resources:
    - name: workspace
      type: git
    - name: image
      type: image
    params:
    - name: path
      description: Path to the manifest to apply
    - name: yqArg
      description: Okay this is a hack, but I didn't feel right hard-codeing `-d1` down below
    - name: yamlPathToImage
      description: The path to the image to replace in the yaml manifest (arg to yq)
  steps:
  - name: replace-image
    image: mikefarah/yq
    command: ['yq']
    args:
    - "w"
    - "-i"
    - "$(inputs.params.yqArg)"
    - "$kube(inputs.params.path)"
    - "$(inputs.params.yamlPathToImage)"
    - "$(inputs.resources.image.url)"
  - name: run-kubectl
    image: lachlanevenson/k8s-kubectl
    command: ['kubectl']
    args:
    - 'apply'
    - '-f'
    - '$(inputs.params.path)'

becomes the following at creation

#This task deploys with kubectl apply -f <filename>
apiVersion: tekton.dev/v1alpha2
kind: Task
metadata:
  name: demo-deploy-kubectl
spec:
  inputs:
    resources:
    - name: workspace
      type: git
    - name: image
      type: image
  params:
  - name: path
    description: Path to the manifest to apply
  - name: yqArg
    description: Okay this is a hack, but I didn't feel right hard-codeing `-d1` down below
  - name: yamlPathToImage
    description: The path to the image to replace in the yaml manifest (arg to yq)
  steps:
  - name: replace-image
    image: mikefarah/yq
    command: ['yq']
    args:
    - "w"
    - "-i"
    - "$(inputs.params.yqArg)"
    - "$kube(inputs.params.path)"
    - "$(inputs.params.yamlPathToImage)"
    - "$(inputs.resources.image.url)"
  - name: run-kubectl
    image: lachlanevenson/k8s-kubectl
    command: ['kubectl']
    args:
    - 'apply'
    - '-f'
    - '$(inputs.params.path)'

Putting an hold as this is not meant to be merged, ever !

/hold
/cc @bobcatfish @skaegi @imjasonh @afrittoli @abayer

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.

This introduces a new api version `v1alpha2` while still allowing user to use `v1alpha1` API. - 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) - `kubectl` get returns the highest priority version, aka the most recent one, here `v1alpha2`. To make *sure* any object created from any version are valid (while getting), we convert them using the admission webhook (#blackmagic 🧙). - Object created using `v1alpha1` API will automatically be converted (see below) - This has the downsize of not being able to `get` v1alpha1 object anymore (although it should be possible using the `client-go` and some even more #blackmagic 🧙 hack on the webhook side probably) - One benefit of this approach (`apis.Convertible`) is that once we *only* support 1.16 (the release with conversion webhook), this will be usable almost as is (for the conversion webhook) 👼. This is done in a quick and dirty way: - Lot's of code is duplicated where it would not be needed, it was just quicker to do so at that point in time - I completely skipped the *variable interpolation* validation for `Parameters` as those values would/will need to be automatically transformed too (but it was a bit complicated for what I wanted to experiment on) - I did not implement the **reconcilier** part of this. It should be simple as: - it will use only one object (the store one, aka `v1alpha1`) - it will make the assumption that the objects are fully transformed (either by the webhook or in-memory by the controller), meaning there will be no *handling backward-compatible* piece of code in the reconcilier (all is contained in the `Validable`, `Defaultable` and `Convertible` impl.). Possible next steps are: - Split this into smaller PR. - Introduce a `v1alpha2` package early but not exposing it before we are ready to. This will allow us to not have to rush this for 0.9, **and** to increment over this in smaller step. - Decide what `v1alpha2` should look like — I did this experiment with `TaskSpec.Inputs.Params` to `TaskSpec.Params` but we should decide what we want to change and target that. - Reduce duplication by reusing anything that didn't change from `v1alpha1` to `v1alpha2` in `v1alpha1` (aka refering the same type in go) - Related to this experiment, implement the variable interpolation validation thing **and** the reconcilier part too. - Need to figure out if the definition types (`Task`, `Pipeline`, …) need a `controller` (I don't think so but…) Signed-off-by: Vincent Demeester <[email protected]>

tekton-robot · 2019-11-06T14:42:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign vdemeester
You can assign the PR to them by writing /assign @vdemeester in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

vdemeester · 2019-11-06T14:44:11Z

cmd/webhook/main.go

@@ -104,7 +113,7 @@ func main() {

 	// Decorate contexts with the current state of the config.
 	ctxFunc := func(ctx context.Context) context.Context {
-		return v1alpha1.WithDefaultConfigurationName(store.ToContext(ctx))
+		return v1alpha2.WithUpgradeViaDefaulting(store.ToContext(ctx))


This tells the webhook to do the conversion 👼

vdemeester · 2019-11-06T14:44:42Z

config/300-clustertask.yaml

+  versions:
+  - name: v1alpha1
+    served: true
+    storage: true
+  - name: v1alpha2
+    served: true
+    storage: false


v1alpha1 is the stored version, but both are served

tekton-robot · 2019-11-06T14:44:49Z

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/reconciler/taskrun/taskrun.go	72.5%	72.3%	-0.2

vdemeester · 2019-11-06T14:45:12Z

examples/pipelineruns/pipelinerun.yaml

@@ -1,5 +1,5 @@
 ---


This files contains v1alpha2 and v1alpha1 Tasks

vdemeester · 2019-11-06T14:46:31Z

hack/update-codegen.sh

@@ -42,7 +42,7 @@ ${GOPATH}/bin/deepcopy-gen \
 # Knative Injection
 ${KNATIVE_CODEGEN_PKG}/hack/generate-knative.sh "injection" \
  github.com/tektoncd/pipeline/pkg/client github.com/tektoncd/pipeline/pkg/apis \
-  "pipeline:v1alpha1" \
+  "pipeline:v1alpha1,v1alpha2" \


This updates codegen to generate v1alpha1 and v1alpha2 clientset, informers, …

vdemeester · 2019-11-06T14:47:35Z

pkg/apis/pipeline/v1alpha1/task_conversion.go

+	"knative.dev/pkg/apis"
+)
+
+func (source *Task) ConvertUp(ctx context.Context, obj apis.Convertible) error {


This allows us to take a v1alpha1 object and "convert it up" to a more recent version (that will be passed, by the webhook or the reconcilier for in-memory upgrade)

vdemeester · 2019-11-06T14:48:39Z

pkg/apis/pipeline/v1alpha1/task_conversion.go

+}
+
+func (sink *Task) ConvertDown(ctx context.Context, obj apis.Convertible) error {
+	switch source := obj.(type) {


This does the opposite of ConvertUp, it takes a "higher" version of the Task object and converts it "back" to v1alpha1 (hence the need for v1alpha1 types to have all the fields

vdemeester · 2019-11-06T14:50:14Z

pkg/apis/pipeline/v1alpha1/task_defaults.go

+	if v1alpha2.IsUpgradeViaDefaulting(ctx) {
+		fmt.Println("********************************************")
+		v := v1alpha2.TaskSpec{}
+		if ts.ConvertUp(ctx, &v) == nil {
+			fmt.Printf("v: %+v\n", v)
+			fmt.Printf("v.inputs: %+v\n", v.Inputs)
+			fmt.Printf("v.outputs: %+v\n", v.Outputs)
+			alpha1 := TaskSpec{}
+			if alpha1.ConvertDown(ctx, v) == nil {
+				*ts = alpha1
+			}
+		}
+	}
+	fmt.Printf("ts: %+v\n", ts)


This combined with https://github.com/tektoncd/pipeline/pull/1529/files#diff-58de452513b7e8d8d3cfea23eb4ae6a8R116 make the conversion automatic from v1alpha1 to a v1alpha2 compatible object (still stored as v1alpha1)

vdemeester · 2019-11-06T14:51:05Z

pkg/apis/pipeline/v1alpha1/task_types.go

+	// must be supplied as inputs in TaskRuns unless they declare a default
+	// value.
+	// +optional
+	Params []ParamSpec `json:"params,omitempty"`


This fields comes from v1alpha2 "struct" and "might not be documented"

vdemeester · 2019-11-06T14:51:43Z

pkg/apis/pipeline/v1alpha1/task_types.go

 	// must be supplied as inputs in TaskRuns unless they declare a default
 	// value.
 	// +optional
-	Params []ParamSpec `json:"params,omitempty"`
+	DeprecatedParams []ParamSpec `json:"params,omitempty"`


Renaming it makes it easier to know it's deprecated and we shouldn't depend/looking at it in the reconcilier

vdemeester · 2019-11-06T14:52:34Z

pkg/apis/pipeline/v1alpha1/task_validation.go

+	/*
+		 	if err := validateInputParameterVariables(ts.Steps, ts.Inputs); err != nil {
+				return err
+			}
+	*/


As said in the comments, this is commented to skip the "auto" migrate variable interpolation

vdemeester · 2019-11-06T14:56:56Z

btw, this is how knative/serving does it 👼 😉

tekton-robot · 2019-11-06T15:13:57Z

@vdemeester: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-unit-tests	`d75b034`	link	`/test tekton-pipeline-unit-tests`
pull-tekton-pipeline-build-tests	`d75b034`	link	`/test pull-tekton-pipeline-build-tests`
pull-tekton-pipeline-integration-tests	`d75b034`	link	`/test pull-tekton-pipeline-integration-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vdemeester · 2019-11-08T17:22:13Z

Split this into smaller PR.

I'll start tackling this next week, especially the following items 👼

Introduce a v1alpha2 package early but not exposing it before we
are ready to. This will allow us to not have to rush this for
0.9, and to increment over this in smaller step.

Reduce duplication by reusing anything that didn't change from
v1alpha1 to v1alpha2 in v1alpha1 (aka refering the same type
in go)

cccfeng · 2019-11-16T11:59:14Z

examples/pipelineruns/pipelinerun.yaml

@@ -131,7 +131,7 @@ spec:
    - "w"
    - "-i"
    - "$(inputs.params.yqArg)"
-    - "$(inputs.params.path)"
+    - "$kube(inputs.params.path)"


Hi, it looks like this line of code has one more word “kube” .

vdemeester · 2019-11-26T18:11:17Z

I am going to close as split work has started 😉
/close

tekton-robot · 2019-11-26T18:11:19Z

@vdemeester: Closed this PR.

In response to this:

I am going to close as split work has started 😉
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tekton-robot requested review from abayer, afrittoli, bobcatfish and imjasonh November 6, 2019 14:42

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 6, 2019

tekton-robot requested a review from skaegi November 6, 2019 14:42

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Nov 6, 2019

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 6, 2019

vdemeester commented Nov 6, 2019

View reviewed changes

examples/pipelineruns/pipelinerun.yaml

@@ -1,5 +1,5 @@

---

Copy link

Member Author

vdemeester Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This files contains v1alpha2 and v1alpha1 Tasks

vdemeester commented Nov 6, 2019

View reviewed changes

vdemeester mentioned this pull request Nov 7, 2019

Investigate use of an admission webhook to ease move to beta #1379

Closed

vdemeester mentioned this pull request Nov 15, 2019

Add empty v1alpha2 package 🎐 #1578

Merged

3 tasks

cccfeng reviewed Nov 16, 2019

View reviewed changes

tekton-robot closed this Nov 26, 2019

vdemeester mentioned this pull request Feb 6, 2020

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

Merged

3 tasks

vdemeester deleted the 1526-v1alpha2 branch April 23, 2020 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce v1alpha2 api with auto conversion #1529

introduce v1alpha2 api with auto conversion #1529

vdemeester commented Nov 6, 2019 •

edited

Loading

tekton-robot commented Nov 6, 2019

vdemeester Nov 6, 2019

vdemeester Nov 6, 2019

tekton-robot commented Nov 6, 2019

vdemeester Nov 6, 2019

vdemeester Nov 6, 2019

vdemeester Nov 6, 2019

vdemeester Nov 6, 2019

vdemeester Nov 6, 2019

vdemeester Nov 6, 2019

vdemeester Nov 6, 2019

vdemeester Nov 6, 2019

vdemeester commented Nov 6, 2019

tekton-robot commented Nov 6, 2019

vdemeester commented Nov 8, 2019

cccfeng Nov 16, 2019 •

edited

Loading

vdemeester commented Nov 26, 2019

tekton-robot commented Nov 26, 2019

introduce v1alpha2 api with auto conversion #1529

introduce v1alpha2 api with auto conversion #1529

Conversation

vdemeester commented Nov 6, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

tekton-robot commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Nov 6, 2019

tekton-robot commented Nov 6, 2019

vdemeester commented Nov 8, 2019

cccfeng Nov 16, 2019 • edited Loading

Choose a reason for hiding this comment

vdemeester commented Nov 26, 2019

tekton-robot commented Nov 26, 2019

vdemeester commented Nov 6, 2019 •

edited

Loading

cccfeng Nov 16, 2019 •

edited

Loading