fix(nvca): merge labels/annos in miniservice metadata cm - #609
Conversation
Signed-off-by: Eric Stroczynski <estroczynski@nvidia.com>
📝 WalkthroughWalkthroughThe miniservice metadata builder now preserves existing labels and annotations. It initializes nil maps and merges generated metadata instead of replacing existing entries. ChangesMetadata preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/compute-plane-services/nvca/internal/miniservice/metadata_configmap.go`:
- Around line 92-105: Extend the regression coverage for the metadata merge
logic around newGeneralObjectLabelsAndAnnotations and the metadata_configmap
round-trip test: verify unrelated labels and annotations are preserved, nil
input maps are handled, and duplicate keys retain the generated values currently
established by maps.Copy. Add focused cases for both labels and annotations
without changing the merge implementation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 48676d95-1444-4524-a63b-604078523936
📒 Files selected for processing (1)
src/compute-plane-services/nvca/internal/miniservice/metadata_configmap.go
| labels, annotations := newGeneralObjectLabelsAndAnnotations( | ||
| r.FeatureFlagFetcher, ms, icmsReq, | ||
| r.ClusterRegion, r.ClusterName, in.FunctionName, in.TaskName, false, | ||
| ) | ||
| if meta.Labels == nil { | ||
| meta.Labels = labels | ||
| } else { | ||
| maps.Copy(meta.Labels, labels) | ||
| } | ||
| if meta.Annotations == nil { | ||
| meta.Annotations = annotations | ||
| } else { | ||
| maps.Copy(meta.Annotations, annotations) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add regression coverage for the merge contract.
The supplied metadata_configmap_test.go round-trip test does not verify that unrelated labels and annotations survive the merge. It also does not define duplicate-key precedence. Add cases for preserved entries, nil input maps, and the intended collision behavior. The current code gives generated values precedence for duplicate keys. (pkg.go.dev)
If tests are intentionally omitted, explain the omission in the Pull Request description.
As per coding guidelines, code changes must include tests; if tests are omitted, explain why in the Pull Request description.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/compute-plane-services/nvca/internal/miniservice/metadata_configmap.go`
around lines 92 - 105, Extend the regression coverage for the metadata merge
logic around newGeneralObjectLabelsAndAnnotations and the metadata_configmap
round-trip test: verify unrelated labels and annotations are preserved, nil
input maps are handled, and duplicate keys retain the generated values currently
established by maps.Copy. Add focused cases for both labels and annotations
without changing the merge implementation.
Source: Coding guidelines
1 participant
TL;DR
Fix labels/annos merging in miniservice metadata ConfigMap
Additional Details (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
For the Reviewer
For QA (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
Issues
Checklist
Summary by CodeRabbit