Better Tekton API package

[chuangw6]

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:

• Internal coupling issue: some _type.go files also define validation functions (pipeline_types.go example) 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? That means we might need to move some validation functions to _validation.go files and perhaps move some getters/setters/MarkStatus to somewhere else as well.

• External coupling issue: To segment pkg/apis to its own module, we need to make sure it doesn’t import other packages in Tekton Pipelines. But now it depends on a number of packages:

  • pkg/reconciler/testing: example
  • pkg/spire/config: example
  • pkg/substitution: example
  • pkg/resolution/common: example
  • pkg/reconciler/pipeline/dag: example

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

Proposal 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.

[chuangw6]

cc @wlynch @dibyom

[wlynch]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

/remove-lifecycle rotten