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

TEP-0060 Resolver to Stable #6579

Open
JeromeJu opened this issue Apr 25, 2023 · 8 comments
Open

TEP-0060 Resolver to Stable #6579

JeromeJu opened this issue Apr 25, 2023 · 8 comments
Labels
area/resolution Issues related to remote resolution kind/feature Categorizes issue or PR as related to a new feature.

Comments

@JeromeJu
Copy link
Member

Feature looking for Promotion

Resolver is in 'beta' right now

Thanks to the inputs @tektoncd/core-collaborators during the WG, feedbacks are required to be collected for promoting
Resolver to stable in v1 so as to avoid the loss of 'default' setup for this feature.

cc @tektoncd/core-maintainers

Current Feature Version

Beta feature

Raise Feature Version

Stable/ GA

Pipeline Release

TBD

Issues Reported by the Users

Usage in Dogfooding Cluster

Examples/Tests

@JeromeJu JeromeJu added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 25, 2023
@lbernick lbernick changed the title Resolver to Stable TEP-0060 Resolver to Stable Jun 1, 2023
@lbernick lbernick added the area/resolution Issues related to remote resolution label Jun 1, 2023
@lbernick
Copy link
Member

lbernick commented Jun 1, 2023

One thing to think about as we're moving resolution to stable is to find a versioning strategy + compatibility policy for the resolution API. For example, it would be nice to use a v1.RefSource instead of a v1beta1.RefSource in ResolvedResource, but making this change would force resolvers to update (although the two structs are identical).

Related, some relevant guidance from the Google AIPs: AIP-213 states that "The public representation of APIs should be self-contained, meaning that all protos used by the API originate in the same proto package, except for common protos, which may be used freely in any API... APIs should not directly depend on protos defined in other APIs. Instead, they should copy and paste the applicable messages into their own API." In this context, I don't think it was a good idea to have v1beta1 structs like Params and RefSource in the resolver API, since it ties the resolver API to the Pipelines v1beta1 API.

@vdemeester
Copy link
Member

One thing to think about as we're moving resolution to stable is to find a versioning strategy + compatibility policy for the resolution API. For example, it would be nice to use a v1.RefSource instead of a v1beta1.RefSource in ResolvedResource, but making this change would force resolvers to update (although the two structs are identical).

If the types are identical, from go persective, using type alias would "just" work, wouldn't it ?

But also, I don't think any of this is related to the API itself (aka the Kubernetes CRD API), only the go library API we define and use (and that other can use).

And according to this :

Tekton Pipelines may introduce breaking changes to its Go client libraries, as long as these changes do not impact users' yaml/json CRD definitions

We are in that situation, as long as it doesn't break yaml/json CRD definitions (which I don't think it's the case), we can make a "breaking" change on the resolvers on using v1 types instead of v1beta1.

@lbernick
Copy link
Member

lbernick commented Jun 1, 2023

One thing to think about as we're moving resolution to stable is to find a versioning strategy + compatibility policy for the resolution API. For example, it would be nice to use a v1.RefSource instead of a v1beta1.RefSource in ResolvedResource, but making this change would force resolvers to update (although the two structs are identical).

If the types are identical, from go persective, using type alias would "just" work, wouldn't it ?

I'm not sure how this would work unless we aliased v1beta1.RefSource to v1.RefSource. I think that's how we did things for v1alpha1 but this time around we decided to create copies. Unless I'm misunderstanding your suggestion?

But also, I don't think any of this is related to the API itself (aka the Kubernetes CRD API), only the go library API we define and use (and that other can use).

I thought about it a bit more and I realized you're right that existing resolvers wouldn't have to update as a result of this change, since the ResolutionRequest CRD hasn't changed. They'd only need to update if they need to pull in a new version of Pipelines for some reason.

However, I wonder if we should change our compatibility policy to be a bit stricter for the resolvers API. I don't think we'd want to make a lot of breaking changes to it even if ResolutionRequest stays the same.

@vdemeester
Copy link
Member

I'm not sure how this would work unless we aliased v1beta1.RefSource to v1.RefSource. I think that's how we did things for v1alpha1 but this time around we decided to create copies. Unless I'm misunderstanding your suggestion?

Yeah, that's the thought (on aliases).

I thought about it a bit more and I realized you're right that existing resolvers wouldn't have to update as a result of this change, since the ResolutionRequest CRD hasn't changed. They'd only need to update if they need to pull in a new version of Pipelines for some reason.

Yes exactly 👼🏼

However, I wonder if we should change our compatibility policy to be a bit stricter for the resolvers API. I don't think we'd want to make a lot of breaking changes to it even if ResolutionRequest stays the same.

In that particular case, even if we don't alias, this is hardly a breaking change 🙃. A resolver's authors just have to change the import path for those types. This is something they are probably used to for various reasons 👼🏼

@JeromeJu
Copy link
Member Author

JeromeJu commented Aug 8, 2023

The RefSource has been updated to v1 in the v1 storage swap, which might make us one step closer to promote this to stable 🙏

Would like to learn if there are any other concerns promoting this feature?
cc @tektoncd/core-maintainers

@lbernick
Copy link
Member

I'm still wondering if we should use non-versioned apis for the resolver interface. I updated our compatibility policy to state that after promoting resolvers to stable, we won't change the interface, so I think we should figure that out before stabilizing this feature.

I think we should also address #5931 before promotion.

@tekton-robot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@jerop
Copy link
Member

jerop commented Nov 13, 2023

/lifecycle frozen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/resolution Issues related to remote resolution kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

5 participants