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

operator: cleanup CRD registration #23701

Merged

Conversation

mhofstetter
Copy link
Member

@mhofstetter mhofstetter commented Feb 13, 2023

Cleanup CRD registration at operator startup by removing boilerplate functions and CRD v1beta1 support.

Cleanup CRD registration at operator startup by removing boilerplate
functions.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter added the release-note/misc This PR makes changes that have no direct user impact. label Feb 13, 2023
@mhofstetter mhofstetter requested a review from a team as a code owner February 13, 2023 09:42
@mhofstetter
Copy link
Member Author

/test

crd *apiextensionsv1.CustomResourceDefinition,
poller poller,
) error {
scopedLog := log.WithField("name", crdName)
scopedLog := log.WithField("name", crd.Name)

if !k8sversion.Capabilities().APIExtensionsV1CRD {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure that this check can go away; can you confirm and, if reasonable, remove it too?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point.

should be possible to remove since the oldest k8s release supported by stable cilium is k8s 1.16 (https://docs.cilium.io/en/stable/concepts/kubernetes/compatibility/#kubernetes-compatibility) and since k8s 1.16 CRD v1 is supported (https://kubernetes.io/docs/reference/using-api/deprecation-guide/#customresourcedefinition-v122).

going to remove this check and its logic

Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

Looks good! If we can get rid of the v1beta1 CRD code, let's do that too.

Support for CRD v1beta1 has been removed, since this is no longer
necessary in all supported k8s versions.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
Copy link
Member Author

removed v1beta1 CRD support

@squeed
Copy link
Contributor

squeed commented Feb 13, 2023

lgtm!

@squeed squeed added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 13, 2023
@mhofstetter
Copy link
Member Author

/test

@nbusseneau
Copy link
Member

I see testing is not done yet, removing ready-to-merge for now, please add it back once CI has completed and has been reviewed :)

@nbusseneau nbusseneau removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 13, 2023
@mhofstetter
Copy link
Member Author

/ci-verifier

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 14, 2023
@nbusseneau nbusseneau merged commit a9cc526 into cilium:master Feb 14, 2023
@mhofstetter mhofstetter deleted the pr/mhofstetter/crd-registration-cleanup branch February 14, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants