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

🌱 Remove reliance on controller-runtime scheme builder #9045

Merged

Conversation

JoelSpeed
Copy link
Contributor

What this PR does / why we need it:

To work towards #9011, we need to remove the reliance on controller-runtime from the API packages.
This updates the scheme building methods in the core v1beta1 API group to rely only on API machinery types and not controller runtime types.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 21, 2023
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 21, 2023
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

Thanks!

It seems innocuous to me to change the type of the SchemeBuilder variable - which is what the apidiff is failing on right now. Does the new SchemeBuilder have to be exported at all?

@JoelSpeed
Copy link
Contributor Author

I think you're right, i don't think it does need to be exported. Currently we are using it in the test builders so that we can access the AddToScheme function, but that can be accessed directly since we alias it at the package level

@JoelSpeed
Copy link
Contributor Author

@killianmuldoon I made the scheme builder private, I think this is good to go if you're happy with the API diff

@chrischdi
Copy link
Member

Nice improvement 👍

Should we track doing the same for the other API's we define (maybe separate PR then / let's have an issue for that)?

@chrischdi
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: cf355907c165e22695fddef078d900485e255f17

@JoelSpeed
Copy link
Contributor Author

Should we track doing the same for the other API's we define (maybe separate PR then / let's have an issue for that)?

Yes, what do you think is the best way to track this? I can either update the linked issue for this to create a checklist for APIs to move across, or I can duplicate the issue for each group. I lean towards the first so that we have a single top level tracker but I'm easy if other prefer the alternative approach

Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

Changes LGTM

Could we document in the book for providers that this is how we expect schemes to be built from API packages?

@chrischdi
Copy link
Member

Should we track doing the same for the other API's we define (maybe separate PR then / let's have an issue for that)?

Yes, what do you think is the best way to track this? I can either update the linked issue for this to create a checklist for APIs to move across, or I can duplicate the issue for each group. I lean towards the first so that we have a single top level tracker but I'm easy if other prefer the alternative approach

Both is totally fine I think. I'm tend towards updating the linked issue with a checklist 👍 .

@killianmuldoon
Copy link
Contributor

/test

@k8s-ci-robot
Copy link
Contributor

@killianmuldoon: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

  • /test pull-cluster-api-build-main
  • /test pull-cluster-api-e2e-main
  • /test pull-cluster-api-test-main
  • /test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-apidiff-main
  • /test pull-cluster-api-e2e-full-dualstack-and-ipv6-main
  • /test pull-cluster-api-e2e-full-main
  • /test pull-cluster-api-e2e-informing-main
  • /test pull-cluster-api-e2e-mink8s-main
  • /test pull-cluster-api-e2e-scale-main-experimental
  • /test pull-cluster-api-e2e-workload-upgrade-1-27-latest-main
  • /test pull-cluster-api-test-mink8s-main

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-apidiff-main
  • pull-cluster-api-build-main
  • pull-cluster-api-e2e-informing-main
  • pull-cluster-api-e2e-main
  • pull-cluster-api-test-main
  • pull-cluster-api-verify-main

In response to this:

/test

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.

@killianmuldoon
Copy link
Contributor

/test pull-cluster-api-e2e-full-main

/hold
For CI results - just as a precaution.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2023
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2023
)

func addKnownTypes(scheme *runtime.Scheme) error {
Copy link
Member

@sbueringer sbueringer Aug 3, 2023

Choose a reason for hiding this comment

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

Q: Isn't this all the same as just:

func AddToScheme(s *runtime.Scheme) {
	s.AddKnownTypes(GroupVersion, objectTypes...)
	metav1.AddToGroupVersion(s, GroupVersion)
}

Would it be simpler to just define the func?

(seems a lot easier to understand to me, but maybe I'm missing something)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pattern seems to be common if you look at say core/v1 or any other API package, so I was just following the usual pattern.

In those other APIs the schemebuilder is exported though, perhaps we should be leaving ours exported for consistency?

Copy link
Member

@sbueringer sbueringer Aug 3, 2023

Choose a reason for hiding this comment

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

No strong opinion from my side (@vincepri?)

I was just wondering if we have to go through the schemeBuilder if we only export the AddToScheme func as we could do that in a simpler way.

@sbueringer
Copy link
Member

One nit. Let's keep the hold just to discuss that one

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2023
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 4, 2023
@JoelSpeed
Copy link
Contributor Author

Added some documentation to explain the new pattern, PTAL

@k8s-ci-robot
Copy link
Contributor

@JoelSpeed: 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-cluster-api-apidiff-main 8b44611 link false /test pull-cluster-api-apidiff-main

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.

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

Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 90cd36d59ab67ec1d6b2fa806f58ec48b8a60e88

@killianmuldoon
Copy link
Contributor

@sbueringer - if you're happy with the current state I think we can unhold this - just waiting on #9045 (comment)

@sbueringer
Copy link
Member

sbueringer commented Aug 7, 2023

Let's go ahead with this. It seems like unnecessary complexity if we don't expose the schemeBuilder. But we can still implement my suggested change later if we ever want to, it doesn't change anything that we export.

Might be wort opening an issue in the kubebuilder repository as a follow-up. Maybe they want to change the way they generate API files so other folks benefit from reduced dependencies as well.

Thx for the nice documentation!!

/approve
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 7, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [killianmuldoon,sbueringer]

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

@k8s-ci-robot k8s-ci-robot merged commit 77935f6 into kubernetes-sigs:main Aug 7, 2023
11 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Aug 7, 2023

[scheme]: https://pkg.go.dev/k8s.io/apimachinery/pkg/runtime#Scheme

By default, Kubebuilder will provide you with a scheme builder like:
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an issue in Kubebuilder to try steer away from the current pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet, I'll make one

In general, you should minimise the imports within the API folder of your package to allow your API types
to be imported cleanly into other projects.

To mitigate this, use the following schemebuilder pattern:
Copy link
Member

Choose a reason for hiding this comment

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

cc @furkatgofurov7 In case we might want to include this as an optional action item in the v1.6 release

Copy link
Member

Choose a reason for hiding this comment

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

@vincepri For the provider migration doc, right? Sounds like a good idea!

Copy link
Member

Choose a reason for hiding this comment

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

Correct yeah

Copy link
Member

Choose a reason for hiding this comment

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

@vincepri @sbueringer +1 to add it to the provider migration doc.

Opened #9146 to cover that, PTAL and let me know what you think, thanks!

@g-gaston
Copy link
Contributor

/area api

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/api Issues or PRs related to the APIs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants