Move cluster name validation to Validate method #1967
Conversation
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @knabben. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-ci-robot
added
cncf-cla: yes
| "'%s' is not a valid cluster name, cluster names must match `%s`", | ||
| opts.Config.Name, validNameRE.String(), | ||
| ) | ||
| } | ||
| // warn if cluster name might typically be too long | ||
| if len(opts.Config.Name) > clusterNameMax { |
There was a problem hiding this comment.
should this warning be moved to the validation as well?
There was a problem hiding this comment.
maybe, I'm not sure if it makes sense for Validate() to warn, and this is not inherent in the API (we don't enforce this restriction ourselves), it's a limitation of the underlying implementation currently
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
@neolit123 can you take a look at it? thanks |
|
sorry I didn't get back to this myself |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, knabben 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
|
/retest |
|
/test pull-kind-e2e-kubernetes |
Quick chore: moving the name regex parsing to
cluster.Validate()method