feat(providers/azure/synapse): add data-warehouse service client (closes #555) - #622
Merged
cristim merged 6 commits intoMay 22, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRegisters Azure DataWarehouse (Synapse) as a supported service, wires a NewSynapseClient factory, and adds a Synapse client implementing recommendations, existing commitments, pricing/offering validation, reservation purchase flows, helpers, and comprehensive unit tests with mocks. ChangesSynapse Reserved Capacity Support
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Provider as AzureProvider
participant Synapse as SynapseClient
participant Azure as Azure APIs
App->>Provider: Request client for ServiceDataWarehouse
Provider->>Synapse: NewSynapseClient(cred, subID, region)
Synapse-->>Provider: ServiceClient
Provider-->>App: SynapseClient
App->>Synapse: GetRecommendations(params)
Synapse->>Azure: ARM Consumption Recommendations (paged)
Azure-->>Synapse: Recommendations pages
Synapse-->>App: []common.Recommendation
App->>Synapse: GetOfferingDetails(rec)
Synapse->>Azure: Retail Prices OData (service/region/SKU)
Azure-->>Synapse: Price items (paged)
Synapse-->>App: OfferingDetails
App->>Synapse: PurchaseCommitment(rec, opts)
Synapse->>Azure: PUT Reservation Orders (management token)
Azure-->>Synapse: Order response (success/error)
Synapse-->>App: common.PurchaseResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
providers/azure/services/synapse/client_test.go (1)
393-415: ⚡ Quick winAdd regression tests for invalid term and missing reservation price.
Given purchase/offering rely on term parsing and reservation price availability, add explicit negative tests to prevent fallback regressions.
✅ Suggested tests
+func TestPurchaseCommitment_invalidTerm(t *testing.T) { + mHTTP := &mockHTTPClient{} + cred := &mockTokenCredential{token: "test-token"} + c := NewClientWithHTTP(cred, "sub-123", "eastus", mHTTP) + + rec := common.Recommendation{ResourceType: "DW500c", Term: "2yr", Count: 1} + _, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + require.Error(t, err) +} + +func TestGetOfferingDetails_missingReservationPrice(t *testing.T) { + const noReservationJSON = `{"Items":[{"currencyCode":"USD","retailPrice":0.5,"type":"Consumption","skuName":"DW100c"}]}` + mHTTP := &mockHTTPClient{} + mHTTP.On("Do", mock.Anything).Return(newHTTPResponse(http.StatusOK, noReservationJSON), nil) + + c := &SynapseClient{subscriptionID: "sub-123", region: "eastus", httpClient: mHTTP} + rec := common.Recommendation{ResourceType: "DW100c", Term: "1yr", PaymentOption: "upfront"} + _, err := c.GetOfferingDetails(context.Background(), rec) + require.Error(t, err) +}Also applies to: 439-450
🤖 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/synapse/client_test.go` around lines 393 - 415, Add two negative unit tests for SynapseClient.GetOfferingDetails: one that passes a Recommendation with an invalid Term (e.g., "invalid") and asserts an error is returned, and another that uses a valid Term but simulates the pricing response missing the reservation price (mockHTTP returning a JSON without reservationPrice) and asserts the method returns an error (or a defined no-offer error). Reference the existing test patterns (TestGetOfferingDetails_monthly and TestGetOfferingDetails_httpError) to reuse mockHTTP setup, SynapseClient{subscriptionID, region, httpClient}, and common.Recommendation to construct inputs and assertions so these tests prevent regressions in term parsing and reservation-price handling.providers/azure/provider_test.go (1)
1151-1157: ⚡ Quick winAdd
ServiceDataWarehouseto the non-account client matrix test.Good addition here for
GetServiceClientForAccount; please also includecommon.ServiceDataWarehouseinTestAzureProvider_GetServiceClient_AllServiceTypes(Line 406 onward) so both client paths are regression-protected.✅ Suggested test update
func TestAzureProvider_GetServiceClient_AllServiceTypes(t *testing.T) { @@ testCases := []struct { service common.ServiceType }{ {common.ServiceCompute}, {common.ServiceRelationalDB}, {common.ServiceCache}, + {common.ServiceDataWarehouse}, }🤖 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/provider_test.go` around lines 1151 - 1157, The test matrix in TestAzureProvider_GetServiceClient_AllServiceTypes is missing common.ServiceDataWarehouse; update the services slice used in that test to include common.ServiceDataWarehouse (matching the list used in the GetServiceClientForAccount test) so the non-account client path also exercises the DataWarehouse service type; look for the services variable in TestAzureProvider_GetServiceClient_AllServiceTypes and append common.ServiceDataWarehouse to it.
🤖 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/synapse/client.go`:
- Around line 236-239: The current logic silently coerces unknown reservation
terms to 1 year by setting termYears := 1 and only accepting "3yr"/"3"; change
this to validate rec.Term explicitly and fail closed: detect supported values
(e.g., "1yr"/"1" and "3yr"/"3"), convert them to termYears accordingly, and
return/propagate an error when rec.Term is unrecognized instead of defaulting to
1; apply the same validation change to the other identical block handling
termYears (the later block referring to rec.Term around lines 319-322) so both
code paths fail on unsupported terms.
- Around line 149-153: GetExistingCommitments currently swallows errors from
createReservationsPager by returning an empty slice and nil error; change the
error handling in GetExistingCommitments to propagate the initialization error
(return nil, err) instead of returning []common.Commitment{} with a nil error,
and consider wrapping the error with context (e.g.,
fmt.Errorf("createReservationsPager: %w", err)) before returning to make
failures from createReservationsPager visible to callers.
- Around line 241-251: Before constructing requestBody, validate the purchase
inputs: ensure rec.ResourceType (SKU) is non-empty and rec.Count is a positive
integer; if validation fails return an explicit error (e.g., fmt.Errorf("invalid
SKU: %q" or "invalid quantity: %d") ) so the caller fails fast. Add these checks
at the start of the function that builds requestBody (where requestBody,
rec.Count and rec.ResourceType are used) and only proceed to create requestBody
and call the external API when both validations pass.
- Around line 415-418: The code block that sets reservationPrice = onDemandPrice
* hoursInTerm * 0.60 invents a synthetic 40% discount when reservation data is
missing; remove this hardcoded fallback and treat missing reservation data as
unknown instead. Replace the fallback assignment with a clear "unknown" signal
(e.g., leave reservationPrice as 0 but return a boolean like
reservationAvailable=false or return an error/nullable value) and add a warning
log (processLogger or similar) so callers can handle absent reservation pricing;
update any callers of reservationPrice to check the new availability
flag/nullable and avoid using an assumed 40% discount. Ensure you reference the
reservationPrice, onDemandPrice, and hoursInTerm variables (and the function
that returns them) when making these changes.
---
Nitpick comments:
In `@providers/azure/provider_test.go`:
- Around line 1151-1157: The test matrix in
TestAzureProvider_GetServiceClient_AllServiceTypes is missing
common.ServiceDataWarehouse; update the services slice used in that test to
include common.ServiceDataWarehouse (matching the list used in the
GetServiceClientForAccount test) so the non-account client path also exercises
the DataWarehouse service type; look for the services variable in
TestAzureProvider_GetServiceClient_AllServiceTypes and append
common.ServiceDataWarehouse to it.
In `@providers/azure/services/synapse/client_test.go`:
- Around line 393-415: Add two negative unit tests for
SynapseClient.GetOfferingDetails: one that passes a Recommendation with an
invalid Term (e.g., "invalid") and asserts an error is returned, and another
that uses a valid Term but simulates the pricing response missing the
reservation price (mockHTTP returning a JSON without reservationPrice) and
asserts the method returns an error (or a defined no-offer error). Reference the
existing test patterns (TestGetOfferingDetails_monthly and
TestGetOfferingDetails_httpError) to reuse mockHTTP setup,
SynapseClient{subscriptionID, region, httpClient}, and common.Recommendation to
construct inputs and assertions so these tests prevent regressions in term
parsing and reservation-price handling.
🪄 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: 21d3cb9e-e8be-4ab7-841c-72bc520a3bab
📒 Files selected for processing (5)
providers/azure/provider.goproviders/azure/provider_test.goproviders/azure/services.goproviders/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
providers/azure/services/synapse/client.go (1)
325-325: 💤 Low valueConsider handling the
io.ReadAllerror.The error from reading the response body is discarded. If the read fails, the error message at line 327 will be incomplete or misleading.
🔧 Suggested improvement
- body, _ := io.ReadAll(resp.Body) + body, readErr := io.ReadAll(resp.Body) if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusAccepted { + if readErr != nil { + return "", fmt.Errorf("reservation purchase failed with status %d (body read error: %v)", resp.StatusCode, readErr) + } return "", fmt.Errorf("reservation purchase failed with status %d: %s", resp.StatusCode, string(body)) }🤖 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/synapse/client.go` at line 325, The read of the HTTP response body discards the error; change the call in the Synapse client (where body, _ := io.ReadAll(resp.Body) is used) to capture and handle the error (e.g., body, err := io.ReadAll(resp.Body)), then if err != nil return or log a clear error including err (or wrap and return it) before using body so downstream error logs or returns aren’t misleading; update the surrounding error handling in the same function to surface the read error.
🤖 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/synapse/client.go`:
- Line 325: The read of the HTTP response body discards the error; change the
call in the Synapse client (where body, _ := io.ReadAll(resp.Body) is used) to
capture and handle the error (e.g., body, err := io.ReadAll(resp.Body)), then if
err != nil return or log a clear error including err (or wrap and return it)
before using body so downstream error logs or returns aren’t misleading; update
the surrounding error handling in the same function to surface the read error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b3953b6-7703-4503-9f4e-d6d05dd5370d
📒 Files selected for processing (2)
providers/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
Member
Author
|
@coderabbitai resolve |
Contributor
✅ Actions performedComments resolved. Approval is disabled; enable |
cristim
force-pushed
the
feat/issue-555-azure-synapse
branch
from
May 21, 2026 12:05
63d89ff to
01f2618
Compare
Member
Author
CodeRabbit review responseAddressed in commit 940e4bc (previous push)
Addressed in commit 01f2618 (this push)
Dismissed
@coderabbitai review |
Contributor
|
The dismissals are also well-justified — the existing (ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ ✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
providers/azure/services/synapse/client_test.go (2)
174-247: ⚡ Quick winAdd explicit Modern/MCA recommendation-shape coverage.
Line 174–247 only validate legacy recommendation fixtures. Since this client claims support for both legacy and modern shapes, add at least one test that feeds a modern/MCA payload and asserts conversion fields.
🤖 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/synapse/client_test.go` around lines 174 - 247, Add a new unit test alongside TestGetRecommendations_singlePage/TestGetRecommendations_multiPage that uses the same fakeRecommendationsPager to supply a modern/MCA-shaped reservation recommendation payload (build or mock a modern armconsumption.ReservationRecommendationClassification with modern fields) and call c.GetRecommendations(ctx, common.RecommendationParams{}); then assert the returned recommendation(s) include expected converted fields (e.g., Service, ResourceType, Count, CommitmentType, Term/PaymentOption, OnDemandCost/CommitmentCost/EstimatedSavings and any modern-specific Details shape) to ensure the conversion logic in GetRecommendations handles MCA/modern shapes as well as legacy ones.
300-323: ⚡ Quick winAdd a positive SCU SKU commitment test.
Line 300–323 verifies non-Synapse filtering, but there’s no positive test for
SCU*SKUs. Add one to lock in the advertisedDW*/SCUacceptance behavior.🤖 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/synapse/client_test.go` around lines 300 - 323, Add a new positive unit test (e.g., TestGetExistingCommitments_acceptSynapseSCU) to complement TestGetExistingCommitments_filterNonSynapse: construct a test client via newTestClient(), set c.SetReservationsPager with a fakeReservationsPager page whose ReservationDetail.Properties.SKUName is an SCU-style SKU (e.g., "SCU_*" or a DW/SCU pattern), call c.GetExistingCommitments(ctx) and assert no error and that the returned commitments slice contains the expected commitment (non-empty and/or matches the SKU). Use the same types used in the diff (fakeReservationsPager, armconsumption.ReservationsDetailsClientListResponse, armconsumption.ReservationDetail, armconsumption.ReservationDetailProperties, SKUName) to locate where to add 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.
Nitpick comments:
In `@providers/azure/services/synapse/client_test.go`:
- Around line 174-247: Add a new unit test alongside
TestGetRecommendations_singlePage/TestGetRecommendations_multiPage that uses the
same fakeRecommendationsPager to supply a modern/MCA-shaped reservation
recommendation payload (build or mock a modern
armconsumption.ReservationRecommendationClassification with modern fields) and
call c.GetRecommendations(ctx, common.RecommendationParams{}); then assert the
returned recommendation(s) include expected converted fields (e.g., Service,
ResourceType, Count, CommitmentType, Term/PaymentOption,
OnDemandCost/CommitmentCost/EstimatedSavings and any modern-specific Details
shape) to ensure the conversion logic in GetRecommendations handles MCA/modern
shapes as well as legacy ones.
- Around line 300-323: Add a new positive unit test (e.g.,
TestGetExistingCommitments_acceptSynapseSCU) to complement
TestGetExistingCommitments_filterNonSynapse: construct a test client via
newTestClient(), set c.SetReservationsPager with a fakeReservationsPager page
whose ReservationDetail.Properties.SKUName is an SCU-style SKU (e.g., "SCU_*" or
a DW/SCU pattern), call c.GetExistingCommitments(ctx) and assert no error and
that the returned commitments slice contains the expected commitment (non-empty
and/or matches the SKU). Use the same types used in the diff
(fakeReservationsPager, armconsumption.ReservationsDetailsClientListResponse,
armconsumption.ReservationDetail, armconsumption.ReservationDetailProperties,
SKUName) to locate where to add the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7be6a8ae-cd25-4c35-bae9-c93d8f8280b9
📒 Files selected for processing (5)
providers/azure/provider.goproviders/azure/provider_test.goproviders/azure/services.goproviders/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
cristim
force-pushed
the
feat/issue-555-azure-synapse
branch
from
May 22, 2026 11:08
01f2618 to
a3c72fd
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/azure/provider_test.go (1)
258-268:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpand service-matrix coverage for newly wired Azure services.
The updated tests cover
ServiceDataWarehouse, but the newly supportedServiceSavingsPlansandServiceSearchservice routes are still not asserted in the supported-services and client-construction matrices. Please add them to these test cases to keep provider wiring fully guarded.✅ Suggested test additions
func TestAzureProvider_GetSupportedServices(t *testing.T) { @@ assert.Contains(t, services, common.ServiceCache) + assert.Contains(t, services, common.ServiceSavingsPlans) + assert.Contains(t, services, common.ServiceSearch) assert.Contains(t, services, common.ServiceDataWarehouse) } func TestAzureProvider_GetServiceClient_AllServiceTypes(t *testing.T) { @@ testCases := []struct { service common.ServiceType }{ {common.ServiceCompute}, {common.ServiceRelationalDB}, {common.ServiceCache}, + {common.ServiceSavingsPlans}, + {common.ServiceSearch}, {common.ServiceDataWarehouse}, } @@ } func TestAzureProvider_GetServiceClientForAccount(t *testing.T) { @@ services := []common.ServiceType{ common.ServiceCompute, common.ServiceRelationalDB, common.ServiceCache, common.ServiceNoSQL, + common.ServiceSavingsPlans, + common.ServiceSearch, common.ServiceDataWarehouse, }Also applies to: 406-413, 1152-1158
🤖 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/provider_test.go` around lines 258 - 268, Update the Azure provider tests to assert the newly wired services: in TestAzureProvider_GetSupportedServices add assertions for common.ServiceSavingsPlans and common.ServiceSearch so AzureProvider.GetSupportedServices is fully covered; likewise, update the Azure client-construction matrix tests (the other Azure provider tests that validate service-to-client wiring) to include entries/expectations for ServiceSavingsPlans and ServiceSearch so the provider's client factory and routing are exercised for those services as well.
🤖 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/synapse/client.go`:
- Around line 114-145: The GetRecommendations method appends every converted
recommendation regardless of region; update the loop in GetRecommendations to
only append recommendations whose region matches the client region (c.region)
after converting (use convertSynapseRecommendation). Specifically, after
converted := c.convertSynapseRecommendation(rec) and nil-check, add a
region-gate (e.g., compare converted.Region or converted.Location to c.region
using a case-insensitive comparison like strings.EqualFold) and skip appending
to recs when they don't match; keep all other paging and error handling
unchanged.
---
Outside diff comments:
In `@providers/azure/provider_test.go`:
- Around line 258-268: Update the Azure provider tests to assert the newly wired
services: in TestAzureProvider_GetSupportedServices add assertions for
common.ServiceSavingsPlans and common.ServiceSearch so
AzureProvider.GetSupportedServices is fully covered; likewise, update the Azure
client-construction matrix tests (the other Azure provider tests that validate
service-to-client wiring) to include entries/expectations for
ServiceSavingsPlans and ServiceSearch so the provider's client factory and
routing are exercised for those services as well.
🪄 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: 7ece8411-1b92-41da-a597-189dda013de2
📒 Files selected for processing (5)
providers/azure/provider.goproviders/azure/provider_test.goproviders/azure/services.goproviders/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
#555) Add Azure Synapse Analytics Reserved Capacity service client as the Azure counterpart to AWS Redshift (issue #473 parity gap). - New package providers/azure/services/synapse implements the full ServiceClient interface: GetRecommendations, GetExistingCommitments, PurchaseCommitment, ValidateOffering, GetOfferingDetails, GetValidResourceTypes. - Recommendations sourced from Azure Consumption API (armconsumption.ReservationRecommendationsClient) using the "SQLDatabaseDTU" resource type filter; supports both Legacy (EA) and Modern (MCA) billing account shapes via the shared internal/recommendations.Extract converter. - Existing commitments retrieved via armconsumption.ReservationsDetails, filtered to DW*/SCU SKUs that identify Synapse SQL Pool reservations. - Purchase issues a PUT to Microsoft.Capacity/reservationOrders using reservedResourceType "SqlDW"; purchase-automation tag applied when PurchaseOptions.Source is non-empty. - Pricing via Azure Retail Prices API (azure-synapse service name); handles "1 Year" / "3 Years" reservation term strings as returned by the live API. - GetValidResourceTypes returns static DW100c-DW30000c list (same approach as AWS Redshift client); no armsynapse SDK dependency needed. - Wire into provider.go (GetSupportedServices + newServiceClientForSubscription) and services.go factory; update provider_test.go to assert ServiceDataWarehouse. - 31 unit tests covering all methods, pager mocks, HTTP mocks, credential errors, SKU filtering, pricing extraction, and tag application.
…n, input guards, pricing safety - Propagate pager-creation error in GetExistingCommitments instead of returning empty - Extract parseReservationTermYears helper that fails closed on unknown terms; replace both silent 1yr-coercion blocks in PurchaseCommitment and GetOfferingDetails - Guard PurchaseCommitment against empty ResourceType and non-positive Count - Remove synthetic 40% reservation-cost fabrication in getSynapsePricing; return a clear "pricing data unavailable" error when the API returns no reservation price - Add test coverage for all four new code paths
- Propagate io.ReadAll error in doPurchaseRequest; when the HTTP response body read fails, surface a clear error message including the read failure rather than silently using an empty/partial body string in the status-error message. - Add common.ServiceDataWarehouse to TestAzureProvider_GetServiceClient_AllServiceTypes so both the account and non-account client paths are regression-protected for the DataWarehouse service type.
cristim
force-pushed
the
feat/issue-555-azure-synapse
branch
from
May 22, 2026 11:24
a3c72fd to
98a249b
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
The pager returns recommendations for all regions; downstream offering/purchase logic is region-bound. Gate each converted recommendation on strings.EqualFold(converted.Region, c.region) so callers receive only the region they constructed the client for. Adds TestGetRecommendations_regionFilter (verifies cross-region rec is dropped) and TestGetRecommendations_modernShape (Modern/MCA payload shape coverage requested by CR). Addresses CR finding on PR #622 (2026-05-22T11:20:18Z).
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
providers/azure/provider_test.go (1)
1155-1161: ⚡ Quick winAdd MemoryDB to explicit-account service coverage.
GetServiceClientForAccountcoverage now includes DataWarehouse but still skipscommon.ServiceMemoryDB, so this test won’t catch regressions in that routing path.Proposed test update
services := []common.ServiceType{ common.ServiceCompute, common.ServiceRelationalDB, common.ServiceCache, common.ServiceNoSQL, + common.ServiceMemoryDB, common.ServiceDataWarehouse, }🤖 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/provider_test.go` around lines 1155 - 1161, The test's services list used by GetServiceClientForAccount omits the memory DB service; update the services slice in providers/azure/provider_test.go (the variable containing []common.ServiceType) to include common.ServiceMemoryDB alongside common.ServiceDataWarehouse so the test exercises the memory DB routing path of GetServiceClientForAccount and will catch regressions for that service.
🤖 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/synapse/client.go`:
- Around line 67-73: NewClientWithHTTP currently stores a nil httpClient and
later code (e.g., purchase/pricing paths calling c.httpClient.Do) can panic;
update NewClientWithHTTP to guard against a nil HTTPClient parameter by
substituting a default implementation (e.g., http.DefaultClient or a
package-level default that implements HTTPClient) when httpClient == nil so
c.httpClient is never nil, and add/update tests for NewClientWithHTTP and the
SynapseClient methods that call c.httpClient.Do to assert no nil deref occurs.
---
Nitpick comments:
In `@providers/azure/provider_test.go`:
- Around line 1155-1161: The test's services list used by
GetServiceClientForAccount omits the memory DB service; update the services
slice in providers/azure/provider_test.go (the variable containing
[]common.ServiceType) to include common.ServiceMemoryDB alongside
common.ServiceDataWarehouse so the test exercises the memory DB routing path of
GetServiceClientForAccount and will catch regressions for that service.
🪄 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: cd18f42e-a22b-4f27-aec3-534c7ff55681
📒 Files selected for processing (5)
providers/azure/provider.goproviders/azure/provider_test.goproviders/azure/services.goproviders/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
…st matrices Add the two newly wired services to all three test functions that guard provider service wiring: TestAzureProvider_GetSupportedServices, TestAzureProvider_GetServiceClient_AllServiceTypes, and TestAzureProvider_GetServiceClientForAccount. Also add ServiceMemoryDB to the GetServiceClientForAccount matrix which was missing. Guard NewClientWithHTTP against a nil httpClient by substituting http.DefaultClient, preventing a nil-dereference panic on purchase and pricing paths that call c.httpClient.Do. Closes CR findings: test matrix gap (ServiceSavingsPlans/ServiceSearch) and nil HTTP client guard.
Member
Author
CR findings addressedFinding 1 - Test matrix gap (ServiceSavingsPlans, ServiceSearch): Commit
Finding 2 - Region filter in GetRecommendations: Already fixed in commit Finding 3 - Nil HTTP client guard in NewClientWithHTTP: Fixed in All 512 tests pass ( @coderabbitai review |
Contributor
|
512/512 passing with clean (ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ ✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/synapse/client.go`:
- Around line 349-351: The validation currently compares SKUs with exact case
using the loop over validSKUs and rec.ResourceType, which rejects SKUs with
different casing; change the comparison to a case-insensitive one (e.g., use
strings.EqualFold(sku, rec.ResourceType)) inside the loop so "dw1000c" and
"DW1000C" match, and add the strings import if not already present; ensure the
branch still returns nil on match.
- Around line 209-210: The SKU filter is too broad because it uses
strings.Contains(skuLower, "scu") and can match unrelated SKUs; update the
conditional around skuLower (the variable in client.go) to detect SCU and
Synapse shapes by prefix instead of substring. Replace the contains check with
prefix checks (e.g., strings.HasPrefix(skuLower, "scu") and
strings.HasPrefix(skuLower, "synapse") or similar Synapse-specific prefixes) so
the line becomes something like checking !strings.HasPrefix(skuLower, "dw") &&
!strings.HasPrefix(skuLower, "scu") && !strings.HasPrefix(skuLower, "synapse")
(or adjust prefixes to the exact Synapse SKU shapes used in your inventory).
🪄 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: 312054ce-2716-412c-ba36-e8d32490a935
📒 Files selected for processing (5)
providers/azure/provider.goproviders/azure/provider_test.goproviders/azure/services.goproviders/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
Comment on lines
+209
to
+210
| if !strings.HasPrefix(skuLower, "dw") && !strings.Contains(skuLower, "scu") && !strings.Contains(skuLower, "synapse") { | ||
| return nil |
Contributor
There was a problem hiding this comment.
Tighten Synapse SKU filtering to avoid false-positive commitments.
Line 209 matches any SKU containing "scu", which can misclassify unrelated reservation SKUs as Synapse. Restrict SCU detection to Synapse-like SKU shapes (e.g., prefix-based) to keep commitment inventory accurate.
Suggested fix
- if !strings.HasPrefix(skuLower, "dw") && !strings.Contains(skuLower, "scu") && !strings.Contains(skuLower, "synapse") {
+ if !strings.HasPrefix(skuLower, "dw") &&
+ !strings.HasPrefix(skuLower, "scu") &&
+ !strings.Contains(skuLower, "synapse") {
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if !strings.HasPrefix(skuLower, "dw") && !strings.Contains(skuLower, "scu") && !strings.Contains(skuLower, "synapse") { | |
| return nil | |
| if !strings.HasPrefix(skuLower, "dw") && | |
| !strings.HasPrefix(skuLower, "scu") && | |
| !strings.Contains(skuLower, "synapse") { | |
| return nil | |
| } |
🤖 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/synapse/client.go` around lines 209 - 210, The SKU
filter is too broad because it uses strings.Contains(skuLower, "scu") and can
match unrelated SKUs; update the conditional around skuLower (the variable in
client.go) to detect SCU and Synapse shapes by prefix instead of substring.
Replace the contains check with prefix checks (e.g., strings.HasPrefix(skuLower,
"scu") and strings.HasPrefix(skuLower, "synapse") or similar Synapse-specific
prefixes) so the line becomes something like checking
!strings.HasPrefix(skuLower, "dw") && !strings.HasPrefix(skuLower, "scu") &&
!strings.HasPrefix(skuLower, "synapse") (or adjust prefixes to the exact Synapse
SKU shapes used in your inventory).
Merged
5 tasks
…ve offering validation Addresses CodeRabbit round-5 findings on PR #622: - L210: Replace `strings.Contains(skuLower, "scu")` with `strings.HasPrefix(skuLower, "scu")` so unrelated SKUs that merely contain "scu" as a substring (e.g. "rescue_*") are no longer misclassified as Synapse reservations. - L351: Use `strings.EqualFold` with `strings.TrimSpace` in `ValidateOffering` so a valid SKU with different casing (e.g. "dw1000c") is accepted, matching the rest of the Azure provider's case-insensitive comparison pattern. Adds tests: - TestValidateOffering_caseInsensitive - TestValidateOffering_trimsWhitespace - TestConvertSynapseReservation_scuPrefix - TestConvertSynapseReservation_scuSubstringNotMatched (regression guard)
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
5 tasks
5 tasks
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
1 participant
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.
Summary
providers/azure/services/synapsepackage implementing theServiceClientinterface for Azure Synapse Analytics Reserved Capacity (Dedicated SQL Pool DWU reservations)provider.go(GetSupportedServices+newServiceClientForSubscription) andservices.gofactory functionImplementation notes
Azure Synapse Analytics does have a reservation product (confirmed via Azure Retail Prices API): DW100c through DW30000c Dedicated SQL Pool DWU SKUs and Spark Compute Units (SCUs). The
armsynapseSDK does not expose a reservation API surface - reservations are issued viaMicrosoft.Capacity/reservationOrders(same path as all other Azure services), so no new SDK dependency was added.The implementation follows the exact pattern established by
cacheandcosmosdb:armconsumption.ReservationRecommendationsClient(resourceType filter:SQLDatabaseDTU)armconsumption.ReservationsDetailsClient(filtered to DW*/SCU SKUs)reservedResourceType: "SqlDW")Azure Synapse Analytics)internal/recommendations.ExtractconverterTest plan
providers/azure/services/synapse/client_test.gocovering allServiceClientinterface methods, pager mocks, HTTP mocks, credential errors, SKU filtering, pricing extraction (1yr/3yr), and tag applicationgo test ./providers/azure/services/synapse/... -count=1 -short-- 31/31 passgo test ./providers/azure/... -count=1 -short-- 421/421 pass (was 420 before this PR)go build ./...-- cleango mod tidy-- no changes (no new dependencies)TestAzureProvider_GetSupportedServicesandTestAzureProvider_GetServiceClientForAccountupdated to assertServiceDataWarehouseSummary by CodeRabbit
New Features
Tests