-
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
✨ [CABPK] Support using kubeadm v1beta2 types internally #4292
✨ [CABPK] Support using kubeadm v1beta2 types internally #4292
Conversation
switch { | ||
case semVersion.LessThan(versionutil.MustParseSemantic("v1.13.0")): | ||
return schema.GroupVersion{}, errors.New("the bootstrap provider for kubeadm doesn't support Kubernetes version lower than v1.13.0") | ||
case semVersion.LessThan(versionutil.MustParseSemantic("v1.15.0")): |
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.
off topic:
- where can one find the minimum supported version of CAPBK in docs or YAML?
- does CABK error out if the user version is older than the minimum?
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.
see https://cluster-api.sigs.k8s.io/reference/versions.html
With this change CABPK will error out for Kubernetes < v1.13, while before things were failing in the cloud init script (which is much more complex to debug).
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.
Let's move the checks above to a validation webhook?
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.
I will open an issue to discuss this improvement.
The problem here is that this is a CABPK limitation, but the version could be defined on another resource (e.g. Machine deployments)
For the sake of this PR this is a little bit out of topic and this will also deviate from the original PR we are back porting.
Nevertheless we are already improving the status quo, so I hop this should not be a blocker
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.
rif #4321
b89a6c3
to
c57d21e
Compare
/retitle ✨ [CABPK] Support using kubeadm v1beta2 types |
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
@CecileRobertMichon over to you
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon 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 |
/test pull-cluster-api-test-release-0-3 |
What this PR does / why we need it:
backport of #4227
This PR implements a stop-gap solution for kubeadm types removal explicitly designed for back-porting on the v1alpha3 branch.
The solution implemented is described in the #4170 (and more specifically this is alternative 2 for v1alpha3).
A proper conversion mechanism is going to be implemented in v1alpha4 only (also described in the proposal).
Which issue(s) this PR fixes:
Fixes #
/milestone v0.3.x