refactor: keep only common types in Generic config specs #1297
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…w type This allows for EKS clusters to reuse the actually generic types.
dkoshkin
force-pushed
the
dkoshkin/refactor-GenericClusterConfigSpec
branch
from
2374cf8 to
5a6ccd8
Compare
dkoshkin
changed the title
jimmidyson
reviewed
Sep 16, 2025
jimmidyson
reviewed
Sep 16, 2025
faiq
approved these changes
Sep 16, 2025
|
nice work! |
dkoshkin
added a commit
that referenced
this pull request
Sep 16, 2025
**What problem does this PR solve?**: Stacked on #1297 This fixes the generic handlers for EKS clusters by properly exposing the `GenericClusterConfigSpec` API in `EKSClusterConfig`. The following fields are still aspirational and are not properly supported in the handlers for EKS, but they can be. ``` // +kubebuilder:validation:Optional Proxy *HTTPProxy `json:"proxy,omitempty"` // +kubebuilder:validation:Optional // +kubebuilder:validation:MaxItems=32 ImageRegistries []ImageRegistry `json:"imageRegistries,omitempty"` // +kubebuilder:validation:Optional GlobalImageRegistryMirror *GlobalImageRegistryMirror `json:"globalImageRegistryMirror,omitempty"` ``` But these are working and I've added broken tests to verify before fixing the API. ``` // +kubebuilder:validation:Optional // +kubebuilder:validation:MaxItems=32 Users []User `json:"users,omitempty"` // NTP defines the NTP configuration for the cluster. // +kubebuilder:validation:Optional NTP *NTP `json:"ntp,omitempty"` ``` **Which issue(s) this PR fixes**: Fixes # **How Has This Been Tested?**: <!-- Please describe the tests that you ran to verify your changes. Provide output from the tests and any manual steps needed to replicate the tests. --> **Special notes for your reviewer**: <!-- Use this to provide any additional information to the reviewers. This may include: - Best way to review the PR. - Where the author wants the most review attention on. - etc. -->
Merged
dkoshkin
added a commit
that referenced
this pull request
Oct 6, 2025
**What problem does this PR solve?**: Moving the handlers and docs around to make it clear when adding new providers and new handlers if they apply to all providers or only those that use kubeadm too bootstrap the Nodes. This better aligns with the changes in #1297 **Which issue(s) this PR fixes**: Fixes # **How Has This Been Tested?**: <!-- Please describe the tests that you ran to verify your changes. Provide output from the tests and any manual steps needed to replicate the tests. --> **Special notes for your reviewer**: <!-- Use this to provide any additional information to the reviewers. This may include: - Best way to review the PR. - Where the author wants the most review attention on. - etc. -->
vijayaraghavanr31
pushed a commit
that referenced
this pull request
Oct 8, 2025
**What problem does this PR solve?**: Moving the handlers and docs around to make it clear when adding new providers and new handlers if they apply to all providers or only those that use kubeadm too bootstrap the Nodes. This better aligns with the changes in #1297 **Which issue(s) this PR fixes**: Fixes # **How Has This Been Tested?**: <!-- Please describe the tests that you ran to verify your changes. Provide output from the tests and any manual steps needed to replicate the tests. --> **Special notes for your reviewer**: <!-- Use this to provide any additional information to the reviewers. This may include: - Best way to review the PR. - Where the author wants the most review attention on. - etc. -->
This was referenced Oct 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?:
After EKS types were introduced, the fields in
GenericClusterConfigSpecandGenericNodeSpecare no longer all "generic" because some do not apply to EKS clusters, as it does not use kubeadm to create the cluster.This PR attempts to address this by only keeping the fields that can be set for both EKS and all other infra types in
GenericClusterConfigSpecand moves the rest of the fields toKubeadmClusterConfigSpec, and similarly forGenericNodeSpec.Because the structs are included
inlinein the individual Cluster types this does not change the external yaml API, only how they are referenced in Go (notice how the CRDs for <aws|docker|nutanix>clusterconfigs did not change).Which issue(s) this PR fixes:
Fixes #
How Has This Been Tested?:
Unit tests.
Special notes for your reviewer:
If we agree with this approach I will have separate refactor PRs to reorganize the handlers and the public docs.