fix(application-create): prevent crash when switching from YAML to form with valuesObject #22368
Conversation
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
8cb660d to
6971b5a
Compare
| function mergeAndFormatHelmValues(valuesObject: any, values: string) { | ||
| if (!valuesObject && !values) { | ||
| return ''; | ||
| } | ||
| //Order of precedence: valuesObject > values | ||
| const mergedValueObject = merge(jsYaml.load(values || ''), valuesObject || {}); | ||
| return jsYaml.dump(mergedValueObject); | ||
| } | ||
|
|
||
| function flattenObject(obj: any, parentPath = '', result = {} as any) { | ||
| forEach(obj, (value, key) => { | ||
| const fullPath = parentPath ? (isArray(obj) ? `${parentPath}[${key}]` : `${parentPath}.${key}`) : key; | ||
| if (isObject(value) || isArray(value)) { | ||
| flattenObject(value, fullPath, result); | ||
| } else { | ||
| result[fullPath] = value; | ||
| } | ||
| }); | ||
| return result; | ||
| } | ||
|
|
||
| function unflattenedObject(obj: any) { | ||
| const result = {}; | ||
| for (const key in obj) { | ||
| if (Object.prototype.hasOwnProperty.call(obj, key)) { | ||
| set(result, key, obj[key]); | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Could we have some unit tests for these utility functions? I think we're trying to mimic Helm's values file merging behavior. If we find later that our merge algorithms differ from Helm's, it would be good to have unit tests to help safely change our algorithm to match Helm's.
There was a problem hiding this comment.
I completely agree！
Give me some time to review the original Helm's values file merging behavior and add test cases for these functions.
There was a problem hiding this comment.
Hello! @crenshaw-dev @nitishfy
I've reviewed the backend implementation of Helm values handling, particularly focusing on SetValuesString and ValuesYAML methods in https://github.com/argoproj/argo-cd/blob/master/pkg/apis/application/v1alpha1/values.go#L15
In my view, the current backend implementation follows a clear pattern:
ValuesObjecttakes precedence overValues(as documented in CRD)- When setting values through
SetValuesString:- Converts YAML to JSON and stores in
ValuesObject - Clears the
Valuesfield to avoid conflicts
- Converts YAML to JSON and stores in
- When retrieving values through
ValuesYAML:- Returns
ValuesObjectif present (converted back to YAML) - Falls back to
ValuesifValuesObjectis nil
- Returns
However, our current UI form doesn't reflect this implementation, which creates two UX issues:
- Values edited in YAML view may appear differently in form view due to the internal conversion
- Users have no visibility into which field (
valuesorvaluesObject) is actually being used
I propose two potential solutions:
-
Enhanced UI with Source Visibility (Recommended):
- Add visual indication of the active values source
- Provide explicit control over which field to modify
- Example mockup:
-
Simplified Single-Field Approach:
- Always use
valuesfield for both display and editing in UI form - Remove the dual-field complexity from UI
- Always use
I recommend the first approach because:
- It provides transparency into the backend behavior
- Maintains consistency with the CRD design
- Gives users explicit control over their configuration
Hope to receive your thoughts on this discussion.
There was a problem hiding this comment.
I have updated the description of this PR.
This PR now only fix the issue of preventing a crash when switching from YAML to form mode with valuesObject during application creation.
It does not include any changes to value or parameters, as I believe the UI should only be responsible for displaying configuration, not handling the logic of data merging.
Signed-off-by: ruichao.hu <ruichao.hu@daocloud.io>
Signed-off-by: ruichao.hu <ruichao.hu@daocloud.io>
6971b5a to
99bcaf6
Compare
Fix #22367
What this PR does
Fixes a crash that occurs when switching from YAML to form mode during application creation, in cases where
valuesObjectis defined in the YAML.How it was fixed
Added handling for the
valuesObjectfield in the form mode to prevent the crash.Why this change is necessary
The
valuesObjectfield structure is consistent with the configuration options. It is only displayed on the frontend and is not meant to be overridden, which is now properly handled.Checklist: