fix(providers/azure): send canonical reservedResourceType enum values - #1208
Conversation
|
@coderabbitai review |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughReplace hardcoded Azure Reservations reservedResourceType literals with armreservations SDK enum constants across multiple service clients, document the SearchService exception, and add tests validating canonical enum usage in Database and Synapse purchase request payloads. ChangesCanonical reservedResourceType Enum Adoption
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/azure/services/synapse/client.go (1)
230-233:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale doc comment to match canonical enum value.
Line 232 still states
"SqlDW", but the request now sends the canonical SDK value (SqlDataWarehouse). Keeping this stale text risks future regressions.Suggested doc-only fix
-// Reservations API two-step flow (calculatePrice -> purchase). The reserved -// resource type is "SqlDW" which covers Dedicated SQL Pool DWU reservations. +// Reservations API two-step flow (calculatePrice -> purchase). The reserved +// resource type is the canonical SDK enum value "SqlDataWarehouse" +// (armreservations.ReservedResourceTypeSQLDataWarehouse), which covers +// Dedicated SQL Pool DWU reservations.🤖 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 230 - 233, The doc comment for PurchaseCommitment is outdated: it says the reserved resource type is "SqlDW" but the code/SDK uses the canonical enum value "SqlDataWarehouse"; update the comment inside the PurchaseCommitment function header to replace "SqlDW" with "SqlDataWarehouse" (or otherwise note the canonical SDK enum) so the comment matches the actual value used and avoids future confusion with the Reservations API flow.
🤖 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.
Outside diff comments:
In `@providers/azure/services/synapse/client.go`:
- Around line 230-233: The doc comment for PurchaseCommitment is outdated: it
says the reserved resource type is "SqlDW" but the code/SDK uses the canonical
enum value "SqlDataWarehouse"; update the comment inside the PurchaseCommitment
function header to replace "SqlDW" with "SqlDataWarehouse" (or otherwise note
the canonical SDK enum) so the comment matches the actual value used and avoids
future confusion with the Reservations API flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18667251-f17d-4df1-b178-724b603dca3f
📒 Files selected for processing (9)
providers/azure/services/cache/client.goproviders/azure/services/compute/client.goproviders/azure/services/cosmosdb/client.goproviders/azure/services/database/client.goproviders/azure/services/database/client_test.goproviders/azure/services/managedredis/client.goproviders/azure/services/search/client.goproviders/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
8c2975b to
fd4b778
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Adversarial review -- PR #1208Reviewed against the stated risk surfaces (SDK enum canonicality, case-sensitivity, sibling enum-as-string patterns, JSON wire format, test fixtures, search/* literal). Net: no blocking findings. Two out-of-scope follow-ups filed. What I checked
CI stateThe five failing checks (
None of those are introduced by this PR. From a code-correctness standpoint the diff is mergeable; the UNSTABLE state reflects repo-level debt that should be addressed at the repo level. Out-of-scope follow-ups filed
Minor nits (no fix required, not blocking)
VerdictNo fix needed on PR #1208 itself. The core claim (SDK constants land canonical wire strings, regression tests fail pre-fix and pass post-fix) is fully verified. Re-request CR once the unrelated CI failures are addressed at the repo level, then merge. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
fd4b778 to
359ef73
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
The Azure SQL Database and Synapse purchase bodies sent hand-written
reservedResourceType literals ("SqlDatabase", "SqlDW") that are not
members of the armreservations ReservedResourceType enum; Azure would
reject calculatePrice with an invalid-value error once non-VM purchases
become reachable (known-issues #731). Replace them with the canonical
SDK constants ReservedResourceTypeSQLDatabases ("SqlDatabases") and
ReservedResourceTypeSQLDataWarehouse ("SqlDataWarehouse").
Also convert the already-correct literals in cache, cosmosdb, compute,
and managedredis to their SDK constants so all purchase bodies are
compile-time-checked enum members, and document that search's
"SearchService" has no SDK enum counterpart (v1.1.0 and v2.0.0).
Regression tests capture the calculatePrice request body and assert the
sent value equals the SDK constant and is a member of
PossibleReservedResourceTypeValues(); both fail against the old
literals.
Closes #1189
359ef73 to
d6647f4
Compare
|
Rebased onto origin/main (132 commits behind). Clean rebase, 1 commit. Gates (providers/azure module):
Branch force-pushed: fix/arch-01-azure-reserved-resource-type @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
echo '== Branch and effective PR diff =='
git status --short
git log --oneline --decorate -3
base="$(git merge-base HEAD origin/main)"
printf 'merge-base: %s\n' "$base"
git diff --stat "$base"...HEAD
git diff --name-only "$base"...HEAD
echo
echo '== Changed-file outlines =='
git diff --name-only "$base"...HEAD | while IFS= read -r file; do
case "$file" in
*.go)
echo "--- $file"
ast-grep outline "$file" --items all --view compact || true
;;
esac
done
echo
echo '== Patch =='
git diff --unified=55 "$base"...HEAD -- 'providers/azure/services/**/*.go'
echo
echo '== Relevant enum/payload occurrences in changed Azure service files =='
rg -n -C 4 --glob '*.go' \
'(ReservedResourceType|reservedResourceType|calculatePrice|PossibleReservedResourceTypeValues|AssertExpectations|\.Body\.Close\s*\()' \
providers/azure/services
echo
echo '== Azure Reservations dependency declaration =='
rg -n -C 2 'armreservations' go.mod providers/azure/go.mod 2>/dev/null || trueLength of output: 201565 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
base="$(git merge-base HEAD origin/main)"
echo '== Exact changed files and concise patch =='
git diff --name-status "$base"...HEAD
git diff --unified=12 "$base"...HEAD
echo
echo '== Focused changed implementation regions =='
for file_and_range in \
'providers/azure/services/cache/client.go:300,340' \
'providers/azure/services/compute/client.go:410,450' \
'providers/azure/services/cosmosdb/client.go:300,332' \
'providers/azure/services/database/client.go:300,337' \
'providers/azure/services/managedredis/client.go:248,280' \
'providers/azure/services/search/client.go:286,310' \
'providers/azure/services/synapse/client.go:226,280' \
'providers/azure/services/database/client_test.go:1245,1305' \
'providers/azure/services/synapse/client_test.go:850,915'
do
file="${file_and_range%%:*}"
range="${file_and_range#*:}"
echo "--- $file ($range)"
sed -n "${range}p" "$file"
done
echo
echo '== Test helper response ownership and bodyclose configuration =='
rg -n -C 3 --glob '*.go' \
'(func (createMockHTTPResponse|newHTTPResponse|fakeHTTPResp)\b|bodyclose|io\.NopCloser)' \
providers/azure/services/database providers/azure/services/synapse \
.golangci.yml .golangci.yaml 2>/dev/null || true
echo
echo '== All changed reservedResourceType assignments =='
git diff --unified=0 "$base"...HEAD -- 'providers/azure/services/**/*.go' |
rg -n -C 2 '^[+-].*reservedResourceType|^[+-].*ReservedResourceType|^[+-].*SqlDW|^[+-].*SqlDatabase'Length of output: 51844
One non-blocking nit: The five converted purchase bodies use the intended SDK constants, the ✅ Action performedReview finished.
|
|
Merged to main after pre-merge review (rebased onto current main, CLEAN + all CI green). Replaces hand-written Azure |
Problem
Closes #1189 (review finding ARCH-01, P2).
The Azure SQL Database and Synapse reservation purchase bodies sent hand-written
reservedResourceTypeliterals that are not members of thearmreservations.ReservedResourceTypeenum:providers/azure/services/database/client.go:"SqlDatabase"(canonical:"SqlDatabases")providers/azure/services/synapse/client.go:"SqlDW"(canonical:"SqlDataWarehouse")Once the #731 role fix makes non-VM Azure purchases reachable, calculatePrice would reject these values, breaking the purchase path for both services. This repeats the PR #1047 failure mode ("MEMORY_MB" instead of the SDK's "MEMORY").
Fix
string(armreservations.ReservedResourceTypeSQLDatabases)andstring(armreservations.ReservedResourceTypeSQLDataWarehouse).ReservedResourceTypeRedisCache,ReservedResourceTypeCosmosDb,ReservedResourceTypeVirtualMachines), so every expressible purchase body value is a compile-time-checked enum member. Zero behavior change for these four."RedisCache": valid enum member, now uses the SDK constant."SearchService": has no counterpart in the armreservations enum in v1.1.0 or v2.0.0 (no search/cognitive-search member exists at all), so it cannot be expressed as an SDK constant; documented in a comment and left for live-catalog verification.resourceType eq 'SqlDatabase'atdatabase/client.go:173is a different API surface (armconsumption reservation recommendations filter, not the Capacity purchase enum) and is intentionally left unchanged here; it needs live verification before touching the read path.Tests
New regression tests capture the calculatePrice request body and assert the sent
reservedResourceTypeequals the SDK constant AND is a member ofarmreservations.PossibleReservedResourceTypeValues():TestDatabaseClient_PurchaseCommitment_CanonicalReservedResourceTypeTestPurchaseCommitment_canonicalReservedResourceType(synapse)Verified both FAIL against the pre-fix code (stash fix, run, unstash): expected
"SqlDatabases"got"SqlDatabase", expected"SqlDataWarehouse"got"SqlDW".Verification run:
go build ./...(root module and providers/azure module): passgo vet ./providers/azure/...: cleango test ./services/...in providers/azure: 495 passed in 9 packagesSummary by CodeRabbit
reservedResourceTypevalues sent in thecalculatePricerequest for SQL Database and Synapse.