OCPBUGS-83767: Helm upgrade — preserve release values when changing chart version#16364
OCPBUGS-83767: Helm upgrade — preserve release values when changing chart version#16364vikram-raj wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@vikram-raj: This pull request references Jira Issue OCPBUGS-83767, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
component/helm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vikram-raj The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@vikram-raj: This pull request references Jira Issue OCPBUGS-83767, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@vikram-raj: This pull request references Jira Issue OCPBUGS-83767, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request refactors Helm chart version upgrade behavior to merge current release values with new chart defaults rather than replacing them entirely. The change spans four files: updated English localization messaging, modified dropdown component logic that invokes new merge utilities, three new exported helper functions handling value extraction and merge semantics with array replacement behavior, and comprehensive test coverage validating merge logic across Form and YAML editor modes. User-entered configuration now takes precedence over new chart defaults, while arrays replace entirely rather than merge by index. 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
frontend/packages/helm-plugin/src/utils/__tests__/helm-utils.spec.ts (1)
382-396: Consider adding Form mode end-to-end test.The current end-to-end test covers YAML mode. A complementary test in Form mode would strengthen coverage of the editor type branching:
🧪 Optional: Add Form mode test
it('mergeHelmValuesOnChartVersionChange should merge chart defaults with formData in Form mode', () => { const newChartValues = { image: { tag: '2.0.0' }, replicas: 1 }; const yamlData = 'replicas: 99\n'; // Should be ignored in Form mode const formData = { replicas: 3, customSetting: true }; const merged = mergeHelmValuesOnChartVersionChange( newChartValues, yamlData, formData, EditorType.Form, ); expect(merged).toEqual({ image: { tag: '2.0.0' }, replicas: 3, customSetting: true, }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/helm-plugin/src/utils/__tests__/helm-utils.spec.ts` around lines 382 - 396, Add a new unit test mirroring the existing YAML-mode spec but exercising the EditorType.Form branch: call mergeHelmValuesOnChartVersionChange with newChartValues, a yamlData string that should be ignored, and a formData object (e.g., { replicas: 3, customSetting: true }) and pass EditorType.Form; assert the returned merged object preserves formData values (replicas: 3), merges chart defaults where formData lacks keys (image.tag: '2.0.0'), and includes other form fields (customSetting: true). Reference mergeHelmValuesOnChartVersionChange and EditorType.Form to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmChartVersionDropdown.tsx`:
- Around line 202-210: The catch block that handles safeDump failures (around
setFieldValue calls in HelmChartVersionDropdown) doesn't update the form's
'yamlData', which can leave the YAML view showing stale content; update
'yamlData' in the same error fallback — e.g. setFieldValue('yamlData',
JSON.stringify(mergedValues, null, 2)) or set it to an empty string/previous
value with a warning — so that switching to YAML reflects the mergedValues; keep
the other fallbacks (editorType, formSchema, formData, chartReadme,
setInitialFormData) as-is.
---
Nitpick comments:
In `@frontend/packages/helm-plugin/src/utils/__tests__/helm-utils.spec.ts`:
- Around line 382-396: Add a new unit test mirroring the existing YAML-mode spec
but exercising the EditorType.Form branch: call
mergeHelmValuesOnChartVersionChange with newChartValues, a yamlData string that
should be ignored, and a formData object (e.g., { replicas: 3, customSetting:
true }) and pass EditorType.Form; assert the returned merged object preserves
formData values (replicas: 3), merges chart defaults where formData lacks keys
(image.tag: '2.0.0'), and includes other form fields (customSetting: true).
Reference mergeHelmValuesOnChartVersionChange and EditorType.Form to locate
where to add the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 02797b10-2a2d-4cfd-a82f-7084e19d7ef1
📒 Files selected for processing (4)
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonfrontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmChartVersionDropdown.tsxfrontend/packages/helm-plugin/src/utils/__tests__/helm-utils.spec.tsfrontend/packages/helm-plugin/src/utils/helm-utils.ts
📜 Review details
🔇 Additional comments (9)
frontend/packages/helm-plugin/locales/en/helm-plugin.json (1)
104-104: LGTM!The updated copy accurately describes the new merge-and-review behavior. Clear guidance for users to review values before upgrading.
frontend/packages/helm-plugin/src/utils/helm-utils.ts (4)
1-11: LGTM on imports!Good choice using
safeLoadfor YAML parsing - avoids arbitrary code execution risks thatloadwould introduce with untrusted input.
376-387: LGTM!Solid merge logic. Using
mergeWithwith an empty target object prevents input mutation, and the array replacement semantics align with Helm's typical usage patterns (e.g.,tolerations,envarrays should be replaced wholesale rather than merged by index).
393-420: LGTM!The fallback chain is sensible: Form mode prefers
formData, YAML mode prefers parsed YAML withformDatafallback on parse failure. ThesafeLoadtry/catch guards against malformed YAML, and the object type checks prevent unexpected return types.
422-431: LGTM!Clean orchestration function. The
?? {}fallback fornewChartValueshandles charts without default values gracefully.frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmChartVersionDropdown.tsx (3)
7-7: LGTM!Both
safeDump(line 194) andsafeLoad(line 129) are used in this component.
55-59: LGTM!
editorTypeis correctly extracted from Formik context. PerHelmInstallUpgradePage.tsx, it's always initialized to eitherEditorType.FormorEditorType.YAML.
88-94: LGTM!Modal text accurately describes the merge-and-review workflow. Proper JSX escaping of the apostrophe.
frontend/packages/helm-plugin/src/utils/__tests__/helm-utils.spec.ts (1)
314-397: Good test coverage!Solid coverage of the key merge scenarios: user overrides, new chart keys, array replacement, editor mode preferences, and invalid YAML fallback. The tests align well with the documented behavior.
| } catch (err) { | ||
| console.error('Failed to serialize merged values:', err); // eslint-disable-line no-console | ||
| // Fall back to using the merged values object without YAML serialization | ||
| setFieldValue('editorType', nextEditorType); | ||
| setFieldValue('formSchema', valuesSchema); | ||
| setFieldValue('formData', mergedValues); | ||
| setFieldValue('chartReadme', chartReadme); | ||
| setInitialFormData(mergedValues); | ||
| } |
There was a problem hiding this comment.
Missing yamlData update in error fallback may cause stale YAML view.
When safeDump fails, yamlData isn't updated while formData gets the merged values. If the user later switches to YAML view, they'll see stale YAML that doesn't reflect the merge. Consider setting yamlData to an empty string or the previous value with a warning, or attempting a best-effort serialization:
🛡️ Proposed fix to handle yamlData in error path
} catch (err) {
console.error('Failed to serialize merged values:', err); // eslint-disable-line no-console
// Fall back to using the merged values object without YAML serialization
setFieldValue('editorType', nextEditorType);
setFieldValue('formSchema', valuesSchema);
+ // Keep yamlData in sync as best we can - use empty or preserve previous
+ setFieldValue('yamlData', '');
+ setInitialYamlData('');
setFieldValue('formData', mergedValues);
setFieldValue('chartReadme', chartReadme);
setInitialFormData(mergedValues);
}Alternatively, if safeDump fails on complex objects, you could try JSON.stringify(mergedValues, null, 2) as a fallback representation in YAML view.
📝 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.
| } catch (err) { | |
| console.error('Failed to serialize merged values:', err); // eslint-disable-line no-console | |
| // Fall back to using the merged values object without YAML serialization | |
| setFieldValue('editorType', nextEditorType); | |
| setFieldValue('formSchema', valuesSchema); | |
| setFieldValue('formData', mergedValues); | |
| setFieldValue('chartReadme', chartReadme); | |
| setInitialFormData(mergedValues); | |
| } | |
| } catch (err) { | |
| console.error('Failed to serialize merged values:', err); // eslint-disable-line no-console | |
| // Fall back to using the merged values object without YAML serialization | |
| setFieldValue('editorType', nextEditorType); | |
| setFieldValue('formSchema', valuesSchema); | |
| // Keep yamlData in sync as best we can - use empty or preserve previous | |
| setFieldValue('yamlData', ''); | |
| setInitialYamlData(''); | |
| setFieldValue('formData', mergedValues); | |
| setFieldValue('chartReadme', chartReadme); | |
| setInitialFormData(mergedValues); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmChartVersionDropdown.tsx`
around lines 202 - 210, The catch block that handles safeDump failures (around
setFieldValue calls in HelmChartVersionDropdown) doesn't update the form's
'yamlData', which can leave the YAML view showing stale content; update
'yamlData' in the same error fallback — e.g. setFieldValue('yamlData',
JSON.stringify(mergedValues, null, 2)) or set it to an empty string/previous
value with a warning — so that switching to YAML reflects the mergedValues; keep
the other fallbacks (editorType, formSchema, formData, chartReadme,
setInitialFormData) as-is.
|
@vikram-raj: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Jira Bug fixes: https://redhat.atlassian.net/browse/OCPBUGS-83767
Analysis / Root cause:
On upgrade, changing the chart version replaced the editor with chart defaults only. The async upgrade API then received a non-empty values object, so Helm did not take the “empty new vals → reuse last release config” path and prior release settings were effectively dropped—unlike helm upgrade merge expectations and bad for Software Catalog charts (e.g. FlightControl/RHEM).
Solution description:
What changed
helm-utils.ts: Added mergeHelmChartVersionUpgradeValues, getCurrentHelmUserValuesForUpgradeMerge, and mergeHelmValuesOnChartVersionChange (chart defaults merged under current form/YAML values so user/release wins).HelmChartVersionDropdown.tsx: Uses mergeHelmValuesOnChartVersionChange after fetching the new chart version.Copy: version-change modal text updated to describe merge + review.
helm-utils.spec.ts: Unit tests for merge behavior, editor precedence, invalid YA ML fallback, and an end-to-end merge scenario.yarn i18n for new/changed strings (if included in the same PR).
Screenshots / screen recording:
Before:
Helm-upgrade-before.mov
After:
Helm.upgrade.After.mov
Test setup:
Browser conformance:
Summary by CodeRabbit
Release Notes
Bug Fixes
Documentation