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

Add support for tektoncd/pipeline v1beta1 api version #26818

Closed

Conversation

qaifshaikh
Copy link

This is a continuation of #26535

We've already added the support for tektoncd/pipeline v0.36.0 in #26762

The next phase is to add support for v1beta1 API versions

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 14, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @qaifshaikh. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/prow Issues or PRs related to prow labels Jul 14, 2022
@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Jul 14, 2022
@qaifshaikh
Copy link
Author

It seems tektoncd/pipeline deprecated the support for conditions in v1beta1.

But recently they completely removed it before releasing support for v1
Here's the issue that discusses it: tektoncd/pipeline#3377
And this is the commit that removes it: tektoncd/pipeline@9d60d0a

I wonder if it's worth resolving the references for the conditions in v1beta1 support for prow? Or does it make sense to just get rid of it overall to prepare for v1 support later?

cc @eddycharly @olemarkus for input 🙏🏻

@matthyx
Copy link
Contributor

matthyx commented Jul 15, 2022

/ok-to-test
(I would personally remove it completely and prepare the jump to v1)

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 15, 2022
@olemarkus
Copy link
Member

I'm also for completely removing conditions and upgrade pipeline to 0.37+.

@eddycharly
Copy link
Member

I'm also for completely removing conditions and upgrade pipeline to 0.37+.

+1

@k8s-ci-robot k8s-ci-robot added the area/prow/deck Issues or PRs related to prow's deck component label Jul 19, 2022
@k8s-ci-robot k8s-ci-robot added area/prow/hook Issues or PRs related to prow's hook component area/prow/tide Issues or PRs related to prow's tide component labels Jul 19, 2022
@qaifshaikh qaifshaikh marked this pull request as ready for review July 19, 2022 23:45
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 19, 2022
@matthyx
Copy link
Contributor

matthyx commented Jul 22, 2022

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from matthyx July 22, 2022 08:42
@alvaroaleman
Copy link
Member

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 2, 2022
@chaodaiG
Copy link
Contributor

chaodaiG commented Aug 2, 2022

AFAICT I have never seen anyone using this

@eddycharly
Copy link
Member

@alvaroaleman we used it in my previous job but I'm pretty sure they switched to something else now.

@qaifshaikh
Copy link
Author

@alvaroaleman
v1 was released a few weeks ago and was not completely stable and tekton added 2 patches in the following week for bug fixes.
I'm planning on doing a couple more PRs to upgrade tekton to v0.38.0 and then add support for v1 as well as it gets more stable

@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 2, 2022
@qaifshaikh
Copy link
Author

@alvaroaleman i've removed the irrelevant changes. Are we good to merge now?

@alvaroaleman
Copy link
Member

Well, I am still not convinced that it is a great idea to do a breaking change now to upgrade to a beta version just to do a new breaking change in a couple of weeks to upgrade to v1.

@qaifshaikh
Copy link
Author

I'll do the v1 upgrade PR as well then!

@qaifshaikh
Copy link
Author

qaifshaikh commented Aug 3, 2022

@alvaroaleman based on what I can see here, they haven't added the v1 support for Pipelines yet. It's only for a few resources like Tasks, Workspaces and Results.

They're still using v1beta1 API versions for Pipeline, ClusterTasks, TaskRun and PipelineRun as per the release announcement

Does it make sense to switch to v1 while still keeping all the v1beta1 dependencies for some of the main packages?

@chaodaiG
Copy link
Contributor

chaodaiG commented Aug 3, 2022

I'm on the same page as @alvaroaleman that if v1 is becoming the recommended version real soon, we probably should wait a bit to avoid confusion to potential users of this feature.

seems that @imjasonh is still active in the Tekton space, a quick question: do we know the timeline when Tekton v1 become the recommended version?

@eddycharly
Copy link
Member

Again, how about supporting multiple versions ?
Or do you prefer supporting only one ?

@qaifshaikh
Copy link
Author

@eddycharly I'm not sure how we'd be supporting multiple versions though 🤔 Any suggestions?

