fix(config): support nested configurations in config apply#1024
fix(config): support nested configurations in config apply#1024shellyco-code wants to merge 2 commits into
Conversation
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
|
@qcserestipy Hi! This PR implements the fix for the nested configuration parsing issue discussed in #1024. Whenever you have some time, I'd really appreciate your review. Thank you! |
qcserestipy
requested review from
Copilot and
qcserestipy
and removed request for
Copilot
qcserestipy
added
the
status/in-progress
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1024 +/- ##
=========================================
- Coverage 10.99% 9.46% -1.53%
=========================================
Files 173 321 +148
Lines 8671 16090 +7419
=========================================
+ Hits 953 1523 +570
- Misses 7612 14432 +6820
- Partials 106 135 +29 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes harbor config apply to correctly read configuration files that wrap settings under a top-level configurations: (YAML) / "configurations" (JSON) key, preventing silent no-op applies for the documented file format (Fixes #1023).
Changes:
- Add a
configWrapperhelper to support nestedconfigurationspayloads. - Update YAML/JSON unmarshaling logic in
config applyto prefer wrapped parsing with fallback behavior. - Add basic command metadata/argument-validation tests for
config apply.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/harbor/root/configurations/apply.go | Adds wrapper-aware YAML/JSON parsing for config apply to handle nested configurations documents. |
| cmd/harbor/root/configurations/apply_test.go | Adds baseline tests for command metadata and flag/arg validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Previously, the YAML and JSON parsing logic used a wrapper-first approach that silently fell back to flat parsing whenever the wrapper unmarshal produced a nil Configurations field. This reintroduced the original silent no-op bug from goharbor#1023: a file with a 'configurations' key pointing to an invalid type (string, list, null) would produce an empty configurations object and report 'No changes detected.' instead of an error. Fix the logic by detecting the presence of the 'configurations' key using a raw map before attempting typed unmarshaling. If the key exists, the wrapper parse is mandatory and any failure (wrong type, null, empty) is treated as a fatal error. If the key is absent the file is parsed as a legacy flat document for backward compatibility. Extract the parsing logic into dedicated parseYAMLConfig and parseJSONConfig helpers to make the intent clear and to allow direct unit testing without a live Harbor server. Add regression tests that cover: - valid wrapped YAML and JSON - malformed wrapped YAML/JSON (scalar, list, null under configurations) - legacy flat YAML and JSON - empty configurations key - invalid syntax Fixes goharbor#1023 Signed-off-by: shellyco-code <shellychahar57@gmail.com>
|
Hey @qcserestipy , addressed all three Copilot comments — fixed the silent fallback bug for malformed wrapped files and added parsing regression tests. Would appreciate another review when you get a chance! |
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if tt.checkResult != nil { | ||
| tt.checkResult(t, got) | ||
| } |
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if tt.checkResult != nil { | ||
| tt.checkResult(t, got) | ||
| } |
qcserestipy
left a comment
qcserestipy
left a comment
There was a problem hiding this comment.
@shellyco-code Thank you for your contribution, just a few small changes requested. Please also address the copilot points with respect to the tests.
| // configuration document for backward compatibility. | ||
| func parseYAMLConfig(data []byte) (*models.Configurations, error) { | ||
| // Use a raw map to detect whether the top-level "configurations" key exists. | ||
| var rawMap map[string]interface{} |
There was a problem hiding this comment.
interface{} can be replaced by anyanydefault
| Configurations *models.Configurations `yaml:"configurations" json:"configurations"` | ||
| } | ||
|
|
||
| // parseYAMLConfig parses a YAML configuration file into *models.Configurations. |
There was a problem hiding this comment.
in principle looks good, can you shorten this doc string
qcserestipy
added
bug
Description
This pull request resolves an issue where the
harbor config applycommand fails silently when applying configuration files that nest configuration parameters under a top-levelconfigurations:key.The unmarshaling block has been updated to attempt to parse the wrapped structure first, falling back to flat parsing if the wrapper is absent, ensuring full backward compatibility.
Type of Change
Please select the relevant type.
Changes
configWrapperhelper struct inapply.goto match the nested configurations block.cmd/harbor/root/configurations/apply.goto attempt wrapper unmarshaling first and fall back to direct unmarshaling on failure or nil.cmd/harbor/root/configurations/apply_test.goimplementing metadata validation and CLI-level error checks usingtestutil.TestCmd.