-
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
🌱 Clean up kube version parsing #4163
🌱 Clean up kube version parsing #4163
Conversation
03348fa
to
10d7c8b
Compare
linking this issues because related #3697 |
/milestone v0.4.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.
Overall great work, thanks @CecileRobertMichon for tackling this!
controlplane/kubeadm/api/v1alpha4/kubeadm_control_plane_webhook.go
Outdated
Show resolved
Hide resolved
Thanks for linking #3697 @fabriziopandini, I knew there was an issue for this but I had lost it... I think we should merge this refactor first which doesn't make any changes to validation then make any necessary changes separately so we don't mix refactoring and validation changes. |
2b7241d
to
67618d8
Compare
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
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 |
What this PR does / why we need it: Currently version parsing in CAPI is inconsistent and there is duplicate constants for regex expression. Also, KCP webhooks (kubeadm/controlplane/api) import the
util
package to use the version parsing helper and kubeSemver which means the util package is unable import the KCP api (it already imports the core API) as it causes dependency cycles. This PR moves the version logic to a new util/version package and deprecates the func in the util package, as well as reuses the same kubeSemver var across all uses in the code. It also makes the difference between "tolerant" semver parsing (with or without "v" prefix) and not ("v" prefix required), which otherwise is easy to miss as it's a 1 char difference in the regex. There are no behavior/functional changes as part of this PR.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Partially addresses #3697