-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Allow control plane provider to set endpoint #10667
🌱 Allow control plane provider to set endpoint #10667
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR!
Eventually, once the ClusterEndpoint proposal is implemented, those endpoints will superseded this change.
What about adding a note on top of https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20230407-flexible-managed-k8s-endpoints.md so we keep track we have implemented this / it should be considered when someone pick up
the implementation
I also think we should fix documentation in:
- https://cluster-api.sigs.k8s.io/developer/architecture/controllers/control-plane#relationship-to-other-cluster-api-types (it is not true anymore that controlPlane providers have to wait for controlPlaneEndpoint, it depends now...)
- https://cluster-api.sigs.k8s.io/developer/architecture/controllers/control-plane#crd-contracts (we should add the controlPlane endpoint as optional)
3dedc70
to
686a919
Compare
@fabriziopandini done, ptal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last nit.
If possible, please add a note in https://cluster-api.sigs.k8s.io/developer/architecture/controllers/cluster#infrastructure-provider indicating that the control plane endpoint might be not provided by the infrastructure cluster (sorry if I forgot to list this page in the first pass)
docs/book/src/developer/architecture/controllers/control-plane.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/architecture/controllers/control-plane.md
Outdated
Show resolved
Hide resolved
docs/book/src/developer/architecture/controllers/control-plane.md
Outdated
Show resolved
Hide resolved
The change seems reasonable to me. |
686a919
to
e3f594d
Compare
@sbueringer & others, ptal |
/lgtm |
LGTM label has been added. Git tree hash: d6f31e3d259613f74ca4b67474655243b96724e9
|
/approve Besides the naming nit /hold in case @sbueringer or others want to take a look too. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrischdi 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 |
e3f594d
to
309b262
Compare
309b262
to
6b230b8
Compare
/lgtm |
LGTM label has been added. Git tree hash: 26cb7a5167caecaf21b06da10ceca36bf64c3990
|
Signed-off-by: Vince Prignano <[email protected]>
6b230b8
to
d4c61cd
Compare
/hold cancel |
@sbueringer @enxebre ready for another review |
/lgtm |
LGTM label has been added. Git tree hash: dba32f9fff1ebb273d4fe2b0463a6dfedb3c6356
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold cancel |
/lgtm from my side as well |
/cherry-pick release-1.7 |
@vincepri: #10667 failed to apply on top of branch "release-1.7":
In response to this:
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-sigs/prow repository. |
What this PR does / why we need it:
/area api
This small change allows the controlPlaneRef, if set, to conditionally set the controlPlaneEndpoint. This change is a short term solution that allows the control plane provider to be on parity with the infrastructure provider, and helps reduce the heavy requirement on the infrastructureRef we have today.
Eventually, once the
ClusterEndpoint
proposal is implemented, those endpoints will superseded this change.