fix to collapse consecutive dashes, strip edge dashes, and cap k8s name at 63 chars#2642
fix to collapse consecutive dashes, strip edge dashes, and cap k8s name at 63 chars#2642Taj010 wants to merge 2 commits intokubeflow:mainfrom
Conversation
Signed-off-by: Taj010 <rsheen003@gmail.com>
manaswinidas
left a comment
manaswinidas
left a comment
There was a problem hiding this comment.
Thanks for tackling this! The core dash-handling fix is solid. A few suggestions to simplify and align with the existing shared utility.
| } | ||
| const alphanumeric = out.replace(/[^a-z0-9]/g, ''); | ||
| return (alphanumeric.length >= 4 ? alphanumeric : `${alphanumeric}x0`).slice(0, 16); | ||
| }; |
There was a problem hiding this comment.
This function and the modified translateDisplayNameForK8s below can be dropped entirely. The full version — with dash collapsing, maxLength, safeK8sPrefix, digit-start protection, and empty-name gen- fallback — already exists in this repo at ~/app/shared/components/utils.ts.
Just import from there:
import { translateDisplayNameForK8s } from '~/app/shared/components/utils';handleUpdateLogic in this file also calls translateDisplayNameForK8s, so both call sites benefit from the switch.
There was a problem hiding this comment.
Adjusted this! Removed the local implementation and now import from ~/app/shared/components/utils.
| registryName, | ||
| }: RegisterAndStoreFieldsProps<D>): React.ReactNode => { | ||
| const autoGeneratedName = React.useMemo( | ||
| () => translateDisplayNameForK8s(formData.jobName), |
There was a problem hiding this comment.
The shared translateDisplayNameForK8s (from ~/app/shared/components/utils) uses genRandomChars() for the empty-name fallback, so calling it inside useMemo will produce a different value on every recompute — breaking the isTouched comparison.
Instead, push name generation through handleUpdateLogic/onDataChange (generate once, store in form state) like odh-dashboard does. In odh-dashboard's handleUpdateLogic, maxLength is read from existingData.k8sName.state and passed to translateDisplayNameForK8s automatically — so maxLength: 63 flows through without needing to pass it at the direct call site. The random value is computed once on name change and persisted, not re-derived every render.
There was a problem hiding this comment.
Addressed — removed render-time translation; generation now happens via handleUpdateLogic/onDataChange and is persisted in form state.
| invalidLength: resourceName.length > 253, | ||
| maxLength: 253, | ||
| invalidLength: resourceName.length > 63, | ||
| maxLength: 63, |
There was a problem hiding this comment.
The 253 → 63 fix is correct and should stay. 👍
| it('prepends safePrefix when name normalizes to empty', () => { | ||
| expect(translateDisplayNameForK8s('!!!', 'safe-')).toBe('safe-'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
If translateDisplayNameForK8s is imported from the shared util instead of being defined locally, these tests become redundant — the shared version already has comprehensive test coverage. Consider testing the RegisterAndStoreFields wiring (e.g. 63-char limit, touched detection) instead.
Also note: expect(translateDisplayNameForK8s('!!!', 'safe-')).toBe('safe-') produces a name ending with -, which isn't valid for K8s. The shared version handles this correctly by appending genRandomChars() after the prefix.
There was a problem hiding this comment.
Addressed — removed redundant K8sNameDescriptionField util tests and added RegisterAndStoreFields wiring tests (63-char limit + touched/manual-name behavior).
| * translateDisplayNameForK8sAndReport in app/shared/components/utils.ts, without per-call randomness). | ||
| * @see https://kubernetes.io/docs/concepts/overview/working-with-objects/names/ | ||
| */ | ||
| export const translateDisplayNameForK8s = (name = '', safePrefix = ''): string => { |
There was a problem hiding this comment.
We should get rid of this copy altogether
There was a problem hiding this comment.
Removed the local re-export
Signed-off-by: Taj010 <rsheen003@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
2 participants
Description
This PR addresses two issues with auto-generated Kubernetes resource names in the model registry UI:
Bad slugs from display names —
translateDisplayNameForK8snow strips leading/trailing dashes, collapses consecutive dashes, and returns a stablegen-<suffix>when the name would otherwise be empty (e.g. punctuation- or dash-only input), matching the main dashboard behavior.Client vs BFF length mismatch — The register-and-store transfer job field now enforces a 63 character limit (was 253) so validation matches the BFF. Memo deps for
autoGeneratedName/isTouchedare fixed; the redundantuseEffectwas removed.How Has This Been Tested?
Manually tested UI and ran
npm run test:unit -- --testPathPattern=K8sNameDescriptionFieldandnpm run test:type-check.Merge criteria:
DCOcheck)ok-to-testhas been added to the PR.If you have UI changes