Repository navigation
refactor(azure): build reservation purchase bodies from SDK enum constants - #448
Conversation
|
Warning Review limit reached
This review includes 12 billable files and costs up to $3.00.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 39 minutes for your next included review. View limit details
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 12 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: using Azure SDK enum constants to build reservation purchase bodies. Linked Issues check The changes satisfy #22and#14. All seven listed clients now use string(armreservations.AppliedScopeTypeShared). They now use reservations.ReservationTermForYears instead of formatted term strings. T…Out of Scope Changes check The changed comment, shared term helper, fake HTTP capture transport, and regression tests directly support #22and#14. The seven client changes are limited to the linked enum conversions and early e…Full details: Linked Issues check
-
Explanation
The changes satisfy
#22and#14. All seven listed clients now use string(armreservations.AppliedScopeTypeShared). They now use reservations.ReservationTermForYears instead of formatted term strings. The helper returns ReservationTermP1Y and ReservationTermP3Y and returns an error for other values. Tests cover both supported terms, unsupported input, zero requests on early rejection, and the expected request bodies for all seven clients. The exact wire values remain Shared, P1Y, and P3Y.
Full details: Out of Scope Changes check
-
Explanation
The changed comment, shared term helper, fake HTTP capture transport, and regression tests directly support
#22and#14. The seven client changes are limited to the linked enum conversions and early error handling. No unrelated service or reservation workflow changes are shown.
-
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
cristim
added
effort/s
|
@coderabbitai review |
|
Independent review verdict: CLEAN at 4780538Reviewer: fresh Claude agent (Sonnet, high effort), not the author. Coordinator: claude-cc-reviewer. Equivalence check: the seven
Compared fields: reservedResourceType, billingScopeId, term, billingPlan, quantity, appliedScopeType, sku.name, location, renew, tags, displayName.
Non-blocking:
Gate:
|
|
CI is green on head |
4780538 to
74b3ce1
Compare
|
Rebased onto main (BEHIND). The range-diff is identical, so the CLEAN verdict carries over to 4780538. (claude-cc-reviewer) |
|
Correction: the rebased head is 74b3ce1; my earlier note read the head before GitHub finished updating. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments: 12
providers/azure/services/purchasebody_test.go: 1
59-70: Pin the loop variables for closure safety.The subtests capture
svc,term, andwant. The go.mod declares Go 1.26.9. Per-iteration loop variable semantics apply, so the code is correct. No change is needed.providers/azure/services/internal/reservations/term.go: 1
9-21: LGTM!providers/azure/services/cache/client.go: 1
324-328: LGTM!Also applies to: 344-344, 355-355
providers/azure/services/compute/client.go: 1
416-419: LGTM!Also applies to: 427-427, 438-438
providers/azure/services/cosmosdb/client.go: 1
325-329: LGTM!Also applies to: 345-345, 356-356
providers/azure/services/database/client.go: 1
357-361: LGTM!Also applies to: 377-377, 388-388
providers/azure/services/managedredis/client.go: 1
306-310: LGTM!Also applies to: 326-326, 329-329
providers/azure/services/search/client.go: 1
17-17: LGTM!Also applies to: 245-249, 270-270, 281-281
providers/azure/services/synapse/client.go: 1
315-319: LGTM!Also applies to: 335-335, 338-338
providers/azure/internal/recommendations/converter.go: 1
48-48: LGTM!providers/azure/services/internal/reservations/term_test.go: 1
10-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Convert this test to a table-driven test.
The test uses two loops with different case shapes. A single table with
years,want, andwantErrfields is clearer. Run each case witht.Run. The failure output then names the exact case.♻️ Proposed refactor
func TestReservationTermForYears(t *testing.T) { - for years, want := range map[int]string{1: "P1Y", 3: "P3Y"} { - got, err := ReservationTermForYears(years) - require.NoError(t, err) - assert.Equal(t, want, string(got)) - } - for _, years := range []int{0, 5} { - _, err := ReservationTermForYears(years) - assert.Error(t, err, "years=%d", years) - } + tests := []struct { + name string + years int + want string + wantErr bool + }{ + {"one year", 1, "P1Y", false}, + {"three years", 3, "P3Y", false}, + {"zero", 0, "", true}, + {"five years", 5, "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ReservationTermForYears(tt.years) + if tt.wantErr { + assert.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, string(got)) + }) + } }Based on learnings: prefer table-driven tests for multiple input/output scenarios of the same function.
Source: Learnings
providers/azure/services/internal/reservations/reservationstest/capture.go: 1
49-58: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.Lock
muwhen readingBodyBytes.
DowritesBodyBytesundermu.BodyWithoutDisplayNamereads it without the lock. A concurrent call is a data race. The current tests call it sequentially, so no failure occurs today. Acquire the lock for the read to keep the helper safe for reuse.🔒️ Proposed fix
func (c *Capture) BodyWithoutDisplayName() ([]byte, error) { + c.mu.Lock() + raw := c.BodyBytes + c.mu.Unlock() var body map[string]any - if err := json.Unmarshal(c.BodyBytes, &body); err != nil { + if err := json.Unmarshal(raw, &body); err != nil {
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
- Review profile: CHILL
- Plan: Essentials
- Run ID:
2ae819c4-abbd-4028-8f9f-49e018fca280
📒 Files selected for processing: 12
providers/azure/internal/recommendations/converter.goproviders/azure/services/cache/client.goproviders/azure/services/compute/client.goproviders/azure/services/cosmosdb/client.goproviders/azure/services/database/client.goproviders/azure/services/internal/reservations/reservationstest/capture.goproviders/azure/services/internal/reservations/term.goproviders/azure/services/internal/reservations/term_test.goproviders/azure/services/managedredis/client.goproviders/azure/services/purchasebody_test.goproviders/azure/services/search/client.goproviders/azure/services/synapse/client.go
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.
…tants
The seven Azure reservation clients wrote appliedScopeType and term as
string literals ("Shared", fmt.Sprintf("P%dY", years)). They now use
armreservations.AppliedScopeTypeShared and ReservationTermP1Y/P3Y through
one reservations.ReservationTermForYears helper, which errors on any other
term. The request bodies are byte-identical: one table test captures
every service's calculatePrice body and passes unchanged against the old
clients.
Closes #22
Closes #14
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
74b3ce1 to
bc23972
Compare
1 participant
Summary
The seven Azure reservation clients (cache, compute, cosmosdb, database, managedredis, search, synapse) built their
calculatePrice/ purchase bodies from string literals:"appliedScopeType": "Shared"andfmt.Sprintf("P%dY", termYears).string(armreservations.AppliedScopeTypeShared).string(term), wheretermcomes from the newreservations.ReservationTermForYears(termYears)inproviders/azure/services/internal/reservations/term.go. It is an explicit switch: 1 maps toReservationTermP1Y, 3 maps toReservationTermP3Y, and any other value is an error. It never formats a string.ParseTermYearsstill runs first and only yields 1 or 3, so no previously accepted input changes.internal/recommendations/converter.go:48, which said the clients hardcode "Shared", now names the SDK constant.Closes #22. Closes #14, the
appliedScopeTypeSDK-constant part.Not touched:
internal/reservations/{idempotencymarker,purchase,strict}.go, which open PRs #422, #423 and #439 own.Verification (macOS, Go 1.26.9)
Byte identity. One table test,
providers/azure/services/purchasebody_test.go(packageservices_test), captures each service's request body through a fake HTTP client (internal/reservations/reservationstest). It compares the body against literal golden JSON for 1yr and 3yr, after stripping only the timestampeddisplayName. The same test passes unchanged with the original HEADclient.gofiles swapped in, so the wire format is identical.Unsupported term. The
5yrcase pins the early rejection byParseTermYears, with zero requests sent.term_test.gocovers the helper, including rejecting 0 and 5.Mutation probes. The independent reviewer ran these in a scratch copy, and each made
TestPurchaseBodiesfail:AppliedScopeTypeSingle, including search with its import kept;Checks.
(cd providers/azure && GOWORK=off go test -race -short ./...)passes.make buildandmake vetpass.gocyclo -over 10is clean.Review. The same non-author reviewer did two passes. Its pass-1 finding was that the 5-year branches were unreachable, so they were trimmed. No actionable findings on tree
986aacda.Fake HTTP transport only; no Azure call or purchase.
🤖 Generated with Claude Code
Summary by CodeRabbit