search

tektoncd / pipeline

A cloud-native Pipeline resource.
https://tekton.dev
Apache License 2.0
8.45k stars 1.77k forks source link

Better Tekton API package #6483

Open chuangw6 opened 1 year ago

chuangw6 commented 1 year ago

I wanted to create this ticket to kick off the discussion around how we can make Tekton API (pkg/apis package) easier to use. Currently, due to the heavy coupling issues in Tekton API, the vendors building on the top of Tekton Pipelines have to import some unnecessary dependencies on the rest of tree of Tekton Pipelines, such as Triggers which only needs to import Tekton API, and Tekton Chains, Results and CLI who need both API and Client. Therefore, segmenting Tekton API pkg/apis and client pkg/client into their own modules might help those vendors reduce the number of dependencies and make it easier to use for all future vendors who only need API and/or client.

The current coupling issues in pkg/apis package:

Since Client only depends on API package in Tekton Pipelines, if we clean up the APIs, client should be slimmer too.

Proposal Steps:

chuangw6 commented 1 year ago

cc @wlynch @dibyom

wlynch commented 1 year ago

I'm generally in favor of this! (though be mindful of breaking changes)

Internal coupling issue: some _type.go files also define validation functions which call other validation code defined in _validation.go files. Should we add a new design principle that _types.go files should only contain struct definitions, not any other function definitions?

These are defined because of the apis.Validatable interface which knative uses to configure the admission webhooks along with Defaultable. These likely need to stay in place attached to the types. There may be some opportunities to move behavior out of the admission webhooks into the reconciler, but we'd have to look at those on a case-by-case basis.

chuangw6 commented 1 year ago

These are defined because of the apis.Validatable interface which knative uses to configure the admission webhooks along with Defaultable. These likely need to stay in place attached to the types. There may be some opportunities to move behavior out of the admission webhooks into the reconciler, but we'd have to look at those on a case-by-case basis.

Thanks for the inputs! Moving them out of api package would be perfect, but I think we should at least move validation out of _types.go file. For example, _validation.go might be a good place for it.

chuangw6 commented 1 year ago

Also, I am wondering if mirroring api and client into their own repos will help us in the long term esp as tekton grows.

Kubernetes has the staging folder in the home repo where all the development happens and periodically syncs individual packages into own repos.

wlynch commented 1 year ago

but I think we should at least move validation out of _types.go file. For example, _validation.go might be a good place for it.

I think that's what we generally aim for (though I wouldn't be surprised if we're not strictly following this everywhere). This should all be the same package though, so it shouldn't make a functional difference which file it lives in.

Also, I am wondering if mirroring api and client into their own repos will help us in the long term esp as tekton grows.

Kubernetes has the staging folder in the home repo where all the development happens and periodically syncs individual packages into own repos.

My preference would be to use Workspaces with a separate module in the same repo to avoid having to maintain extra automation. I can't think of any benefits for separate repos that a separate module wouldn't also give us.

dibyom commented 1 year ago

I think moving api and client to its own module makes sense. It would be tough to move the validation/defaulting/conversion functions since the knative/pkg conventions depend on our types having those functions. Refactoring the package to not depend on other packages in tekton sounds like a good idea though.

I'm not sure about a separate repo, not sure how much benefit that will bring over the cost of maintaining another repo.

chuangw6 commented 1 year ago

I think that's what we generally aim for (though I wouldn't be surprised if we're not strictly following this everywhere). This should all be the same package though, so it shouldn't make a functional difference which file it lives in.

I agree that files doesn't make a functional differences given that they are in the same package anyways, but I think clearly separating _type.go and _validation.go files will help the vendors who is able to cherrypick which files to import from tekton pipelines. For instance, when they only need _types.go in their third_party, this will help avoid importing unnecessary dependencies.

My preference would be to use Workspaces with a separate module in the same repo to avoid having to maintain extra automation.

👍

wlynch commented 1 year ago

separating _type.go and _validation.go files will help the vendors who is able to cherrypick which files to import from tekton pipelines.

I would not rely on file names as a stable reference. You're welcome to selectively parse/transform files in compliance with the LICENSE, but I doubt we will guarantee compatibility. Package boundaries are probably safest.

chuangw6 commented 1 year ago

It would be tough to move the validation/defaulting/conversion functions since the knative/pkg conventions depend on our types having those functions.

Thanks for the input. @dibyom!

I would not rely on file names as a stable reference. Package boundaries are probably safest.

That's a good point.

I will think about this a bit more.

From today (2023-04-03) API WG discussion, we got community agreement, and we will start from step 1 and see how it goes.

bendory commented 1 year ago

I think moving api and client to its own module makes sense. It would be tough to move the validation/defaulting/conversion functions since the knative/pkg conventions depend on our types having those functions. Refactoring the package to not depend on other packages in tekton sounds like a good idea though.

I'm not sure about a separate repo, not sure how much benefit that will bring over the cost of maintaining another repo.

Suggestion: let's complete the currently-defined steps

Step 1: move all the validation funcs from _type.go files to _validation.go files Step 2 (not too sure): move getter/setter/markstatus funcs from _types.go files to somewhere else?? Step 3: makes sure pkg/apis package doesn’t depend on other packages in Tekton Pipelines Step 4: segment api and client into their own go modules.

and then proceed to evaluate the trade-offs in moving all the API definitions into a separate repo.

On my mind: providers, vendors, and users who have a dependency on Tekton really should have no requirement to clone the entire pipelines repo as part of their third-party vendoring process. Perhaps Go modules are the right approach, or perhaps a more portable approach is to move all our public APIs that users code against into Protocol Buffer definitions in a stand-alone repo -- this enables API dependencies with no code / implementation dependencies whatsoever?

Having tossed that idea into this issue, I don't think it's worth debating this until we have a stand-alone Go module, IMO that's a clear first step toward this path anyway, and it may be all that is needed today.

tekton-robot commented 1 year ago

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.

tekton-robot commented 1 year ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten with a justification. Rotten issues close after an additional 30d of inactivity. 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 rotten

Send feedback to tektoncd/plumbing.

HumairAK commented 1 year ago

/remove-lifecycle rotten