Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the storage version to v1beta1 types #2577

Merged
merged 1 commit into from
May 12, 2020

Conversation

vdemeester
Copy link
Member

@vdemeester vdemeester commented May 7, 2020

Changes

This changes the stored version to v1beta1 👼 This will reduce some confusion on where to add fields and how to work with which version in most of the tektoncd/pipeline code.
Still a little bit of work to do:

Closes #2410

/kind cleanup

Submitter Checklist

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

See the contribution guide for more details.

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

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.

Release Notes

Change the storage version to v1beta1 types

@tekton-robot tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels May 7, 2020
@tekton-robot tekton-robot requested review from bobcatfish and dlorenc May 7, 2020 18:01
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 7, 2020
@@ -22,7 +22,6 @@ import (
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
Copy link
Member Author

Choose a reason for hiding this comment

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

We should never import pkg/reconciler packages outside of pkg/reconciler 😈

Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.6% 2.9
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 72.4% 72.5% 0.1
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7

@ghost
Copy link

ghost commented May 8, 2020

very exciting. cant wait for v1beta1 everywhere!

@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch from d378b44 to 2ac5e0e Compare May 11, 2020 10:09
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.6% 2.9
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 34.1% -40.6
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch from 2ac5e0e to c76feb3 Compare May 11, 2020 10:28
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.6% 2.9
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 75.5% 0.8
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch from c76feb3 to 9c16f2a Compare May 11, 2020 10:37
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.6% 2.9
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 75.5% 0.8
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch from 9c16f2a to 55427ec Compare May 11, 2020 10:47
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.6% 2.9
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 75.5% 0.8
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch from 55427ec to c71660e Compare May 11, 2020 11:22
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.6% 2.9
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 75.5% 0.8
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

@vdemeester
Copy link
Member Author

# github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun/resources
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go:266:39: undefined: v1alpha1

I don't understand 🤔

@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch 2 times, most recently from aa8e64c to 5b92e22 Compare May 11, 2020 11:35
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.6% 2.9
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 75.5% 0.8
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch from 5b92e22 to 79734a2 Compare May 11, 2020 11:40
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.6% 2.9
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 72.0% 73.2% 1.2
pkg/reconciler/pipelinerun/resources/conditionresolution.go 81.7% 82.0% 0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 90.1% 90.5% 0.4
pkg/reconciler/pipelinerun/resources/pipelinespec.go 92.9% 100.0% 7.1
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 75.5% 0.8
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

@vdemeester vdemeester changed the title wip: Change the storage version to v1beta1 types Change the storage version to v1beta1 types May 11, 2020
@tekton-robot tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2020
@vdemeester
Copy link
Member Author

@sbwsg @bobcatfish we can provide a job to do the upgrade (as serving does here : https://github.com/knative/serving/blob/master/config/post-install/storage-version-migration.yaml). I think it should be only part of the documentation (not deployed as part of ko apply -f config/) and it would be more than nice to have it in the operator (cc @nikhil-thomas)

@ghost
Copy link

ghost commented May 11, 2020

Conflicting files

pkg/apis/pipeline/v1beta1/taskrun_defaults_test.go
pkg/apis/resource/v1alpha1/git/git_resource_test.go
pkg/reconciler/pipelinerun/pipelinerun_test.go
pkg/reconciler/pipelinerun/resources/apply_test.go
pkg/reconciler/pipelinerun/resources/conditionresolution_test.go
pkg/reconciler/pipelinerun/resources/input_output_steps_test.go
pkg/reconciler/pipelinerun/resources/pipelinerunresolution_test.go
pkg/reconciler/taskrun/resources/cloudevent/cloudevent_test.go
pkg/reconciler/taskrun/resources/image_exporter_test.go
pkg/reconciler/taskrun/taskrun_test.go 

D: my apologies, i guess these all stem from the cmp.Diff changeset.

@vdemeester
Copy link
Member Author

🙀

@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch from 79734a2 to ca8ff88 Compare May 11, 2020 13:38
@vdemeester
Copy link
Member Author

@sbwsg should be fixed 🎐

@ghost
Copy link

ghost commented May 11, 2020

@sbwsg @bobcatfish we can provide a job to do the upgrade (as serving does here : https://github.com/knative/serving/blob/master/config/post-install/storage-version-migration.yaml). I think it should be only part of the documentation (not deployed as part of ko apply -f config/)

I like the idea of documenting this in its first iteration. Would it become a part of any future release, do you think?

I guess the idea is to not break anyone who's happy with their alpha tasks and doesn't want to rock the boat with an upgrade. But are these folks even upgrading? And then how long should we be supporting them? That might be a bigger question than we can answer here though.

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.6% 2.9
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 72.0% 73.2% 1.2
pkg/reconciler/pipelinerun/resources/conditionresolution.go 81.7% 82.0% 0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 90.1% 90.5% 0.4
pkg/reconciler/pipelinerun/resources/pipelinespec.go 92.9% 100.0% 7.1
pkg/reconciler/pipelinerun/resources/resultrefresolution.go 87.5% 88.6% 1.1
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 75.5% 0.8
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch from ca8ff88 to d21e298 Compare May 11, 2020 13:48
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 85.7% 88.6% 2.9
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 72.0% 73.2% 1.2
pkg/reconciler/pipelinerun/resources/conditionresolution.go 81.7% 82.0% 0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 90.1% 90.5% 0.4
pkg/reconciler/pipelinerun/resources/pipelinespec.go 92.9% 100.0% 7.1
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 75.5% 0.8
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

@vdemeester
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

want *corev1.PodSpec
wantAnnotations map[string]string
}{{
desc: "simple",
ts: v1alpha1.TaskSpec{TaskSpec: v1beta1.TaskSpec{
Steps: []v1alpha1.Step{{Container: corev1.Container{
ts: v1beta1.TaskSpec{
Copy link

Choose a reason for hiding this comment

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

🎉

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I read through and I guess my only question is: are we at a point where chunks of the v1alpha1 packages can start to be deleted? Or is that still a ways off?

@tekton-robot
Copy link
Collaborator

[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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2020
@vdemeester
Copy link
Member Author

I read through and I guess my only question is: are we at a point where chunks of the v1alpha1 packages can start to be deleted? Or is that still a ways off?

We can start to think about it but… if it affects the API served, we need to make a deprecation notice on the CRDs not in v1alpha1 anymore (and I think we should take some time). But in a gist, yes, after this PR gets merged, we can start cleaning the v1alpha1 package with stuff that will never be executed (I need to check the order of things, but some validation and defaulting might be done after the conversion and thus not required anymore). Also we can clean the struct themselves if we feel like it (aka removing the embedded types, …) but this is gonna break the Go API so… we need to be careful.

@@ -21,7 +21,8 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
resourcev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/resource/v1alpha1"
Copy link
Member

Choose a reason for hiding this comment

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

much cleaner 👍

Type: "image",
Params: []v1alpha1.ResourceParam{{
Params: []v1beta1.ResourceParam{{
Copy link
Member

Choose a reason for hiding this comment

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

Should this be Params: []resourcev1alpha1.ResourceParam instead of Params: []v1beta1.ResourceParam? 🤔

@pritidesai
Copy link
Member

thanks @vdemeester
/lgtm but it has conflicts now 😢

@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch from d21e298 to 8d250e4 Compare May 12, 2020 08:52
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 88.1% 90.5% 2.4
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/pipelinerun/resources/conditionresolution.go 81.7% 82.0% 0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 90.1% 90.5% 0.4
pkg/reconciler/pipelinerun/resources/pipelinespec.go 92.9% 100.0% 7.1
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 75.5% 0.8
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

- switch storage to v1beta1 for CRDs (config/)
- use v1beta1 in controllers, …

Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
@vdemeester vdemeester force-pushed the 2410-v1beta1-storage branch from 8d250e4 to a01d514 Compare May 12, 2020 09:00
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
internal/builder/v1alpha1/task.go 68.4% 66.4% -2.0
pkg/apis/pipeline/v1beta1/condition_types.go Do not exist 0.0%
pkg/apis/pipeline/v1beta1/container_replacements.go Do not exist 100.0%
pkg/apis/pipeline/v1beta1/pipelinerun_types.go 88.1% 90.5% 2.4
pkg/apis/pipeline/v1beta1/resource_paths.go Do not exist 85.7%
pkg/apis/pipeline/v1beta1/step_replacements.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 72.0% 73.3% 1.2
pkg/reconciler/pipelinerun/resources/conditionresolution.go 81.7% 82.0% 0.3
pkg/reconciler/pipelinerun/resources/pipelinerunresolution.go 90.1% 90.5% 0.4
pkg/reconciler/pipelinerun/resources/pipelinespec.go 92.9% 100.0% 7.1
pkg/reconciler/taskrun/resources/taskspec.go 93.3% 100.0% 6.7
pkg/reconciler/taskrun/taskrun.go 74.7% 75.5% 0.8
pkg/reconciler/taskrun/validate_resources.go 91.8% 95.1% 3.3

@vdemeester
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Nice work, thank you so much!!!
/lgtm

@@ -0,0 +1,81 @@
/*
Copyright 2019 The Tekton Authors
Copy link
Member

Choose a reason for hiding this comment

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

Very NIT: should we update the year in the copyright statements of the new files in the beta folder?

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 12, 2020
@vdemeester
Copy link
Member Author

/test pull-tekton-pipeline-integration-tests

@tekton-robot tekton-robot merged commit 1fbac2a into tektoncd:master May 12, 2020
@vdemeester vdemeester deleted the 2410-v1beta1-storage branch May 12, 2020 13:37
@bobcatfish
Copy link
Collaborator

YAAAAASSS!!!! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change the storage version to v1beta1 types
5 participants