-
Notifications
You must be signed in to change notification settings - Fork 163
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
Use K8sNameDescriptionField in connection type page #3257
Use K8sNameDescriptionField in connection type page #3257
Conversation
Skipping CI for Draft Pull Request. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3257 +/- ##
==========================================
- Coverage 84.96% 84.95% -0.02%
==========================================
Files 1302 1302
Lines 29101 29099 -2
Branches 7828 7827 -1
==========================================
- Hits 24727 24722 -5
- Misses 4374 4377 +3
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can still submit the form if there are errors with the resource name.
frontend/src/concepts/k8s/utils.ts
Outdated
export const isK8sDSGResource = (x?: K8sResourceCommon): x is K8sDSGResource => !!x?.metadata?.name; | ||
export const isK8sDSGResource = (x?: K8sResourceCommon): x is K8sDSGResource => | ||
!!x?.metadata && 'name' in x.metadata; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For prefilling the k8sNameDesc fields, it runs the following:
export const setupDefaults = ({
initialData,
limitNameResourceType,
safePrefix,
}: UseK8sNameDescriptionDataConfiguration): K8sNameDescriptionFieldData => {
let initialName = '';
let initialDescription = '';
let initialK8sNameValue = '';
let configuredMaxLength = MAX_RESOURCE_NAME_LENGTH;
if (isK8sDSGResource(initialData)) {
initialName = getDisplayNameFromK8sResource(initialData);
initialDescription = getDescriptionFromK8sResource(initialData);
initialK8sNameValue = initialData.metadata.name;
}
if (limitNameResourceType != null) {
configuredMaxLength = ROUTE_BASED_NAME_LENGTH;
}
So specifically for the duplicate scenario, there will be data like the description and display name filled out, but no resource name is defined yet, and it will fail the check since "" is false. This change makes "" still count as true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I reverted this change due to other changes anyways. For duplicate, it will now pass in the resource name, so the type check will always pass.
Passing in the resource name would originally block you from editing it, but i added editK8sNameValue
to allow edit of the prefilled value
2d97d04
to
47aabd3
Compare
2f10602
to
d08c3a4
Compare
d08c3a4
to
27bc5fa
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt 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 |
Closes https://issues.redhat.com/browse/RHOAIENG-13400
Description
Use the new
K8sNameDescriptionField
component. This prevents the user from entering a too long of a name which causes a resource name error when saving.How Has This Been Tested?
Tested locally by running through the different connection type operations.
For the bug referenced in the jira issue, paste a lot of text https://www.lipsum.com/feed/html and click save. It will now truncate the resource name
Test Impact
No updates, existing tests cover the previous functionality. (and the long name is a server side error)
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main