Repository navigation
fix: do not panic when overriding attributes with non-identifier keys - #1798
NopeCharon wants to merge 1 commit into
Conversation
Overriding a parent or plugin devfile element whose attributes contain an
object or list value under a key that is not a valid Go identifier, such
as the commonly used `pod-overrides` attribute, panicked with:
panic: reflect.StructOf: field 0 has invalid name
During the strategic merge, mapEnabledPatchMetaFromStruct describes a map
type to the strategicpatch library by building a struct type with a
single field, named after the map key with strings.Title. Map keys like
`pod-overrides` or `app.kubernetes.io/part-of` are not valid struct field
names, so reflect.StructOf panics as soon as the merge descends into such
an attribute value.
The synthesized struct only ever has one field, so the field name and its
JSON tag do not need to carry the map key. Use a fixed `Value` field with
the `value` JSON name, and look up the patch metadata with that fixed key
whenever the type being described is a map. The resulting patch strategy
and value types are unchanged, so attribute values are still replaced as
a whole as before.
Add an overriding test fixture covering object and list attributes with
non-identifier keys at the top level and in a component, including the
scenario from the issue.
Fixes devfile#1268
Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: NopeCharon 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 |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Map lookup key and fixtures pkg/utils/overriding/overriding.go, pkg/utils/overriding/test-fixtures/patches/attributes-with-non-identifier-keys/* |
Map lookups use the fixed key "value"; non-map lookups retain the original key. Struct and slice metadata lookups pass the returned key to the delegate. The fixtures define original, patch, and expected YAML data for non-identifier attribute keys, list values, and a runtime pod override. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Merge Risk: 
e8906
-
The change addresses non-identifier map attributes and adds coverage for object, list, and pod-override values without an identified merge-blocking risk.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "fix: do not panic when overriding attributes with non-identifier keys" matches the main change. The objectives say the PR prevents panics when overriding parent or plugin devfile elements w… Linked Issues check Issue #1268requires successful parsing when a child devfile overrides a parent attribute whose value is a list or object under a non-identifier key (for examplepod-overrides). The panic came from …Out of Scope Changes check The whole-PR diff against the merge base changes pkg/utils/overriding/overriding.goand adds four files undertest-fixtures/patches/attributes-with-non-identifier-keys/(original, patch, result YA…Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Full details: Title check
-
Explanation
The title "fix: do not panic when overriding attributes with non-identifier keys" matches the main change. The objectives say the PR prevents panics when overriding parent or plugin devfile elements whose attributes hold object or list values under keys such as pod-overrides and app.kubernetes.io/part-of. The raw summary confirms the change to map patch metadata lookup in overriding.go, which uses a fixed "value" JSON key instead of a name derived from the map key, plus new test fixtures for these attributes. The title is specific, short, and states the primary user-visible effect.
Full details: Linked Issues check
-
Explanation
Issue
#1268requires successful parsing when a child devfile overrides a parent attribute whose value is a list or object under a non-identifier key (for examplepod-overrides). The panic came fromreplaceMapWithSingleKeyStruct, which builtreflect.StructOfwith a field namedstrings.Title(key)and JSON tagkey. The head commit changes this: the synthesized struct now has a fixed fieldValuewith JSON tagvalue, andLookupPatchMetadataForStructandLookupPatchMetadataForSliceuse the returned lookup key (valuefor map types, the original key otherwise). The map key no longer reachesreflect.StructOf, so the reported panic path is removed. Because the map had one field and was always looked up by its own key, the lookup result for valid keys is unchanged. The new fixturetest-fixtures/patches/attributes-with-non-identifier-keysis picked up automatically byTestOverridingPatches(it walkstest-fixtures/patches). It covers a top-levelapp.kubernetes.io/part-ofobject, a top-levellist-attribute, and aruntimecomponent withpod-overrides.spec.imagePullSecrets, which matches the issue scenario. Limits: I did not execute the tests. The test pass result is author-reported, and the verdict rests on the source diff and fixture wiring.
Full details: Out of Scope Changes check
-
Explanation
The whole-PR diff against the merge base changes
pkg/utils/overriding/overriding.goand adds four files undertest-fixtures/patches/attributes-with-non-identifier-keys/(original, patch, result YAML). Theoverriding.gohunks all serve the panic fix: the newmapValueFieldKeyconstant, the updatedreplaceMapWithSingleKeyStructsignature and return value, the tag and field name change, and the two call sites. Removing the deprecatedstrings.Titlecall is a direct part of the same field-name change. TheAttributeshandling is unchanged. The fixtures test the fixed behavior. No unrelated files or behavior changes were found in the reviewed diff.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files. (3 skipped: 3 unsupported.)
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Description of Changes
Fixes a panic when a parent (or plugin) devfile element is overridden and its attributes contain an object or list value under a key that is not a valid Go identifier, such as the commonly used
pod-overridesattribute:During the strategic merge,
mapEnabledPatchMetaFromStructdescribes a map type to thestrategicpatchlibrary by building a struct type with a single field, named after the map key withstrings.Title. Keys likepod-overridesorapp.kubernetes.io/part-ofare not valid struct field names, soreflect.StructOfpanics once the merge goes into such an attribute value. Scalar attribute values never reach this lookup, which is why only object and list values triggered it. The nested list in the issue's example is not what causes it: the same patch passes if the key is renamed topodOverrides.The synthesized struct only ever has one field, so its field name and JSON tag don't need to carry the map key. This PR uses a fixed
Valuefield (JSON namevalue) and looks up the patch metadata with that fixed key whenever the type being described is a map. The patch strategy and value types are unchanged, so attribute values are still replaced as a whole, as before. This also removes the deprecatedstrings.Titlecall.Related Issue(s)
Fixes #1268
Acceptance Criteria
Testing and documentation do not need to be complete in order for this PR to be approved. However, tracking issues must be opened for missing testing/documentation.
New testing and documentation issues can be opened under
devfile/api/issues.You can check the respective criteria below if either of the following is true:
If criteria is left unchecked please provide an explanation why.
Unit/Functional tests
QE Integration test
Not needed: this is a bug fix in the overriding utility that is covered by the unit test fixtures.
Documentation
Not needed: there is no user-facing behavior change other than the panic being gone.
Client Impact
None: overrides that worked before produce the same result. Overrides that panicked before now succeed.
Tests Performed
attributes-with-non-identifier-keysoverriding fixture. It covers object and list attributes with non-identifier keys at the top level and in a component, including the exact scenario from the issue. It panics without the fix and passes with it.go test ./...at the repository root passes.go fmt ./...andgo vet ./pkg/utils/...are clean.How To Test
To reproduce the original panic, revert the change in
overriding.goand run the same command.Notes To Reviewer
Summary by CodeRabbit