🌱 Improve Cluster variable defaulting/validation errors

[sbueringer]

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:

Before

The Cluster "my-cluster-2" is invalid: spec.topology.variables: Invalid value: []v1beta1.ClusterVariable{v1beta1.ClusterVariable{Name:"imageRepository", DefinitionFrom:"", Value:v1.JSON{Raw:[]uint8{0x22, 0x6b, 0x38, 0x73, 0x2e, 0x67, 0x63, 0x72, 0x2e, 0x69, 0x6f, 0x22}}}, v1beta1.ClusterVariable{Name:"imageRepository", DefinitionFrom:"", Value:v1.JSON{Raw:[]uint8{0x22, 0x6b, 0x38, 0x73, 0x2e, 0x67, 0x63, 0x72, 0x2e, 0x69, 0x6f, 0x22}}}, v1beta1.ClusterVariable{Name:"lbImage", DefinitionFrom:"", Value:v1.JSON{Raw:[]uint8{0x7b, 0x22, 0x69, 0x6d, 0x61, 0x67, 0x65, 0x52, 0x65, 0x70, 0x6f, 0x73, 0x69, 0x74, 0x6f, 0x72, 0x79, 0x22, 0x3a, 0x22, 0x6b, 0x69, 0x6e, 0x64, 0x65, 0x73, 0x74, 0x22, 0x2c, 0x22, 0x69, 0x6d, 0x61, 0x67, 0x65, 0x54, 0x61, 0x67, 0x22, 0x3a, 0x22, 0x76, 0x32, 0x30, 0x32, 0x31, 0x30, 0x37, 0x31, 0x35, 0x2d, 0x61, 0x36, 0x64, 0x61, 0x33, 0x34, 0x36, 0x33, 0x22, 0x7d}}}, v1beta1.ClusterVariable{Name:"controlPlaneTaint", DefinitionFrom:"", Value:v1.JSON{Raw:[]uint8{0x66, 0x61, 0x6c, 0x73, 0x65}}}}: cluster variables not valid: variable "imageRepository" from "" is defined more than once

After

The Cluster "my-cluster-2" is invalid: spec.topology.variables: Invalid value: "[Name: imageRepository DefinitionFrom: ,Name: imageRepository DefinitionFrom: ,Name: lbImage DefinitionFrom: ,Name: controlPlaneTaint DefinitionFrom: ]": cluster variables not valid: variable "imageRepository" from "" is defined more than once

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 #

[sbueringer]

TIL: Technically we produce this error during defaulting and validation, so we have two of them. But this gets deduplicated during error aggregation in apierrors.NewInvalid

[chrischdi]

The Cluster "my-cluster-2" is invalid: spec.topology.variables: Invalid value: "[Name: imageRepository DefinitionFrom: ,Name: imageRepository DefinitionFrom: ,Name: lbImage DefinitionFrom: ,Name: controlPlaneTaint DefinitionFrom: ]": cluster variables not valid: variable "imageRepository" from "" is defined more than once

Is DefinitionFrom always empty?

If yes I think we should not add all this and the error may be sufficient?

otherwise lgtm

[sbueringer]

The Cluster "my-cluster-2" is invalid: spec.topology.variables: Invalid value: "[Name: imageRepository DefinitionFrom: ,Name: imageRepository DefinitionFrom: ,Name: lbImage DefinitionFrom: ,Name: controlPlaneTaint DefinitionFrom: ]": cluster variables not valid: variable "imageRepository" from "" is defined more than once

Is DefinitionFrom always empty?

If yes I think we should not add all this and the error may be sufficient?

otherwise lgtm

No it is not always empty. It can also vary between variables (we also have it in other errors and it can be crucial to understand what is wrong). But I agree it is confusing in the cases where it is empty. Maybe we should do a follow-up to audit all our variable errors to try to simplify in cases where it is empty and we can.

I could also implement basically an "omitempty" in this error.

[sbueringer]

@killianmuldoon WDYT?

[sbueringer]

@killianmuldoon ^^

[killianmuldoon]

/area clusterclass

[killianmuldoon]

IMO this is a really big improvement as-is, and I don't think we need to complicate it.

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 511bb48957a192cc440e5e2c3f7e63c8ed925d3e

[sbueringer]

Alrighty. Let's go with that one as is then

/approve

[sbueringer]

I would cherry-pick into release-1.5 as it's a significant improvement to our error messages and it falls under the "experimental feature" clause of our backport policy

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

• OWNERS [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sbueringer]

/cherry-pick release-1.5

[k8s-infra-cherrypick-robot]

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.5 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.5

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/test-infra repository.

[k8s-infra-cherrypick-robot]

@sbueringer: new pull request created: #9479

In response to this:

/cherry-pick release-1.5

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/test-infra repository.