Conversation
umswmayj
requested review from
PhilippMatthes,
SoWieMarkus,
auhlig,
juliusclausnitzer and
mblos
as code owners
📝 WalkthroughWalkthroughDeduplication added to pagination handling in Nova client functions: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
PhilippMatthes
approved these changes
Mar 31, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go (1)
174-181: Harden the mock handler against unexpected extra requests.Line 177 indexes
tt.responses[callCount]directly; if pagination regresses, this can panic and hide the root cause. Prefer explicit failure.Suggested test hardening
handler := func(w http.ResponseWriter, r *http.Request) { w.Header().Add("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - if _, err := w.Write([]byte(tt.responses[callCount])); err != nil { + if callCount >= len(tt.responses) { + t.Fatalf("unexpected extra request #%d to %s", callCount+1, r.URL.String()) + } + if _, err := w.Write([]byte(tt.responses[callCount])); err != nil { t.Fatalf("failed to write response: %v", err) } callCount++ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go` around lines 174 - 181, The mock HTTP handler in the test uses tt.responses[callCount] directly, which can panic if extra requests arrive; update the handler function (handler, callCount, tt.responses) to first check that callCount < len(tt.responses) and call t.Fatalf (with context like the unexpected request method/URL and current callCount) when an unexpected extra request occurs, otherwise write the response and then increment callCount—this makes the test fail explicitly instead of panicking.internal/knowledge/datasources/plugins/openstack/nova/nova_api.go (1)
126-133: Consider adding a duplicate-skipped metric (not just warn logs).Line 128 logs each duplicate, but
SyncAllServersconsumeslen(allServers)for gauges/db replacement; a dedicated counter for skipped duplicates would make monitoring changes explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/datasources/plugins/openstack/nova/nova_api.go` around lines 126 - 133, SyncAllServers currently logs duplicates via slog.Warn when iterating list.Servers but still only uses len(allServers) for downstream gauges/replacement; add a dedicated counter metric (e.g., duplicateServersSkipped) and increment it inside the duplicate branch (right after detecting seen[s.ID] and before continue) so skipped items are counted; ensure the metric is declared/registered alongside other metrics used by this package and update any metric names or labels to match existing conventions so monitoring dashboards can reflect duplicates separately from total servers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go`:
- Around line 174-181: The mock HTTP handler in the test uses
tt.responses[callCount] directly, which can panic if extra requests arrive;
update the handler function (handler, callCount, tt.responses) to first check
that callCount < len(tt.responses) and call t.Fatalf (with context like the
unexpected request method/URL and current callCount) when an unexpected extra
request occurs, otherwise write the response and then increment callCount—this
makes the test fail explicitly instead of panicking.
In `@internal/knowledge/datasources/plugins/openstack/nova/nova_api.go`:
- Around line 126-133: SyncAllServers currently logs duplicates via slog.Warn
when iterating list.Servers but still only uses len(allServers) for downstream
gauges/replacement; add a dedicated counter metric (e.g.,
duplicateServersSkipped) and increment it inside the duplicate branch (right
after detecting seen[s.ID] and before continue) so skipped items are counted;
ensure the metric is declared/registered alongside other metrics used by this
package and update any metric names or labels to match existing conventions so
monitoring dashboards can reflect duplicates separately from total servers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57c93565-8eb7-4db2-91ac-61838bbd02cf
📒 Files selected for processing (2)
internal/knowledge/datasources/plugins/openstack/nova/nova_api.gointernal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go
Contributor
Test Coverage ReportTest Coverage 📊: 68.6% |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go (1)
186-193: Potential data race oncallCountvariable.The
callCountvariable is read and incremented by the HTTP handler running in a separate goroutine without synchronization. While this works in practice due to the sequential nature of the HTTP requests, the race detector would flag this.Consider using an atomic counter or a mutex to eliminate the data race:
♻️ Suggested fix using atomic
+import "sync/atomic" + func TestNovaAPI_GetAllServers_DeduplicatesServers(t *testing.T) { // ... test cases ... for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - callCount := 0 + var callCount atomic.Int32 handler := func(w http.ResponseWriter, r *http.Request) { w.Header().Add("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - if _, err := w.Write([]byte(tt.responses[callCount])); err != nil { + idx := callCount.Load() + if _, err := w.Write([]byte(tt.responses[idx])); err != nil { t.Fatalf("failed to write response: %v", err) } - callCount++ + callCount.Add(1) }The same pattern applies to the other deduplication tests (
TestNovaAPI_GetDeletedServers_DeduplicatesServers,TestNovaAPI_GetAllHypervisors_DeduplicatesHypervisors,TestNovaAPI_GetAllMigrations_DeduplicatesMigrations).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go` around lines 186 - 193, The test handler uses an unsynchronized integer callCount that the HTTP server goroutine reads and increments, causing a data race; replace callCount with a concurrency-safe counter (e.g., use sync/atomic's uint32/uint64 and atomic.LoadUint32/atomic.AddUint32 or protect access with a sync.Mutex) in the handler closure so reads and increments are atomic; update the handler in this test (referencing callCount and handler) and apply the same change to the other deduplication tests (TestNovaAPI_GetDeletedServers_DeduplicatesServers, TestNovaAPI_GetAllHypervisors_DeduplicatesHypervisors, TestNovaAPI_GetAllMigrations_DeduplicatesMigrations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go`:
- Around line 186-193: The test handler uses an unsynchronized integer callCount
that the HTTP server goroutine reads and increments, causing a data race;
replace callCount with a concurrency-safe counter (e.g., use sync/atomic's
uint32/uint64 and atomic.LoadUint32/atomic.AddUint32 or protect access with a
sync.Mutex) in the handler closure so reads and increments are atomic; update
the handler in this test (referencing callCount and handler) and apply the same
change to the other deduplication tests
(TestNovaAPI_GetDeletedServers_DeduplicatesServers,
TestNovaAPI_GetAllHypervisors_DeduplicatesHypervisors,
TestNovaAPI_GetAllMigrations_DeduplicatesMigrations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1abc2f0b-eed1-406f-9b7c-90de4e89b810
📒 Files selected for processing (2)
internal/knowledge/datasources/plugins/openstack/nova/nova_api.gointernal/knowledge/datasources/plugins/openstack/nova/nova_api_test.go
SoWieMarkus
approved these changes
Mar 31, 2026
umswmayj
changed the title
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
3 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.
No description provided.