Skip to content

feat(providers/azure/managed-redis): add Azure Managed Redis service client (closes #556) - #620

Merged
cristim merged 6 commits into
feat/multicloud-web-frontendfrom
feat/issue-556-azure-managed-redis
May 22, 2026
Merged

feat(providers/azure/managed-redis): add Azure Managed Redis service client (closes #556)#620
cristim merged 6 commits into
feat/multicloud-web-frontendfrom
feat/issue-556-azure-managed-redis

Conversation

@cristim

@cristim cristim commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds providers/azure/services/managedredis as Azure's counterpart to AWS MemoryDB for Redis (AWS → Azure Feature Parity Gaps #473 parity gap)
  • Azure Cache for Redis is used as the implementation: it is the closest Azure equivalent to AWS MemoryDB (fully managed, in-memory, reservation-based pricing)
  • The new ManagedRedisClient registers under ServiceMemoryDB so it is routed and reported symmetrically with the AWS MemoryDB client at the provider-dispatch level
  • Reuses existing armredis/v3 and armconsumption SDK packages - no new dependencies

Changes

  • providers/azure/services/managedredis/client.go: new ManagedRedisClient implementing the full provider.ServiceClient interface (GetRecommendations, GetExistingCommitments, PurchaseCommitment, ValidateOffering, GetOfferingDetails, GetValidResourceTypes)
  • providers/azure/services/managedredis/client_test.go: 34 tests covering all methods, pager injection, HTTP mock, token error paths, SKU fallback
  • providers/azure/services.go: NewManagedRedisClient factory function
  • providers/azure/provider.go: ServiceMemoryDB added to GetSupportedServices() and newServiceClientForSubscription() switch
  • providers/azure/services_test.go: TestNewManagedRedisClient
  • providers/azure/provider_test.go: ServiceMemoryDB assertions in GetSupportedServices and AllServiceTypes tests

Test plan

  • go test ./providers/azure/services/managedredis/... -count=1 -short: 34 passed
  • go test ./... -count=1 -short (azure module): 426 passed across 11 packages
  • go build ./...: clean
  • go mod tidy: clean
  • Pre-commit hooks (gofmt, go vet, gocyclo, security): all passed

Closes #556
Refs #473

Summary by CodeRabbit

  • New Features

    • Added Azure Cache for Redis (MemoryDB) as a supported service and a managed Redis client for recommendations, SKU discovery, pricing/offers, existing reservations, and reservation purchases with cost/savings estimates.
  • Tests

    • Added extensive tests covering client behavior, SKU discovery/fallbacks, pricing parsing, recommendations pagination, reservation listing, purchase flows, validation, and edge cases.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@cristim has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 30 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b4a98834-c877-4abd-927a-aa660cbfa325

📥 Commits

Reviewing files that changed from the base of the PR and between 31c88ae and fab11b6.

📒 Files selected for processing (6)
  • providers/azure/provider.go
  • providers/azure/provider_test.go
  • providers/azure/services.go
  • providers/azure/services/managedredis/client.go
  • providers/azure/services/managedredis/client_test.go
  • providers/azure/services_test.go
📝 Walkthrough

Walkthrough

Adds Azure Cache for Redis (ServiceMemoryDB) support: provider wiring, a ManagedRedisClient implementing discovery, recommendations, existing commitments, purchase execution, pricing/offering details, and a comprehensive mocked test suite.

Changes

Azure Cache for Redis Service Implementation

Layer / File(s) Summary
Provider registration and service wiring
providers/azure/provider.go, providers/azure/provider_test.go, providers/azure/services.go, providers/azure/services_test.go
ServiceMemoryDB added to supported services; provider switch now constructs NewManagedRedisClient; tests assert presence and client creation.
ManagedRedisClient structure & constructors
providers/azure/services/managedredis/client.go, providers/azure/services/managedredis/client_test.go
Defines HTTPClient and pager interfaces, ManagedRedisClient struct, NewClient/NewClientWithHTTP constructors, pager setters, and accessors for service type and region; includes constructor and setter tests.
Service contract: recommendations, commitments, SKU discovery
providers/azure/services/managedredis/client.go, providers/azure/services/managedredis/client_test.go
Implements GetRecommendations (consumption recommendations paging + conversion), GetExistingCommitments (reservation details filtered to Redis SKUs), ValidateOffering, GetValidResourceTypes with pager-based discovery and curated fallback, and recommendation conversion; tests cover paging, filtering, and conversion paths.
Purchase execution and pricing helpers
providers/azure/services/managedredis/client.go, providers/azure/services/managedredis/client_test.go
Implements PurchaseCommitment (reservation order PUT with management token), GetOfferingDetails (Azure Retail Prices query, upfront/recurring computation), getRedisPricing/parsePriceItems, and RedisPricing/AzureRetailPrice models; tests cover pricing parsing, pagination, and purchase success/failure cases.
Test suite and mocks
providers/azure/services/managedredis/client_test.go
Comprehensive tests with in-file mock pagers, HTTP client, and token credential covering constructors, SKU discovery, validation, recommendations, commitments, pricing parsing, purchase flows (success/error), setter wiring, and helper functions.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • LeanerCloud/CUDly#591: Modifies the shared newServiceClientForSubscription switch in providers/azure/provider.go that this PR extends to route the new ServiceMemoryDB case.

Suggested labels

priority/p2, severity/medium, urgency/this-quarter

Poem

🐰 I hopped into Azure's caching glen,
Found Redis nodes and counted them then.
I parsed prices, paged through the night,
Built a client that hums just right.
Floppy ears, tests passing bright.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 38.89% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding Azure Managed Redis service client support, and references the closed issue.
Linked Issues check ✅ Passed All coding objectives from #556 are met: discovery of Cache for Redis instances, collection of metrics/recommendations, commitment pricing integration, and service structure following AWS MemoryDB patterns.
Out of Scope Changes check ✅ Passed All changes are scoped to Azure Managed Redis implementation and supporting infrastructure; no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/issue-556-azure-managed-redis

Comment @coderabbitai help to get the list of available commands and usage tips.

@cristim cristim added priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/all-users Affects every user effort/l Weeks type/feat New capability triaged Item has been triaged labels May 20, 2026
@cristim

cristim commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
providers/azure/services/managedredis/client_test.go (1)

541-552: ⚡ Quick win

Strengthen conversion test with non-empty Azure payload assertions.

Current test validates only default/static fields from a nil input, so payload-to-recommendation mapping regressions won’t be detected.

🤖 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 541 - 552,
TestConvertRecommendation currently calls convertRecommendation with nil, only
asserting defaults; update the test to supply a realistic, non-nil Azure
recommendation payload and assert that fields are mapped correctly. Construct a
sample payload object matching the type expected by convertRecommendation
(populate unique fields like RecommendationId/ID, InstanceType/SKU, Term,
PaymentOption, Region, SubscriptionID/Account, CommitmentType), call
c.convertRecommendation(ctx, payload), and add assertions that the returned rec
contains the corresponding values (check Provider, Service, Account, Region,
CommitmentType, Term, PaymentOption and at least one payload-derived identifier
or SKU field) to catch regressions in mapping. Ensure you reference the
NewClient and convertRecommendation helpers already used in the test.
🤖 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 `@providers/azure/services/managedredis/client.go`:
- Around line 190-192: The reservationDetailsPager function is hardcoding the
reservation order ID ("00000000-0000-0000-0000-000000000000") which prevents
GetExistingCommitments from discovering real reservations; change the
implementation to first enumerate actual reservation orders for the subscription
(or accept a reservationOrderID param) and pass each real order ID into
client.NewListByReservationOrderPager (instead of the synthetic GUID) so
reservationDetailsPager iterates real orders and returns details for actual
reservation commitments.
- Around line 529-539: convertRecommendation currently ignores its
armconsumption.ReservationRecommendationClassification input and returns a
static Recommendation; update ManagedRedisClient.convertRecommendation to read
the incoming recommendation payload (the function parameter), extract
SKU/instance type, recommended quantity/count, estimated cost/pricing and
term/payment details from the classification object and populate the
corresponding fields on common.Recommendation (e.g., SKU/InstanceType,
Quantity/Count, EstimatedCost/MonthlyCost, Term, PaymentOption) instead of
hardcoding values; preserve existing fields like Provider, Service, Account,
Region, CommitmentType and Timestamp while mapping any available pricing/usage
metadata from the classification into the Recommendation so downstream
validation/pricing uses the real data.
- Around line 229-233: The reservationOrderID generation using time.Now().Unix()
(variable reservationOrderID in providers/azure/services/managedredis/client.go)
can collide under concurrent purchases; change it to use a high-entropy unique
value (e.g., append a UUID or use time.Now().UnixNano() or a crypto-random
suffix) when building reservationOrderID and purchaseURL so each
reservationOrderID is globally unique; update the creation of reservationOrderID
and any dependent code that expects that string (reservationOrderID and
purchaseURL) to use the new unique format.
- Around line 160-169: The code currently hides reservation API failures by
returning an empty commitments slice on pager creation error and breaking
silently on pager.NextPage errors; update reservationDetailsPager() error
handling to return the error (e.g., return nil, err) instead of commitments,
nil, and change the pager iteration that handles page retrieval (pager.NextPage)
to propagate the error (return nil, err or a wrapped error) rather than breaking
the loop so callers can distinguish real API/auth failures from a true "no
commitments" result; refer to reservationDetailsPager, pager.NextPage, and the
commitments return value when making the change.

---

Nitpick comments:
In `@providers/azure/services/managedredis/client_test.go`:
- Around line 541-552: TestConvertRecommendation currently calls
convertRecommendation with nil, only asserting defaults; update the test to
supply a realistic, non-nil Azure recommendation payload and assert that fields
are mapped correctly. Construct a sample payload object matching the type
expected by convertRecommendation (populate unique fields like
RecommendationId/ID, InstanceType/SKU, Term, PaymentOption, Region,
SubscriptionID/Account, CommitmentType), call c.convertRecommendation(ctx,
payload), and add assertions that the returned rec contains the corresponding
values (check Provider, Service, Account, Region, CommitmentType, Term,
PaymentOption and at least one payload-derived identifier or SKU field) to catch
regressions in mapping. Ensure you reference the NewClient and
convertRecommendation helpers already used in the test.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: bb58da09-c5bf-4de5-bc11-f00f48746686

📥 Commits

Reviewing files that changed from the base of the PR and between 0d7bb59 and c203c05.

📒 Files selected for processing (6)
  • providers/azure/provider.go
  • providers/azure/provider_test.go
  • providers/azure/services.go
  • providers/azure/services/managedredis/client.go
  • providers/azure/services/managedredis/client_test.go
  • providers/azure/services_test.go

Comment thread providers/azure/services/managedredis/client.go
Comment thread providers/azure/services/managedredis/client.go Outdated
Comment thread providers/azure/services/managedredis/client.go Outdated
Comment thread providers/azure/services/managedredis/client.go Outdated
@cristim

cristim commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
providers/azure/services/managedredis/client_test.go (1)

171-179: 💤 Low value

Use context.Background() instead of nil context.

Line 173 passes nil as the context to GetValidResourceTypes. While this may work in tests where the context isn't deeply used, it's inconsistent with other tests and could cause issues if the implementation changes to use context features.

Suggested fix
 func TestGetValidResourceTypes_Fallback(t *testing.T) {
 	c := NewClient(nil, "invalid-sub", "eastus")
-	skus, err := c.GetValidResourceTypes(nil)
+	skus, err := c.GetValidResourceTypes(context.Background())
 	require.NoError(t, err)
🤖 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 171 - 179,
In TestGetValidResourceTypes_Fallback replace the nil contexts with
context.Background() so both the NewClient call and the c.GetValidResourceTypes
call use a real context; update NewClient(nil, "invalid-sub", "eastus") and
c.GetValidResourceTypes(nil) to use context.Background() to match other tests
and avoid passing nil into NewClient and GetValidResourceTypes.
providers/azure/services/managedredis/client.go (2)

107-123: 💤 Low value

Pricing API pagination not implemented.

AzureRetailPrice has a NextPageLink field, but getRedisPricing only processes the first page. If the API returns paginated results for a SKU query, some pricing items may be missed, potentially causing incorrect cost calculations.

Also applies to: 452-477

🤖 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.go` around lines 107 - 123,
AzureRetailPrice's NextPageLink is ignored by getRedisPricing causing only the
first page of pricing to be used; update getRedisPricing to follow pagination:
after the initial request parse AzureRetailPrice, append its Items to a
cumulative slice, then while AzureRetailPrice.NextPageLink is non-empty perform
additional GET requests to that NextPageLink (using the same HTTP client/context
used in the file), unmarshal each page into AzureRetailPrice, append Items, and
continue until NextPageLink is empty or an error occurs; ensure errors are
handled and aggregated count/Items are used for final pricing calculations.

516-527: 💤 Low value

Currency and price extraction may be inconsistent with mixed items.

parsePriceItems overwrites currency with any non-empty CurrencyCode and takes the first matching ReservationTerm/Consumption price. If the API returns items with different currencies or multiple matching prices, the result could be unexpected.

Consider filtering items by a consistent currency first, or validating that all items share the same currency.

🤖 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.go` around lines 516 - 527, The
loop in parsePriceItems lets currency be overwritten by any non-empty
CurrencyCode and picks the first matching ReservationTerm or Consumption price,
which can produce inconsistent results when items have mixed currencies; update
parsePriceItems to first determine a single consistent currency (e.g., pick the
most common CurrencyCode or require all non-empty CurrencyCode values to match)
and then filter items to that currency before setting currency, reservation, and
onDemand; if mixed currencies are detected, log or return an error/warning so
callers know the data is inconsistent. Use the existing symbols items,
CurrencyCode, ReservationTerm, Type, currency, reservation, and onDemand to
locate and implement this logic.
🤖 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 171-179: In TestGetValidResourceTypes_Fallback replace the nil
contexts with context.Background() so both the NewClient call and the
c.GetValidResourceTypes call use a real context; update NewClient(nil,
"invalid-sub", "eastus") and c.GetValidResourceTypes(nil) to use
context.Background() to match other tests and avoid passing nil into NewClient
and GetValidResourceTypes.

In `@providers/azure/services/managedredis/client.go`:
- Around line 107-123: AzureRetailPrice's NextPageLink is ignored by
getRedisPricing causing only the first page of pricing to be used; update
getRedisPricing to follow pagination: after the initial request parse
AzureRetailPrice, append its Items to a cumulative slice, then while
AzureRetailPrice.NextPageLink is non-empty perform additional GET requests to
that NextPageLink (using the same HTTP client/context used in the file),
unmarshal each page into AzureRetailPrice, append Items, and continue until
NextPageLink is empty or an error occurs; ensure errors are handled and
aggregated count/Items are used for final pricing calculations.
- Around line 516-527: The loop in parsePriceItems lets currency be overwritten
by any non-empty CurrencyCode and picks the first matching ReservationTerm or
Consumption price, which can produce inconsistent results when items have mixed
currencies; update parsePriceItems to first determine a single consistent
currency (e.g., pick the most common CurrencyCode or require all non-empty
CurrencyCode values to match) and then filter items to that currency before
setting currency, reservation, and onDemand; if mixed currencies are detected,
log or return an error/warning so callers know the data is inconsistent. Use the
existing symbols items, CurrencyCode, ReservationTerm, Type, currency,
reservation, and onDemand to locate and implement this logic.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 20f35628-e74f-4ded-b552-54e6855ddfe0

📥 Commits

Reviewing files that changed from the base of the PR and between c203c05 and cfd6a36.

📒 Files selected for processing (2)
  • providers/azure/services/managedredis/client.go
  • providers/azure/services/managedredis/client_test.go

@cristim

cristim commented May 21, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai resolve

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Comments resolved. Approval is disabled; enable reviews.request_changes_workflow to allow explicit top-level @coderabbitai resolve or @coderabbitai approve commands.

@cristim
cristim force-pushed the feat/issue-556-azure-managed-redis branch from cfd6a36 to d3a54a2 Compare May 21, 2026 12:10
@cristim

cristim commented May 21, 2026

Copy link
Copy Markdown
Member Author

CR findings addressed

First review pass (4 actionable, all fixed in commit 108b736):

Finding Action
Error propagation: GetExistingCommitments silently returned empty on pager init/page errors Fixed: propagate errors with fmt.Errorf wrapping
Hardcoded reservation order ID 00000000-... blocked real commitment discovery Fixed: switched to subscription-scope NewListPager
time.Now().Unix() collision on concurrent purchases Fixed: replaced with uuid.New().String()
convertRecommendation ignored its input, returned static object Fixed: now delegates to recommendations.Extract(rec) and maps all fields

Second review pass (2 actionable, fixed in commit d3a54a2; 2 dismissed):

Finding Action
nil context passed to GetValidResourceTypes in test Fixed: replaced with context.Background()
Pricing API pagination not followed (NextPageLink ignored) Fixed: getRedisPricing now loops over pages; added TestGetOfferingDetails_Paginated
Mixed-currency concern in parsePriceItems Dismissed: Azure Retail Prices API returns a homogeneous currency for a specific SKU+region filter; multi-currency response is not a realistic scenario here
Strengthen TestConvertRecommendation with non-nil payload Dismissed: TestConvertRecommendation_legacy (added in the first CR fix commit) already covers this with a fully populated fixture via BuildLegacyReservationRecommendation and asserts Provider, Service, Account, Region, ResourceType, Count, CommitmentType, Term, PaymentOption, and all three cost fields

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

@cristim: Thank you for the detailed summary — the tracking table makes it very clear what was done and why the two dismissals are justified. The rationale for both is sound:

  • Mixed-currency: Filtering by SKU + region on the Azure Retail Prices API does produce a single-currency response in practice, so defensive multi-currency handling would be over-engineering here.
  • TestConvertRecommendation strengthening: TestConvertRecommendation_legacy with BuildLegacyReservationRecommendation already provides the full field-coverage originally requested.

Triggering a fresh review now to pick up the latest commits.

(◕‿◕✿)🐇

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (2)
providers/azure/services/managedredis/client_test.go (2)

389-400: ⚡ Quick win

Strengthen the 3-year offering test to validate term-specific pricing selection.

TestGetOfferingDetails_3yr currently uses fixture data with only "1 Years" reservation pricing, so it does not actually verify 3-year matching logic. Add a fixture item for "3 Years" and assert expected 3-year-derived values.

🤖 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 389 - 400,
TestGetOfferingDetails_3yr is not actually exercising 3-year logic because the
samplePricingJSON fixture only contains "1 Years" pricing; update the test so
the fixture returned by samplePricingJSON (used in TestGetOfferingDetails_3yr
via NewClientWithHTTP and the mockHTTPClient Do call) includes a pricing entry
with "3 Years" term and appropriate price values, then assert that
GetOfferingDetails(context.Background(),
common.Recommendation{ResourceType:"Premium_P1", Term:"3yr",
PaymentOption:"monthly"}) returns details.Term == "3yr", UpfrontCost == expected
(e.g., 0) and RecurringCost matches the "3 Years" price (use the exact expected
float value from the new fixture) to ensure term-specific selection is
validated.

253-256: ⚡ Quick win

Use non-nil context in TestValidateOffering_InvalidSKU.

Line 255 passes nil as context, which is non-idiomatic for Go tests and can hide context-related failures. Prefer context.Background().

Suggested fix
 func TestValidateOffering_InvalidSKU(t *testing.T) {
 	c := NewClient(nil, "sub", "eastus")
-	err := c.ValidateOffering(nil, common.Recommendation{ResourceType: "Bogus_Z99"})
+	err := c.ValidateOffering(context.Background(), common.Recommendation{ResourceType: "Bogus_Z99"})
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "invalid Azure Cache for Redis SKU")
 }
🤖 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 253 - 256,
Replace the nil context in the test TestValidateOffering_InvalidSKU with a
proper non-nil context (e.g., context.Background()) when calling
c.ValidateOffering; locate the test function TestValidateOffering_InvalidSKU and
change the call c.ValidateOffering(nil, common.Recommendation{ResourceType:
"Bogus_Z99"}) to use context.Background() so NewClient/ValidateOffering receive
a real context for the assertion.
🤖 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 `@providers/azure/services/managedredis/client_test.go`:
- Around line 374-430: Add verification that the mockHTTPClient expectations
were met in every test that sets h.On("Do", ...) but currently doesn't assert
them: update TestGetOfferingDetails_1yr, TestGetOfferingDetails_3yr,
TestGetOfferingDetails_NoUpfront, TestGetOfferingDetails_APIError,
TestGetOfferingDetails_NoPricing (and the PurchaseCommitment tests below) to
call h.AssertExpectations(t) at the end of each test (or register
t.Cleanup(func(){ h.AssertExpectations(t) })) so the mocked Do invocation
defined on the mockHTTPClient is enforced for functions NewClientWithHTTP and
GetOfferingDetails.

In `@providers/azure/services/managedredis/client.go`:
- Around line 365-373: The current code returns whatever partial skuSet
collectSKUsFromPager produced even when the pager encountered an error; change
collectSKUsFromPager to return an error (e.g., func collectSKUsFromPager(ctx,
pager) (map[string]struct{}, error)) and have the caller check that error before
converting skuSet to a slice; on error, discard partial results and return
c.commonSKUs(), nil instead of continuing with partial data. Update both call
sites that build skus from skuSet (the blocks creating skus slice and returning
skus) to handle the new error return and fall back to c.commonSKUs() when
collectSKUsFromPager returns an error. Ensure unique symbols referenced:
collectSKUsFromPager, skuSet, skus, and c.commonSKUs().
- Around line 73-79: NewClientWithHTTP currently stores a nil HTTPClient
directly which can cause a panic later; update NewClientWithHTTP to check if the
provided httpClient is nil and replace it with the package's default HTTPClient
(or http.DefaultClient) before assigning to the ManagedRedisClient field so
ManagedRedisClient.httpClient is never nil; reference the NewClientWithHTTP
constructor and the ManagedRedisClient.httpClient/HTTPClient types when making
the change.
- Around line 548-563: The code currently creates a local recurringCost float
and always returns a non-nil pointer for Recommendation.RecurringMonthlyCost,
losing the nil/missing distinction; change the mapping to pass
f.RecurringMonthlyCost directly into the returned common.Recommendation (i.e.,
set RecurringMonthlyCost: f.RecurringMonthlyCost) instead of creating a
zero-valued pointer via recurringCost so the field stays nil when unavailable
(look for the code building the common.Recommendation in client.go that
references f.RecurringMonthlyCost and recurringCost).

---

Nitpick comments:
In `@providers/azure/services/managedredis/client_test.go`:
- Around line 389-400: TestGetOfferingDetails_3yr is not actually exercising
3-year logic because the samplePricingJSON fixture only contains "1 Years"
pricing; update the test so the fixture returned by samplePricingJSON (used in
TestGetOfferingDetails_3yr via NewClientWithHTTP and the mockHTTPClient Do call)
includes a pricing entry with "3 Years" term and appropriate price values, then
assert that GetOfferingDetails(context.Background(),
common.Recommendation{ResourceType:"Premium_P1", Term:"3yr",
PaymentOption:"monthly"}) returns details.Term == "3yr", UpfrontCost == expected
(e.g., 0) and RecurringCost matches the "3 Years" price (use the exact expected
float value from the new fixture) to ensure term-specific selection is
validated.
- Around line 253-256: Replace the nil context in the test
TestValidateOffering_InvalidSKU with a proper non-nil context (e.g.,
context.Background()) when calling c.ValidateOffering; locate the test function
TestValidateOffering_InvalidSKU and change the call c.ValidateOffering(nil,
common.Recommendation{ResourceType: "Bogus_Z99"}) to use context.Background() so
NewClient/ValidateOffering receive a real context for the assertion.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9a129400-45b0-4868-9fb7-f827343daff8

📥 Commits

Reviewing files that changed from the base of the PR and between cfd6a36 and d3a54a2.

📒 Files selected for processing (6)
  • providers/azure/provider.go
  • providers/azure/provider_test.go
  • providers/azure/services.go
  • providers/azure/services/managedredis/client.go
  • providers/azure/services/managedredis/client_test.go
  • providers/azure/services_test.go

Comment thread providers/azure/services/managedredis/client_test.go
Comment thread providers/azure/services/managedredis/client.go
Comment thread providers/azure/services/managedredis/client.go Outdated
Comment thread providers/azure/services/managedredis/client.go Outdated
@cristim

cristim commented May 21, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 21, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 21, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim

cristim commented May 22, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim force-pushed the feat/issue-556-azure-managed-redis branch from 31c88ae to 2f73e2b Compare May 22, 2026 10:39
@cristim

cristim commented May 22, 2026

Copy link
Copy Markdown
Member Author

Rebased onto latest feat/multicloud-web-frontend. Conflicts in providers/azure/provider.go (GetSupportedServices list + GetServiceClient switch) and providers/azure/services.go (import list + factory funcs) where this branch's ServiceMemoryDB / NewManagedRedisClient additions had to be merged alongside the recently-landed ServiceSavingsPlans + ServiceSearch additions from #592. Both sides preserved; all 464 azure-module tests pass locally.

@cristim

cristim commented May 22, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim added 6 commits May 22, 2026 12:55
…client (closes #556)

Add providers/azure/services/managedredis as Azure's counterpart to AWS
MemoryDB. Azure Cache for Redis is the closest Azure equivalent: a fully-
managed in-memory caching service with reservation-based pricing via the
Azure Consumption and Retail Prices APIs.

The new client registers under ServiceMemoryDB so it is routed and reported
symmetrically with the AWS MemoryDB client at the provider-dispatch level.
It reuses the armredis/v3 and armconsumption SDK packages already present
in the azure provider module - no new dependencies.

Changes:
- providers/azure/services/managedredis/client.go: new ManagedRedisClient
  implementing the full provider.ServiceClient interface (GetRecommendations,
  GetExistingCommitments, PurchaseCommitment, ValidateOffering,
  GetOfferingDetails, GetValidResourceTypes)
- providers/azure/services/managedredis/client_test.go: 34 tests covering
  all methods, pager injection, HTTP mock, token error, SKU fallback
- providers/azure/services.go: NewManagedRedisClient factory
- providers/azure/provider.go: ServiceMemoryDB in GetSupportedServices and
  newServiceClientForSubscription switch
- providers/azure/services_test.go: TestNewManagedRedisClient
- providers/azure/provider_test.go: ServiceMemoryDB assertions in
  GetSupportedServices and AllServiceTypes tests
…r discovery, UUID, recommendation mapping

- Propagate pager-creation and NextPage errors in GetExistingCommitments instead
  of swallowing them (return nil, err rather than empty slice or silent break)
- Replace hardcoded zero-UUID order ID in reservationDetailsPager with the
  subscription-scope NewListPager so all real reservation orders are enumerated;
  update ReservationsDetailsPager interface to ListResponse (not ByReservationOrder)
- Use uuid.New().String() for PurchaseCommitment reservation order ID to avoid
  second-level collisions under concurrent purchases
- Replace static stub in convertRecommendation with recommendations.Extract so
  all Azure recommendation fields (SKU, count, cost, region, term) are mapped
- Update tests: pager mock type, pager-error case now asserts error, new
  convertRecommendation tests cover nil and legacy recommendation shapes
- Follow NextPageLink in getRedisPricing so all pricing pages are
  consumed rather than only the first response
- Replace nil context with context.Background() in
  TestGetValidResourceTypes_Fallback
- Add TestGetOfferingDetails_Paginated to exercise the multi-page
  fetch path end-to-end
…indings

- Guard NewClientWithHTTP against a nil HTTPClient by substituting a
  default *http.Client with a 30s timeout, avoiding nil-deref panics on
  the first Do call.
- Change collectSKUsFromPager to return (map, error) and have
  GetValidResourceTypes discard partial results and fall back to the
  curated commonSKUs() list on pager error, preventing false validation
  failures for valid SKUs.
- Pass the converter's *float64 RecurringMonthlyCost through directly so
  nil (the documented "provider did not return a monthly breakdown"
  sentinel in pkg/common/types.go) stays distinct from an explicit 0.
…er CR

- Register h.AssertExpectations(t) via t.Cleanup on every test using
  mockHTTPClient (TestGetOfferingDetails_{1yr,3yr,NoUpfront,APIError,
  NoPricing} and TestPurchaseCommitment_*), so the mocked Do call is
  required to fire instead of being silently optional.
- Add a "3 Years" reservation entry to samplePricingJSON and assert the
  exact recurring/total cost in TestGetOfferingDetails_3yr so the test
  actually exercises 3-year term selection rather than just asserting
  RecurringCost > 0 against the 1-year fixture.
- Replace nil context with context.Background() in
  TestValidateOffering_InvalidSKU for consistency with other tests.
…servation price

- Add parseTermYears helper (mirrors synapse) to reject any term string
  outside the explicit "1yr"/"3yr" allowlist instead of silently defaulting
  to 1 year in PurchaseCommitment and GetOfferingDetails
- Remove hardcoded 45% reservation-price fallback in getRedisPricing; return
  an error when the API returns no reservation price for the requested SKU
- Add TestPurchaseCommitment_InvalidTerm and TestGetOfferingDetails_InvalidTerm
  to exercise the new error paths
@cristim
cristim force-pushed the feat/issue-556-azure-managed-redis branch from 2f73e2b to fab11b6 Compare May 22, 2026 10:55
@cristim

cristim commented May 22, 2026

Copy link
Copy Markdown
Member Author

CR pass-3 response

All four pass-3 findings have been addressed in the commits already on this branch (fd519d2 + 9c74d47). Summary:

Finding Location Severity Status How addressed
Guard nil custom HTTP client in NewClientWithHTTP client.go:84 Major Fixed fd519d202 — nil check added; falls back to &http.Client{Timeout: 30s}
Don't return partial SKU set after pager failure client.go:373 Major Fixed fd519d202collectSKUsFromPager now returns (map[string]bool, error); caller discards partial results and falls back to commonSKUs() on error
Preserve nil when recurring monthly cost is unavailable client.go:563 Minor Fixed fd519d202 — removed the intermediate var recurringCost float64 intermediary; f.RecurringMonthlyCost (*float64) is passed through directly
Add h.AssertExpectations(t) to all mockHTTPClient tests client_test.go:451 Major Fixed 9c74d477bt.Cleanup(func() { h.AssertExpectations(t) }) added to every TestGetOfferingDetails_* and TestPurchaseCommitment_* test that sets h.On("Do", ...)

Test run: 38/38 pass (go test ./services/managedredis/... -count=1 -short). Build clean (go build ./...).

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

@cristim: All four pass-3 fixes are well-handled:

  • Nil HTTP client guard: The &http.Client{Timeout: 30 * time.Second} fallback is a solid default — prevents panics on nil dereference and adds a sensible production timeout.
  • Partial SKU set on pager failure: Returning (map[string]bool, error) from collectSKUsFromPager and discarding partial results on error is the right fail-safe pattern — avoids serving a truncated list that could silently miscategorize resources.
  • Nil-preserving recurring cost: Passing f.RecurringMonthlyCost (*float64) directly removes the implicit zero-value coercion, which keeps nil semantics intact downstream.
  • AssertExpectations in cleanup: Using t.Cleanup rather than a deferred call at the top of each test is the idiomatic testify pattern and ensures mock expectations are always checked even on early t.Fatal.

Triggering review to pick up fd519d2 + 9c74d47.

(◕‿◕✿)🐇

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit 78d3273 into feat/multicloud-web-frontend May 22, 2026
5 checks passed
@cristim

cristim commented May 22, 2026

Copy link
Copy Markdown
Member Author

All 4 actionable CodeRabbit findings verified against HEAD (fab11b6) and already resolved before merge:

  1. h.AssertExpectations(t) in all mockHTTPClient tests - Added via t.Cleanup(func() { h.AssertExpectations(t) }) in all GetOfferingDetails_* and PurchaseCommitment_* tests (commit 9c74d47).

  2. Nil HTTP client guard in NewClientWithHTTP - Nil check added: falls back to &http.Client{Timeout: 30 * time.Second} (commit fd519d2), mirroring the synapse client pattern from PR feat(providers/azure/synapse): add data-warehouse service client (closes #555) #622.

  3. No partial SKU sets on pager errors - collectSKUsFromPager now returns (map[string]bool, error) and propagates errors; GetValidResourceTypes discards partial results and falls back to commonSKUs() (commit fd519d2).

  4. Nil RecurringMonthlyCost preserved - convertRecommendation passes f.RecurringMonthlyCost (*float64) directly, keeping nil distinct from zero (commit fd519d2).

Tests: 38 passed, go vet clean, gofmt clean. PR merged at 2026-05-22T11:17:10Z with all 5 CI checks green.

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

@cristim: Clean closure — all four pass-4 findings confirmed resolved at HEAD (fab11b6) before merge, and the CI gate held. The implementation is in great shape:

  • t.Cleanup + AssertExpectations ensures mock exhaustion is always verified even on early failures.
  • The nil HTTP client guard with a 30 s timeout is a solid production-safe default.
  • The (map[string]bool, error) return from collectSKUsFromPager with discard-on-error is exactly the right fail-closed pattern.
  • Passing f.RecurringMonthlyCost (*float64) directly keeps nil semantics intact downstream.

Triggering the requested review to confirm no new issues surfaced in the rebase commits.

(◕‿◕✿)🐰✨

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/l Weeks impact/all-users Affects every user priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/feat New capability urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant