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

fall back to controller-runtime defaulting for the apiserver #436

Merged

Conversation

redradrat
Copy link
Contributor

What this PR does / why we need it:

For the mock controlplane, the defaulting was copy/pasted over from controller-runtime (I assume in some earlier state). As of https://github.com/kubernetes-sigs/controller-runtime/pull/1541/files, their defaulting got revamped, and thus we can fall back to their defaulting. Only for advanced users, we can allow to override this, but should overhaul this config in the future as well, as the property used to override is deprecated.

Fixes #

Signed-off-by: Ralph Kühnert <kuehnert.ralph@gmail.com>
@redradrat redradrat force-pushed the upstream-mock-controlplane-defaulting branch from e1e9540 to 66f2ecc Compare December 1, 2022 00:25
@kensipe
Copy link
Member

kensipe commented Dec 7, 2022

looks like a number of PRs coming working on the same area of code... I will review this today. thanks @redradrat !

@kensipe
Copy link
Member

kensipe commented Dec 7, 2022

I was just recently upgrading testenv libs and was debating on this... I was debating on if we should detect version and configure based on that providing tests for different versions of APIs... It seems reasonable to move on, establish ourselves on the latest and handle any issues if they come up... it is unlikely and the only reason I can think of to support older versions is for v1beta1 support of CRDs... but that was so long ago now. Thanks for this PR. I will merge this and adjust my current code accordingly. Agree with the the overhaul on config at some point. Thanks so much!

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the reason to not refactor the StartTestEnv function to remove the first argument... but this accomplishes the same affect. I will follow up after this merges. Thanks again!

@kensipe kensipe merged commit 4ad67ce into kudobuilder:main Dec 7, 2022
@redradrat
Copy link
Contributor Author

redradrat commented Dec 11, 2022

@kensipe tbh I don't know the reason either... I wrote this so brain-fried after long hours - I even forgot sending it at all 😅 sorry

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants