[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: camilamacedo86
Once this PR has been reviewed and has the lgtm label, please assign alvaroaleman for approval. For more information see the Code Review Process.

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

This seems like a breaking change to me.

Additional I think the CRDs we generate already behave this way as the apiserver recursively defaults top down

PR needs rebase.

Hi @sbueringer,

Sorry, I took a closer look at this, and you're absolutely right to push back.

I also read your comment on the issue, and I agree.

We could add tests to verify the current behavior, but I don't think this warrants keeping the issue open. I'll leave a comment on the issue and suggest we close it.

This doesn't really test our behaviour so much as it tests the API server behaviour, but I guess if that's beneficial to make sure that if the upstream behaviour changes, that we can handle it, then I'm ~ok with merging this

/lgtm

@sbueringer do you agree?

LGTM label has been added.

Hi @sbueringer,

Could you please help with this one?

After taking a deeper look and revisiting your comment, I completely agree with your feedback. We're only adding tests here now, as discussed: #622 (comment)

Thank you so much for your help!

This doesn't really test our behaviour so much as it tests the API server behaviour,

Agree. controller-gen just takes the default values from the markers and sets them in the corresponding fields in the CRD. I assume we already have sufficient test coverage for this. Accordingly this PR just adds regression test coverage for the defaulting code in the kube-apiserver. I would assume and hope that this test coverage already exists in the k/k repository and they don't intentionally make breaking changes to the v1 CRD API.

but I guess if that's beneficial to make sure that if the upstream behaviour changes, that we can handle it, then I'm ~ok with merging this
@sbueringer do you agree?

Independent of that, as Joel's okay with it and I don't have a strong objection against it, I"m fine with adding this test coverage.

But can we please change this test to use envtest so it tests actual kube-apiserver behavior? This would not be guaranteed if we directly call the same functions that the apiserver is currently calling internally. Additionally the maintenance effort for the test should be lower if we don't have to keep the test code in sync with the kube-apiserver code.

Please also change the PR title to refer to what we are testing vs. just pointing to an issue.

Hi @sbueringer

Thank you for looking this one.

But can we please change this test to use envtest so it tests actual kube-apiserver behavior? This would not be guaranteed if we directly call the same functions that the apiserver is currently calling internally. Additionally the maintenance effort for the test should be lower if we don't have to keep the test code in sync with the kube-apiserver code.

That was indeed a good idea and suggestion done !
Rebased with main as well.
So we are probably good to fly 🪰 here.

Thx

/lgtm

/assign @JoelSpeed
(in case you want to take another look)

LGTM label has been added.

@JoelSpeed could you give a help here for we get this one merged?

c/c @sbueringer

I think it's okay, let's merge

/lgtm
/approve

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here