🌱 ClusterClass: validate MachineDeployment topology name not empty #5951
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
requested review from
fabriziopandini,
killianmuldoon and
stmcginnis
k8s-ci-robot
added
the
size/M
internal/webhooks/cluster.go
Outdated
| @@ -215,6 +215,21 @@ func (webhook *Cluster) validateTopology(ctx context.Context, oldCluster, newClu | |||
| return allErrs | |||
| } | |||
|
|
|||
| // MachineDeploymentTopology name should be defined. | |||
| if newCluster.Spec.Topology.Workers != nil { | |||
There was a problem hiding this comment.
Would this check be better in check. MachineDeploymentTopologiesAreUniqueAndDefinedInClusterClass ? It's where we're doing the rest of the checks for the MDs (and it means we only have to parse the list once)
There was a problem hiding this comment.
It felt to me like it wouldn't fit into the name as the name is very specifically describing what it is validating.
WDYT about MachineDeploymentTopologiesAreValidAndDefinedInClusterClass
with that name, I agree let's move it there.
There was a problem hiding this comment.
Yeah the name change is good with me - there's no need for it to be that explicit as long as the comment is clear.
Signed-off-by: Stefan Büringer [email protected]
sbueringer
force-pushed
the
pr-validate-md-topology-name
branch
from
ac7667e to
7c3d171
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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
Signed-off-by: Stefan Büringer [email protected]
What this PR does / why we need it:
Setting the MD topology name to an empty string leads to errors during reconcile, so this PR now
validates that it is not empty.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #