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.

Sign up for GitHub

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

Jump to bottom

Change the storage version to v1beta1 types #2410

Closed
vdemeester opened this issue Apr 16, 2020 · 0 comments · Fixed by #2577
Closed

Change the storage version to v1beta1 types #2410

vdemeester opened this issue Apr 16, 2020 · 0 comments · Fixed by #2577
Assignees
@vdemeester
Labels
area/roadmap Issues that are part of the project (or organization) roadmap (usually an epic) kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Milestone
Pipelines 0.13 🐱

Comments

@vdemeester
Copy link
Member

Once conversion webhook are in place (see #2047), we should be able to switch to v1beta1 to storage types (and when we go to v1 we will be able to switch to v1 as storage right away).

This should help cleaning up the code (both in pkg/apis and pkg/reconcilier) and will make the API easier to reason about (aka v1alpha1 won't need to have v1beta1 fields anymore).

The target for this should be one release after #2047 (so tentatively adding it to a 0.13 milestone)

/kind cleanup
/assign
@tekton-robot tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Apr 16, 2020
@vdemeester vdemeester added the area/roadmap Issues that are part of the project (or organization) roadmap (usually an epic) label Apr 16, 2020
@vdemeester vdemeester added this to the Pipelines 0.13 🐱 milestone Apr 16, 2020
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 16, 2020
@bobcatfish
Try to make path usage less confusing 🛣️
d1ba2f1 
In tektoncd#2408 it was confusing how the the path to the resource input worked
in the tutorial. I think this can be slightly improved by using variable
interpolation to get the path so the author of the Tasks etc. doesn't
have to worry as much about exactly where on disk it is.

I thought about updating the v1alpha1 example as well but I'm assuming
we'll be getting rid of those once we merge tektoncd#2410 so it didn't seem
worth it. Plus there are other examples that probably could be updated
but I feel like a small improvement is better than none at all :D
@bobcatfish bobcatfish mentioned this issue Apr 16, 2020
Merged
1 task
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 16, 2020
@bobcatfish
Try to make path usage less confusing 🛣️
ac72720 
In tektoncd#2408 it was confusing how the the path to the resource input worked
in the tutorial. I think this can be slightly improved by using variable
interpolation to get the path so the author of the Tasks etc. doesn't
have to worry as much about exactly where on disk it is.

I thought about updating the v1alpha1 example as well but I'm assuming
we'll be getting rid of those once we merge tektoncd#2410 so it didn't seem
worth it. Plus there are other examples that probably could be updated
but I feel like a small improvement is better than none at all :D
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 16, 2020
@bobcatfish
Try to make path usage less confusing 🛣️
6612c6f 
In tektoncd#2408 it was confusing how the the path to the resource input worked
in the tutorial. I think this can be slightly improved by using variable
interpolation to get the path so the author of the Tasks etc. doesn't
have to worry as much about exactly where on disk it is.

I thought about updating the v1alpha1 example as well but I'm assuming
we'll be getting rid of those once we merge tektoncd#2410 so it didn't seem
worth it. Plus there are other examples that probably could be updated
but I feel like a small improvement is better than none at all :D
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 16, 2020
@bobcatfish
Try to make path usage less confusing 🛣️
aa6545c 
In tektoncd#2408 it was confusing how the the path to the resource input worked
in the tutorial. I think this can be slightly improved by using variable
interpolation to get the path so the author of the Tasks etc. doesn't
have to worry as much about exactly where on disk it is.

I thought about updating the v1alpha1 example as well but I'm assuming
we'll be getting rid of those once we merge tektoncd#2410 so it didn't seem
worth it. Plus there are other examples that probably could be updated
but I feel like a small improvement is better than none at all :D
tekton-robot pushed a commit that referenced this issue Apr 17, 2020
@bobcatfish @tekton-robot
Try to make path usage less confusing 🛣️
62737cf 
In #2408 it was confusing how the the path to the resource input worked
in the tutorial. I think this can be slightly improved by using variable
interpolation to get the path so the author of the Tasks etc. doesn't
have to worry as much about exactly where on disk it is.

I thought about updating the v1alpha1 example as well but I'm assuming
we'll be getting rid of those once we merge #2410 so it didn't seem
worth it. Plus there are other examples that probably could be updated
but I feel like a small improvement is better than none at all :D
This was referenced Apr 17, 2020
Merged
Merged
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 22, 2020
@bobcatfish
Use the same Deps logic in v1alpha1 and v1beta1
45ef5d7 
Sometimes the PipelineTask that Deps is being executed on is actually
v1beta1 instead of v1alpha1 and the old Deps function, which doesn't
account for Results, is being called.

This PR duplicates the logics so the Deps function is the same.
After tektoncd#2410 we'll be able to assume we're always using the v1beta1 types
and will not need the logic in both places and can avoid bugs like this.

It also duplicates the DAG test logic which is the closest thing Deps()
currently has to a set of unit tests.

Part of tektoncd#2463
@bobcatfish bobcatfish mentioned this issue Apr 22, 2020
Merged
2 tasks
tekton-robot pushed a commit that referenced this issue Apr 23, 2020
@bobcatfish @tekton-robot
Use the same Deps logic in v1alpha1 and v1beta1
893dde2 
Sometimes the PipelineTask that Deps is being executed on is actually
v1beta1 instead of v1alpha1 and the old Deps function, which doesn't
account for Results, is being called.

This PR duplicates the logics so the Deps function is the same.
After #2410 we'll be able to assume we're always using the v1beta1 types
and will not need the logic in both places and can avoid bugs like this.

It also duplicates the DAG test logic which is the closest thing Deps()
currently has to a set of unit tests.

Part of #2463
@vdemeester vdemeester mentioned this issue Apr 23, 2020
Merged
2 tasks
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 23, 2020
@bobcatfish
Use the same Deps logic in v1alpha1 and v1beta1
1bc5c50 
Sometimes the PipelineTask that Deps is being executed on is actually
v1beta1 instead of v1alpha1 and the old Deps function, which doesn't
account for Results, is being called.

This PR duplicates the logics so the Deps function is the same.
After tektoncd#2410 we'll be able to assume we're always using the v1beta1 types
and will not need the logic in both places and can avoid bugs like this.

It also duplicates the DAG test logic which is the closest thing Deps()
currently has to a set of unit tests.

Part of tektoncd#2463

(cherry picked from commit 893dde2)
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 23, 2020
@bobcatfish
Add DAG tests for v1beta1 as well as v1alpha1 📈
8286c91 
The fix for tektoncd#2463 made sure that we use the same Deps logic for both
v1alpha1 and v1beta1 types. This DAG test is the closest thing that Deps
has to unit test.

I meant to include this test in
tektoncd@45ef5d7
but forgot to add it!!

This test would be only temporary since after tektoncd#2410 we won't need to
duplicated any of this logic, but it seems useful to have in the
meantime. (Or until we add unit tests for Deps!!)
@bobcatfish bobcatfish mentioned this issue Apr 23, 2020
Merged
2 tasks
tekton-robot pushed a commit that referenced this issue Apr 23, 2020
@bobcatfish @tekton-robot
Add DAG tests for v1beta1 as well as v1alpha1 📈
cc0d54f 
The fix for #2463 made sure that we use the same Deps logic for both
v1alpha1 and v1beta1 types. This DAG test is the closest thing that Deps
has to unit test.

I meant to include this test in
45ef5d7
but forgot to add it!!

This test would be only temporary since after #2410 we won't need to
duplicated any of this logic, but it seems useful to have in the
meantime. (Or until we add unit tests for Deps!!)
tekton-robot pushed a commit that referenced this issue Apr 23, 2020
@bobcatfish @tekton-robot
Use the same Deps logic in v1alpha1 and v1beta1
93fb97c 
Sometimes the PipelineTask that Deps is being executed on is actually
v1beta1 instead of v1alpha1 and the old Deps function, which doesn't
account for Results, is being called.

This PR duplicates the logics so the Deps function is the same.
After #2410 we'll be able to assume we're always using the v1beta1 types
and will not need the logic in both places and can avoid bugs like this.

It also duplicates the DAG test logic which is the closest thing Deps()
currently has to a set of unit tests.

Part of #2463

(cherry picked from commit 893dde2)
This was referenced May 4, 2020
Merged
Merged
Merged
Merged
@pritidesai pritidesai mentioned this issue Nov 20, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vdemeester vdemeester

Labels
area/roadmap Issues that are part of the project (or organization) roadmap (usually an epic) kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Milestone
Pipelines 0.13 🐱
Development

Successfully merging a pull request may close this issue.

Change the storage version to v1beta1 types vdemeester/tektoncd-pipeline
2 participants
@vdemeester @tekton-robot