Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update validation text msg for legacy name fields #718

Merged
merged 1 commit into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ const useMappingFormState = (
useFormState({
name: useFormField(
'',
getMappingNameSchema(mappingsQuery, mappingBeingEdited).label('Name').required()
getMappingNameSchema(mappingsQuery, mappingBeingEdited).label('Mapping Name').required()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference, please do minimal changes when fixing the legacy code, this change can be avoided.

Copy link
Collaborator Author

@sgratch sgratch Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

),
sourceProvider: useFormField<SourceInventoryProvider | null>(
null,
Expand Down
2 changes: 1 addition & 1 deletion packages/legacy/src/Plans/components/Wizard/PlanWizard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export type PlanWizardMode = 'create' | 'edit' | 'duplicate';

const useMappingFormState = (mappingsQuery: UseQueryResult<IKubeList<Mapping>>) => {
const isSaveNewMapping = useFormField(false, yup.boolean().required());
const newMappingNameSchema = getMappingNameSchema(mappingsQuery, null).label('Name');
const newMappingNameSchema = getMappingNameSchema(mappingsQuery, null).label('Mapping Name');
const isCreateMappingSelected = useFormField(false, yup.boolean().required());
const selectedExistingMapping = useFormField<Mapping | null>(null, yup.mixed<Mapping | null>());
const isPrefilled = useFormField(false, yup.boolean());
Expand Down
6 changes: 3 additions & 3 deletions packages/legacy/src/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,10 @@ export enum StepType {

export const dnsLabelNameSchema = yup
.string()
.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])?)*$/, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@sgratch sgratch Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yaacov Please note that without this fix:

  1. The validation will be for 63 max len instead of 253
  2. a dedicated error message of: 'xxx name must be at most 253 characters' won't appear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm missing the "for more information see k8s documention"

I'll add it in a folowup

Copy link
Collaborator Author

@sgratch sgratch Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

excludeEmptyString: true,
});

Expand Down