Tekton removed most of the support for the v1alpha1 API versions in the recent releases

@eddycharly
Copy link
Member

eddycharly commented Aug 3, 2022

Of course, if tekton removes v1alpha1 we will have to remove support when upgrading.
We could:

  • Support alpha1 and beta1 side by side (using the latest tekton version that supports both)
  • Deprecate use of alpha1
  • Upgrade tekton and remove alpha1 support
  • Add support for v1

Does this make sense ?

@eddycharly
Copy link
Member

eddycharly commented Aug 3, 2022

@alvaroaleman are there any benefits from generating the tekton client rather than using https://github.com/tektoncd/pipeline/tree/main/pkg/client ?

Something like this #27018

@qaifshaikh
Copy link
Author

I think that makes sense if we have a lot of users leveraging the v1alpha1 support for the pipelines on prow.
But I think there aren't many users for it right now since v1beta1 became the standard years ago and we hadn't upgraded the pipelines since v0.14.3.

Adding the v1beta1 support might attract a lot of new users for sure, since it adds capabilities like ephemeral volumes, additional triggers and support for Tasks and Steps to parallelize jobs on more levels.

So picking up from your suggestion, if we upgrade tekton pipelines to v0.38.2 (which is the latest version right now) in prow right now:

  1. We deprecate and remove v1alpha1 immediately since it has already been removed from tekton!
  2. The only remaining component in pipelinev1alpha1 is run which has been replaced by runv1alpha1
  3. Next, we add full support for pipelinev1beta1 which my PR does.
  4. And last we are prepared for pipelinev1 as tekton moves forward to make it the standard

I'm not sure if we can come up with any smoother transition than this, at this point in time.

@eddycharly
Copy link
Member

I agree there are probably very little users, still we should give them a chance to upgrade.

@vanwho
Copy link

vanwho commented Aug 3, 2022

Perhaps if folks haven't upgraded their key pipeline orchestrator in over 2 years, it would be safe to assume they won't upgrade to the absolute latest prow version in the immediate, and likely whenever they do, will be upgrading their stack and testing. Removing support for something that is removed from the 'upstream' integration seems quite reasonable. Everyone has control over their own upgrade path and versions, we probably shouldn't restrict advancement as if things aren't versioned.

@eddycharly
Copy link
Member

Chances are that it will cause issues with existing prow jobs too. If the job contains a v1alpha1 spec, we probably won't be able to load it back to a v1beta1 spec.

@eddycharly
Copy link
Member

Structure wise, it could look something like this #27034

@qaifshaikh
Copy link
Author

qaifshaikh commented Aug 4, 2022

I see what you mean 🤔
So, we don't upgrade tekton pipelines anymore for now? Because if we do, v1alpha1 is removed and prowjobs will start failing for users leveraging v1alpha1.

That would also mean we have to wait for v1 and I'm okay with that since it might be a while for it to become the standard.
What do you think @alvaroaleman ?

@eddycharly
Copy link
Member

eddycharly commented Aug 4, 2022

When we decide to drop v1alpha1 support, we can probably remove the field (or use something like RawExtension), but not changing its type.

@qaifshaikh
Copy link
Author

Sounds good to me.
Do you want me to do a new PR for the multi support? or do we go with your PR?

@eddycharly
Copy link
Member

eddycharly commented Aug 4, 2022

I have one PR to add beta1 client.

And I have another PR that introduces new fields to support multiple versions.

Now I wonder if we need one controller per version or one controller for all versions.

@qaifshaikh
Copy link
Author

IMO controller per version would be easier to deprecate. But only if its not too much work to support both.

@eddycharly
Copy link
Member

Not sure, but maybe we can use v1beta1 and use conversion, I'll try that.

@qaifshaikh
Copy link
Author

Close for #27032 and #27034

@qaifshaikh qaifshaikh closed this Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/config Issues or PRs related to code in /config area/prow/bump Updates to the k8s prow cluster area/prow/deck Issues or PRs related to prow's deck component area/prow/hook Issues or PRs related to prow's hook component area/prow/tide Issues or PRs related to prow's tide component area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants