Update validation text msg for legacy name fields #718
Conversation
Update the validation error message for the 'mapping name', 'plan name' and 'namespace name' fields (legacy code), to be aligned with k8s rules. This is a follow up for kubev2v#715. Signed-off-by: Sharon Gratch <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
yaacov
added
the
kind/cleanup
| @@ -64,7 +64,7 @@ const useMappingFormState = ( | |||
| useFormState({ | |||
| name: useFormField( | |||
| '', | |||
| getMappingNameSchema(mappingsQuery, mappingBeingEdited).label('Name').required() | |||
| getMappingNameSchema(mappingsQuery, mappingBeingEdited).label('Mapping Name').required() | |||
There was a problem hiding this comment.
For future reference, please do minimal changes when fixing the legacy code, this change can be avoided.
There was a problem hiding this comment.
@yaacov Without this change, the error message will not include the entity name, i.e. it will be displayed for mapping field as "Name should be..." instead of "Mapping name should be..."
For Plan and namespace it was already set, so it's nicer to do the same for mapping as well.
There was a problem hiding this comment.
yes, but this is the legacy code, we should not touch it if we don't need to, in this case we don't need to.
| .max(63) | ||
| .matches(/^(?=.{1,253}$)[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$/, { | ||
| .max(253) | ||
| .matches(/^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$/, { |
There was a problem hiding this comment.
it was nice to keep the exact regexp as the new code, but this works too, for next time, please keep changes in the legacy code to minimum.
There was a problem hiding this comment.
@yaacov Please note that without this fix:
- The validation will be for 63 max len instead of 253
- a dedicated error message of: 'xxx name must be at most 253 characters' won't appear.
There was a problem hiding this comment.
The validation will be for 63 max len instead of 253
63 id fine, this code is working since the begining, it hasn't bothered anyone until now, and will not in the future.
a dedicated error message of: 'xxx name must be at most 253 characters' won't appear.
that is fine too, name is good enough, again this code worked from the first version of this application.
note:
when we replace this code, we will make it nicer, we have an issue to check validations and help texts in the provider forms #646, and we will have a similar task once we replace the mapping and plan forms.
on the other hand, we should not touch the legacy code, we don't want to maintain it, and every change we do, means we own the new code change and need to maintain it.
| message: ({ label }) => | ||
| `${label} can only contain lowercase alphanumeric characters, dashes and dots, and must start and end with an alphanumeric character, see k8s documentation for more details.`, | ||
| `${label} must be a valid Kubernetes name (i.e., must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).`, |
There was a problem hiding this comment.
oww, didn't notice the , must contain no more than 253 characters that is too much information for a help text, more like a "learn more" thing,
I'll remove it in a folowup
There was a problem hiding this comment.
I'm missing the "for more information see k8s documention"
I'll add it in a folowup
There was a problem hiding this comment.
This is not added for the provider name error message. I think we should try being consisted.
Once we say a valid k8s name, it's implies that you can check the docs, but anyway it's nice to be added as well.
If you are convinced, I'll add for both the legacy and the providers name text msg, something like the following?: "For more information, see the Kubernetes documentation"
In separate PRs :)
A followup fix for: #646
References: #715
Update the validation error message for the '
mapping name', 'plan name' and 'namespace name' fields (legacy code), to be aligned with k8s rules.This is a follow up for #715.