Open
Conversation
📝 WalkthroughWalkthroughMetadata support is added to the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Merging this branch will not change overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Member
Author
|
I validated this is not a breaking change. minikube startApply the crd before this change without the field: git checkout 0c0978 && kubectl apply -f charts/openstack-hypervisor-operator/crds/Create example hypervisor resource: cat <<EOF | kubectl apply -f -
apiVersion: kvm.cloud.sap/v1
kind: Hypervisor
metadata:
name: example-hypervisor
EOFAdd some aggregate into the status: kubectl patch hypervisor example-hypervisor --type=merge --subresource status --patch 'status: {aggregates: [{"name": "example-name", "uuid": "example-uuid"}]}'Now apply the new crd: git checkout add-aggregate-metadata && kubectl apply -f charts/openstack-hypervisor-operator/crds/That succeeds and keeps the status: kubectl get hypervisor example-hypervisor -o yaml#...
status:
aggregates:
- name: example-name
uuid: example-uuidAnd I can populate the metadata field: kubectl patch hypervisor example-hypervisor --type=merge --subresource status --patch 'status: {aggregates: [{"name": "example-name", "uuid": "example-uuid", "metadata": {"a": "b"}}]}'# ...
status:
aggregates:
- metadata:
a: b
name: example-name
uuid: example-uuid |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
applyconfigurations/api/v1/aggregate.go (1)
35-47: Minor typo in comment.Line 36: "build" should be "built" for grammatical correctness.
📝 Suggested fix
// WithMetadata puts the entries into the Metadata field in the declarative configuration -// and returns the receiver, so that objects can be build by chaining "With" function invocations. +// and returns the receiver, so that objects can be built by chaining "With" function invocations. // If called multiple times, the entries provided by each call will be put on the Metadata field, // overwriting an existing map entries in Metadata field with the same key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applyconfigurations/api/v1/aggregate.go` around lines 35 - 47, The comment on the WithMetadata method has a grammatical typo: change "objects can be build by chaining" to "objects can be built by chaining" in the doc comment for AggregateApplyConfiguration.WithMetadata so the documentation is correct and reads "built" instead of "build".internal/openstack/aggregates_test.go (1)
122-134: Consider using a map-based assertion for metadata to avoid order dependency.The current test collects metadata into a slice by index, then asserts on specific indices. While this works because
ApplyAggregatespreserves the order ofdesiredAggregates, the earlier assertions fornamesanduuidsuseConsistOfwhich is order-independent.For consistency and robustness, consider asserting metadata by aggregate name:
💡 Suggested improvement
- metadata := make([]map[string]string, len(aggregates)) for i, agg := range aggregates { names[i] = agg.Name uuids[i] = agg.UUID - metadata[i] = agg.Metadata } Expect(names).To(ConsistOf("agg1", "agg2", "agg3")) Expect(uuids).To(ConsistOf("uuid-agg1", "uuid-agg2", "uuid-agg3")) - Expect(metadata[0]).To(Equal(map[string]string{"key1": "value1", "key2": "value2"})) // agg1 metadata should be preserved - Expect(metadata[1]).To(BeNil()) // agg2 has no metadata field, should be nil - Expect(metadata[2]).To(BeEmpty()) // agg3 has empty metadata, should be preserved + // Assert metadata by name to avoid order dependency + metadataByName := make(map[string]map[string]string) + for _, agg := range aggregates { + metadataByName[agg.Name] = agg.Metadata + } + Expect(metadataByName["agg1"]).To(Equal(map[string]string{"key1": "value1", "key2": "value2"})) + Expect(metadataByName["agg2"]).To(BeNil()) + Expect(metadataByName["agg3"]).To(BeEmpty())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/openstack/aggregates_test.go` around lines 122 - 134, The test currently indexes metadata by position which assumes order; instead, map the returned aggregates by their Name (e.g., build a map[string]map[string]string from the aggregates slice produced by ApplyAggregates) and assert each aggregate's metadata by looking up metadataByName["agg1"], metadataByName["agg2"], metadataByName["agg3"]; update the Expect assertions to reference the map lookups (preserving the same expectations: agg1 equals the map, agg2 is nil, agg3 is empty) so the test no longer depends on slice ordering while keeping the existing names/uuids ConsistOf checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@applyconfigurations/api/v1/aggregate.go`:
- Around line 35-47: The comment on the WithMetadata method has a grammatical
typo: change "objects can be build by chaining" to "objects can be built by
chaining" in the doc comment for AggregateApplyConfiguration.WithMetadata so the
documentation is correct and reads "built" instead of "build".
In `@internal/openstack/aggregates_test.go`:
- Around line 122-134: The test currently indexes metadata by position which
assumes order; instead, map the returned aggregates by their Name (e.g., build a
map[string]map[string]string from the aggregates slice produced by
ApplyAggregates) and assert each aggregate's metadata by looking up
metadataByName["agg1"], metadataByName["agg2"], metadataByName["agg3"]; update
the Expect assertions to reference the map lookups (preserving the same
expectations: agg1 equals the map, agg2 is nil, agg3 is empty) so the test no
longer depends on slice ordering while keeping the existing names/uuids
ConsistOf checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2d38d88-1ddd-405f-b164-2e60aa78987a
📒 Files selected for processing (6)
api/v1/hypervisor_types.goapi/v1/zz_generated.deepcopy.goapplyconfigurations/api/v1/aggregate.gocharts/openstack-hypervisor-operator/crds/kvm.cloud.sap_hypervisors.yamlinternal/openstack/aggregates.gointernal/openstack/aggregates_test.go
notandy
approved these changes
Mar 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change adds aggregate metadata to the type kvmv1.Aggregate struct, and passes it in the aggregates controller.
Summary by CodeRabbit
New Features
Tests