fix(azure/managedredis): pass subscription scope not filter to recommendations pager (follow-up to #1455) - #1475
Conversation
…mmendations pager (follow-up to #1455) NewListPager's first argument is the billing scope; managedredis passed the ODATA filter string there, producing a malformed URL where every request errored -- Azure Managed Redis reservation recommendations were entirely broken on main. The filter belongs in ClientListOptions.Filter (the sibling cache/client.go has the correct shape). Also add the pagination cap + per-page ctx.Err() check every other Azure service client has. Extracted recommendationsListArgs() so a unit test can assert the subscription scope shape without a real Azure client (the injected mock pager bypasses NewListPager, which is why the wrong shape slipped through green tests).
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughManaged Redis recommendation retrieval now constructs subscription-scoped Azure pager arguments, passes the resource-type filter separately, checks cancellation during pagination, and enforces a maximum page count. A regression test verifies the pager argument wiring. ChangesManaged Redis recommendations
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
providers/azure/services/managedredis/client_test.go (2)
340-348: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regression coverage for cancellation and the page cap.
The changed
GetRecommendationspath now has two important guards, but this test only covers argument construction. Add tests for a canceled context and a pager with more thanmaxRecsPagespages.As per coding guidelines,
*_test.goshould prefer mock-first tests for new code.🤖 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 `@providers/azure/services/managedredis/client_test.go` around lines 340 - 348, Extend the managed Redis recommendations tests beyond recommendationsListArgs to cover GetRecommendations with a canceled context and a pager exposing more than maxRecsPages pages. Use mock-first setup for the pager/client dependencies, asserting cancellation returns promptly and the implementation stops after the configured page cap while preserving collected results up to that limit.Source: Coding guidelines
340-348: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert both required OData filter clauses.
This test only checks
resourceType; it would still pass ifproperties/scope eq 'Shared'were accidentally removed.Proposed assertion
assert.Contains(t, *opts.Filter, "resourceType eq 'RedisCache'") + assert.Contains(t, *opts.Filter, "properties/scope eq 'Shared'")🤖 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 `@providers/azure/services/managedredis/client_test.go` around lines 340 - 348, Update TestRecommendationsListArgs_UsesSubscriptionScope to assert that opts.Filter contains both required OData clauses: the existing resourceType condition and the properties/scope condition for 'Shared'.
🤖 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.
Nitpick comments:
In `@providers/azure/services/managedredis/client_test.go`:
- Around line 340-348: Extend the managed Redis recommendations tests beyond
recommendationsListArgs to cover GetRecommendations with a canceled context and
a pager exposing more than maxRecsPages pages. Use mock-first setup for the
pager/client dependencies, asserting cancellation returns promptly and the
implementation stops after the configured page cap while preserving collected
results up to that limit.
- Around line 340-348: Update TestRecommendationsListArgs_UsesSubscriptionScope
to assert that opts.Filter contains both required OData clauses: the existing
resourceType condition and the properties/scope condition for 'Shared'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0afb5ad5-f13f-4c76-98d1-9cc7d9fa5516
📒 Files selected for processing (2)
providers/azure/services/managedredis/client.goproviders/azure/services/managedredis/client_test.go
Follow-up to #1455 — HIGH (Fable adversarial sweep)
Azure Managed Redis reservation recommendations are entirely broken on main:
GetRecommendationscallsNewListPager(filter, ...), but the first argument is the billing scope, not the ODATA filter. The wrong shape produces a malformed URL where every request errors (the exact failure mode documented incompute/client.go). The siblingcache/client.gohas the correct shape.Fix
/subscriptions/<id>as the scope; move the filter intoClientListOptions.Filter.maxRecsPages) + per-pagectx.Err()check every other Azure service client has.recommendationsListArgs()so a unit test asserts the subscription-scope shape without a real Azure client (the injected mock pager bypassesNewListPager, which is why the bug slipped through green tests).Verification
go build ./...,go vet, managedredis suite green.TestRecommendationsListArgs_UsesSubscriptionScopefails if the filter is passed as scope again.Summary by CodeRabbit