EDM-2925: Adding helm application type #457
Conversation
WalkthroughAdds Helm application support across types, UI, validation, serialization, and translations; introduces AppTypeHelm, Helm form component, Helm fields in specs, YAML validation/parsing, UploadField accessibility changes, and UI/labeling updates. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@libs/i18n/locales/en/translation.json`:
- Line 391: Fix the typo in the translation entry whose key/value is "Order of
precedence: Files are applied in the ordered listed. If the same parameter is
defined in multiple files, the value in the last file takes precedence and
overrides previous values." — change "ordered listed" to "listed order" (or
"order listed") consistently in both the JSON key and its value so the
user-facing string reads correctly (e.g., "Order of precedence: Files are
applied in the listed order. ...").
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx`:
- Around line 60-66: The reset logic misses the case where appType is Helm but
specType isn't OCI_IMAGE, so switching from INLINE to Helm won't mark the app as
incomplete; update the Helm detection and incomplete check: modify isHelm (from
isHelmImageAppForm) to also return true when app.appType === AppType.HELM and
specType !== AppSpecType.OCI_IMAGE, and keep isHelmIncomplete = isHelm &&
!('valuesFiles' in app) (or add an explicit check for appType === AppType.HELM
in isHelmIncomplete) so shouldResetApp becomes true in that transition
(referencing isHelmImageAppForm, isHelmIncomplete, shouldResetApp, specType,
AppSpecType, and app.appType).
In `@libs/ui-components/src/components/form/UploadField.tsx`:
- Line 92: The aria-label is currently applied to FormGroup in UploadField; move
it to the actual control by removing aria-label from the FormGroup (keep
fieldId/isRequired) and pass aria-label into the FileUpload component instead
(ensure prop name matches the local variable, e.g., ariaLabel). Update the JSX
in UploadField so FileUpload receives ariaLabel and the FormGroup only handles
fieldId/isRequired/label concerns to follow PatternFly accessibility best
practices.
🧹 Nitpick comments (3)
libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (1)
485-488: Consider using empty object fallback instead of whitespace string for YAML parsing.The
' '(whitespace) fallback foryaml.loadparses tonull, which then gets stringified as"null"byJSON.stringify. This could cause false positives when comparing against an empty{}fromimageApp.values || {}.♻️ Suggested improvement
- const updatedValues = yaml.load(updatedApp.valuesYaml || ' ') as Record<string, unknown>; - if (JSON.stringify(imageApp.values || {}) !== JSON.stringify(updatedValues)) { + const updatedValues = updatedApp.valuesYaml + ? (yaml.load(updatedApp.valuesYaml) as Record<string, unknown>) + : {}; + if (JSON.stringify(imageApp.values || {}) !== JSON.stringify(updatedValues || {})) {libs/ui-components/src/types/deviceSpec.ts (1)
125-144: Type guards correctly updated forAppFormunion.The signature change from
AppBasetoAppFormis correct since these guards discriminate members of theAppFormunion type.However,
getAppIdentifier(lines 155-164 in this file) doesn't have an explicit branch forHelmImageAppForm. Since Helm apps have optionalnameand mandatoryimage, they should follow the same pattern as other image apps.♻️ Suggested fix for getAppIdentifier
export const getAppIdentifier = (app: AppForm) => { if (isSingleContainerAppForm(app)) { return app.name || app.image; } - if (isQuadletImageAppForm(app) || isComposeImageAppForm(app)) { + if (isQuadletImageAppForm(app) || isComposeImageAppForm(app) || isHelmImageAppForm(app)) { return app.name || app.image; } // Name is mandatory for inline applications return app.name; };libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsx (1)
144-151: Consider addingmaxFileByteslimit for consistency.The
UploadFieldhere doesn't specifymaxFileBytes, whileApplicationInlineFormusesMAX_INLINE_FILE_SIZE_BYTES(1MB). Large YAML values could cause issues.♻️ Optional: Add file size limit
- <UploadField ariaLabel={t('Inline values')} name={`${appFieldName}.valuesYaml`} isDisabled={isReadOnly} /> + <UploadField + ariaLabel={t('Inline values')} + name={`${appFieldName}.valuesYaml`} + isDisabled={isReadOnly} + maxFileBytes={1024 * 1024} // 1MB limit + />
| "Delete values file": "Delete values file", | ||
| "Fill in the existing values files before you can add more values files.": "Fill in the existing values files before you can add more values files.", | ||
| "Add values file": "Add values file", | ||
| "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.", |
There was a problem hiding this comment.
Fix typo in user-facing string.
"ordered listed" should be "order listed" or "listed order".
✏️ Proposed fix
-"Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.",
+"Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.", | |
| "Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.", |
🤖 Prompt for AI Agents
In `@libs/i18n/locales/en/translation.json` at line 391, Fix the typo in the
translation entry whose key/value is "Order of precedence: Files are applied in
the ordered listed. If the same parameter is defined in multiple files, the
value in the last file takes precedence and overrides previous values." — change
"ordered listed" to "listed order" (or "order listed") consistently in both the
JSON key and its value so the user-facing string reads correctly (e.g., "Order
of precedence: Files are applied in the listed order. ...").
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsx
Show resolved
Hide resolved
celdrake
force-pushed
the
EDM-2925-helm-application-type
branch
from
468d2df to
5151f6b
Compare
celdrake
force-pushed
the
EDM-2925-helm-application-type
branch
from
5151f6b to
95640a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsx`:
- Around line 29-31: The FormGroup for the application name is marked isRequired
in EditDeviceWizard (FormGroup with label t('Application name') containing
TextField name `${appFieldName}.name`) but the Helm validation schema
(validApplicationAndVolumeName(t) used in validations.ts around the Helm app
schema) does not call .required(), causing a mismatch; fix by either removing
the isRequired prop from that FormGroup/TextField if the name is optional, or
update the Helm validation to require the field by chaining .required() onto
validApplicationAndVolumeName(t) in the Helm schema so the UI and validation
agree.
♻️ Duplicate comments (2)
libs/ui-components/src/components/form/UploadField.tsx (1)
92-128: Move the accessible label to the control, not the FormGroup.
aria-labelonFormGroupdoesn’t label the actual input;FileUploadshould receive it directly, andfieldIdshould match the controlid. This matches prior guidance.🔧 Suggested fix
- <FormGroup fieldId={fieldId} aria-label={ariaLabel} isRequired={isRequired}> + <FormGroup fieldId={fieldId} isRequired={isRequired}> <FileUpload type="text" - id={field.name} + id={fieldId} + aria-label={ariaLabel} isRequired={isRequired}libs/i18n/locales/en/translation.json (1)
391-391: Fix typo in user-facing string."ordered listed" should be "listed order".
✏️ Proposed fix
- "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the ordered listed. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.", + "Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.": "Order of precedence: Files are applied in the listed order. If the same parameter is defined in multiple files, the value in the last file takes precedence and overrides previous values.",
🧹 Nitpick comments (4)
libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationInlineForm.tsx (1)
36-43: Avoid nestedFormGroupwrappers.
UploadFieldalready renders aFormGroup, so wrapping it creates nested groups and can disconnect the visible label from the control. Consider adding a prop to letUploadFieldrender without its internalFormGroup, or move the visible label intoUploadFielditself.libs/ui-components/src/components/Device/EditDeviceWizard/deviceSpecUtils.ts (2)
229-237: Consider graceful degradation instead of throwing.The YAML validation in
validations.tsalready ensures valid YAML before submission. However, iftoAPIApplicationis called with invalid YAML (e.g., during testing or from external code), throwing an error here could be unexpected. Consider whether logging a warning and returning an emptyvaluesobject might be more resilient, or document that this function expects pre-validated input.
485-488: Object comparison is order-sensitive and whitespace fallback is unusual.Two concerns:
yaml.load(updatedApp.valuesYaml || ' ')- the fallback to' '(two spaces) is unconventional. Both empty string and whitespace-only YAML parse toundefined. Consider using|| ''for clarity.
JSON.stringifycomparison is property-order sensitive. If the same values are serialized in different orders, this will incorrectly detect a change.♻️ Suggested improvement
- const updatedValues = yaml.load(updatedApp.valuesYaml || ' ') as Record<string, unknown>; - if (JSON.stringify(imageApp.values || {}) !== JSON.stringify(updatedValues)) { + const updatedValues = yaml.load(updatedApp.valuesYaml || '') as Record<string, unknown> | undefined; + const currentValues = imageApp.values || {}; + const newValues = updatedValues || {}; + // Note: JSON.stringify comparison is order-sensitive; consider deep-equal for robustness + if (JSON.stringify(currentValues) !== JSON.stringify(newValues)) {libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsx (1)
108-128: Tooltip shows misleading message when button is enabled.The tooltip with "Fill in the existing values files before you can add more values files" displays even when the button is enabled and clickable. This could confuse users.
Consider conditionally showing the tooltip only when the button is disabled, or changing the tooltip content to something always-relevant like guidance on values file usage.
♻️ Suggested approach
- <Tooltip - className="fctl-application-helm-form__tooltip" - content={ - <Content>{t('Fill in the existing values files before you can add more values files.')}</Content> - } - > + <Tooltip + className="fctl-application-helm-form__tooltip" + content={ + <Content>{t('Fill in the existing values files before you can add more values files.')}</Content> + } + trigger={canAddValuesFile ? 'manual' : 'mouseenter focus'} + >Alternatively, wrap the Tooltip conditionally:
{!canAddValuesFile ? ( <Tooltip content={...}> <Button .../> </Tooltip> ) : ( <Button .../> )}
| <FormGroup label={t('Application name')} isRequired> | ||
| <TextField aria-label={t('Application name')} name={`${appFieldName}.name`} isDisabled={isReadOnly} /> | ||
| </FormGroup> |
There was a problem hiding this comment.
Application name field marked as required but validation allows empty.
The isRequired prop is set on the FormGroup, but the Helm validation schema in validations.ts line 717 uses validApplicationAndVolumeName(t) without .required(). This creates a visual inconsistency - the field shows as required but accepts empty values.
Either remove isRequired from the FormGroup or add .required() to the validation schema if the name should be mandatory for Helm apps.
🐛 Option 1: Remove isRequired from UI
- <FormGroup label={t('Application name')} isRequired>
+ <FormGroup label={t('Application name')}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <FormGroup label={t('Application name')} isRequired> | |
| <TextField aria-label={t('Application name')} name={`${appFieldName}.name`} isDisabled={isReadOnly} /> | |
| </FormGroup> | |
| <FormGroup label={t('Application name')}> | |
| <TextField aria-label={t('Application name')} name={`${appFieldName}.name`} isDisabled={isReadOnly} /> | |
| </FormGroup> |
🤖 Prompt for AI Agents
In
`@libs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationHelmForm.tsx`
around lines 29 - 31, The FormGroup for the application name is marked
isRequired in EditDeviceWizard (FormGroup with label t('Application name')
containing TextField name `${appFieldName}.name`) but the Helm validation schema
(validApplicationAndVolumeName(t) used in validations.ts around the Helm app
schema) does not call .required(), causing a mismatch; fix by either removing
the isRequired prop from that FormGroup/TextField if the name is optional, or
update the Helm validation to require the field by chaining .required() onto
validApplicationAndVolumeName(t) in the Helm schema so the UI and validation
agree.
1 participant
New application type: Helm charts.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.