🌱 Deprecate the scheme builder

[alvaroaleman]

Controller-Runtime should never be imported into api packages and the scheme builder is only useful in those, so deprecate it.

[alvaroaleman]

We could also word this stronger and mention that api packages should not import anything other than other api packages and apimachinery

[sbueringer]

Fine for me. Or we phrase it more generically and write that folks should be extremely careful with any dependency they add to their API (packages).

E.g. having dependencies on corev1.ObjectReference is an extremely bad idea. Similarly embeddeding any external API types into an API needs careful consideration as it couples APIs (somewhat fine with k/k v1 APIs, but even then there are non-trivial trade-offs in my opinion like inheriting all changes that are made to these APIs elsewhere)

@JoelSpeed Do you have some input for how we should phrase this?
I think this is deprecation warning is a good place to share some guidance. I wouldn't go into too much detail but some high-level points could be good here

[JoelSpeed]

Or we phrase it more generically and write that folks should be extremely careful with any dependency they add to their API (packages).

This sounds like a good way to frame it

I would target this message to APIs should import minimal dependencies. Maybe something along the lines of:

// API Packages should import minimal dependencies, typically including only k8s.io/apimachinery sub-packages and standard library packages.
// Importing additional packages affects consumers of your API and adds complexity to their dependency management.

[alvaroaleman]

I've reworded it mostly based on Joels suggestion with the addition that importing other API packages is also fine, let me know what you think

[sbueringer]

The way you phrased it sounds good to me.

I had my share of problems with importing API packages in API packages so now I'm extremely careful with those.

Problems there can be:

• Importing types that don't actually fit the use case, example metav1.ObjectReference (that one even has a godoc comment that it should not be used)
• Inheriting changes of the imported types including schema changes
• Making it necessary to cut a new apiVersion because the referenced apiVersion was bumped to avoid breaking changes (e.g. if a v1beta1 type is embedded into another API and then that type is bumped to v1beta2)

So at that point I only consider it safe to embed external API types if:

• the lifecycles align (e.g. some k/k v1 type or something that is bumped in lock step with the API the type is embedded in)
• the use case perfectly fits (that means the fields, godoc and schema validation perfectly fits any one of those might diverge already or might diverge over time)

All that being said, I'm fine with the current wording. We don't have to be too detailed here :) (just sharing some context)

[alvaroaleman]

I think that comment really just tries to explain what is okay to have as a dependency. If it actually makes sense to depend on other apis is yet another question that IMHO we shouldn't try to answer here.

[JoelSpeed]

From an API review perspective, we only generally allow importing other API packages if you are "passing through" the data. E.g. you have a type with an embedded pod spec and you then just copy that directly onto another type

Stefan's write up is detailed enough that I don't need to add anymore, but I am fine with the current wording

[troy0820]

/lgtm

I know this isn't the removal yet, but we may want to allow @camilamacedo86 to be aware of the changes if they wish to get in front of this with kubebuilder.

/hold

[k8s-ci-robot]

LGTM label has been added.

Details

Git tree hash: e675826794effe86010581339f6382e8bcf9a074

[sbueringer]

Fine for me. Or we phrase it more generically and write that folks should be extremely careful with any dependency they add to their API (packages).

E.g. having dependencies on corev1.ObjectReference is an extremely bad idea. Similarly embeddeding any external API types into an API needs careful consideration as it couples APIs (somewhat fine with k/k v1 APIs, but even then there are non-trivial trade-offs in my opinion like inheriting all changes that are made to these APIs elsewhere)

@JoelSpeed Do you have some input for how we should phrase this?
I think this is deprecation warning is a good place to share some guidance. I wouldn't go into too much detail but some high-level points could be good here

[sbueringer]

Last nit

[k8s-ci-robot]

@alvaroaleman: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff	5257194	link	false	/test pull-controller-runtime-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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-sigs/prow repository. I understand the commands that are listed here.

[sbueringer]

Thx!

/lgtm
/approve

Keeping the hold so @JoelSpeed can take another look

[k8s-ci-robot]

LGTM label has been added.

Details

Git tree hash: 3ebffa234fe07f7a11cfd93cdabcff21067c5ee7

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman,sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment