⚠️ Move kubeadm controllers to internal #5493
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
| // Following types provides access to reconcilers implemented in internal/controllers, thus | ||
| // allowing users to provide a single binary "batteries included" with Cluster API and providers of choice. |
There was a problem hiding this comment.
After some thinking about how to make it possible for users to create single binary "batteries included", I decided to keep the controllers folders, which makes the project nicely consistent with kubebuilder scaffolding, but to leave here only an alias / a proxy type that make it public a small surface of internal/controllers.
In perspective, this will give us more freedom in shifting around our internal/controllers if required, eg. by moving a set of reconcilers to subfolders, without surfacing this in our public surface.
Looking forward for opinions about this approach.
| @@ -59,6 +59,10 @@ const ( | |||
| KubeadmConfigControllerName = "kubeadmconfig-controller" | |||
| ) | |||
|
|
|||
| var ( | |||
| defaultTokenTTL = 15 * time.Minute | |||
There was a problem hiding this comment.
Could we export this bit and reuse it when we declare the flag?
|
+1 on the approach, seems like the proposed changes give us a clean api surface |
|
I think this makes sense. It gives us flexibility and clear boundaries between public/internal. |
fabriziopandini
force-pushed
the
move-kubeadm-controllers-to-internal
branch
from
22c8b37 to
34ed185
Compare
|
/test pull-cluster-api-test-main |
fabriziopandini
force-pushed
the
move-kubeadm-controllers-to-internal
branch
from
34ed185 to
c1c6bee
Compare
|
@fabriziopandini: The following test failed, say
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. |
|
Thanks! /lgtm
Not sure about this. I previously assumed every breaking change should have the breaking change icon to mark it accordingly. From a consumer perspective it is a breaking change, imho. But I'll stay out of this discussion :) |
k8s-ci-robot
added
the
lgtm
fabriziopandini
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
Note: this PR is not marked as breaking given that we agreed on the criteria on beforehand. However, happy to change icon if we prefer to surface this in the release note.
Which issue(s) this PR fixes:
Rif #5455