[release-0.14] 🐛 inherited defaults not respected in cache BuilderWithOptions

[akalenyu]

When using BuilderWithOptions some of the inherited (manager/cluster) options are lost.
If they were provided, they would be stronger than the cache package defaults
(In my case, the default dynamic mapper is overridden with discovery).

From a quick look at 0.15/0.16 this is not an issue there.

[k8s-ci-robot]

Hi @akalenyu. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

[akalenyu]

/cc @joelanford

[sbueringer]

From a quick look at 0.15/0.16 this is not an issue there.

Is it a problem there or not? If it is we should fix it first on main

Nvm we got rid of this code

[sbueringer]

/ok-to-test

Fix seems okay to me. Not sure if we are still actively maintaining this branch though (no strong opinions from my side)

[sbueringer]

/lgtm

[sbueringer]

@alvaroaleman WDYT?

[sbueringer]

/lgtm cancel

Would be good to have some unit test coverage

[alvaroaleman]

I'd rather have ppl upgrade controller-runtime tbh

[akalenyu]

I'd rather have ppl upgrade controller-runtime tbh

So we have some release branches where we made the decision not to bump
back when the release note came out

A note from the maintainers

The following release is probably the largest in the history of the project. Controller Runtime is a foundational piece for almost all controllers and operators and we're aware that breaking changes are never an ask for our users, especially while running production services.

We take breaking changes very seriously and carefully reviewed each one of these changes to improve the codebase, user experience, and future maintainability of the project.

The v0.15 release is a stepping stone towards maturity.

As always, please reach out in Slack in #controller-runtime.

As far as I can tell based on https://github.com/kubernetes-sigs/controller-runtime/commits/release-0.14
There will be others in the same boat (notice at some point that they're using the wrong mapper)

Would be good to have some unit test coverage

Let me know if you would accept this fix, then I can work on some. Note, like you mention above,
this code is non existent anymore in later branches

[sbueringer]

As far as I can tell based on https://github.com/kubernetes-sigs/controller-runtime/commits/release-0.14
There will be others in the same boat (notice at some point that they're using the wrong mapper)

Can you elaborate, I don't understand what you mean. Are there blockers which prevent you from bumping to 0.16? If yes, I would prefer getting those resolved over maintaining 0.14 indefinitely

Edit: Ah wait, you already bumped on the main branch, but you have older branches with older CR versions that you have to maintain?

[akalenyu]

Edit: Ah wait, you already bumped on the main branch, but you have older branches with older CR versions that you have to maintain?

Yup

[sbueringer]

Alright, sounds fine from my side.

@alvaroaleman Fine for you as well?

[alvaroaleman]

yeah okay, fair