Repository navigation
fix(azure): roll out reservation fixes and preserve exchange scopes - #475
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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. 📝 WalkthroughWalkthroughAzure exchange preview and execution now use tenant-listed reservation metadata with requested quantities. The Azure provider dependency changed. Exchange quote fixtures now specify ChangesAzure exchange updates
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ExchangeHandler
participant AzureClient
participant CalculateExchange
ExchangeHandler->>AzureClient: List tenant reservations
ExchangeHandler->>ExchangeHandler: Check ownership and apply requested quantities
ExchangeHandler->>CalculateExchange: Price verified reservation sources
sequenceDiagram
participant ExecuteHandler
participant AzureClient
participant CalculateExchange
ExecuteHandler->>AzureClient: List and verify tenant reservations
ExecuteHandler->>CalculateExchange: Re-quote verified sources
ExecuteHandler->>AzureClient: Execute exchange using authorized client
Merge Risk: ⚪ Minimal · up to Azure exchange preview and execution now keep server-owned reservation scopes and change only the requested quantity. No concrete merge-blocking issue was found. Live-cloud acceptance has not been tested, so the final CI results should be confirmed before merging. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation Issue
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
CI result for exact commit
Keep this PR open. Do not treat the passing affected-path tests as green CI or full security coverage. Verify the combined tree and required CI after #473 lands; the upstream Azure stack and real-scenario acceptance also remain open. |
db21110 to
4345f7e
Compare
|
Rebased onto current main (3131779) to clear the DIRTY merge state. Conflict shape: go.mod/go.sum only - main had bumped providers/aws to v0.0.0-20261003204812-9962786e0695 while this PR bumps providers/azure to v0.0.0-20261004143708-56555e1be095. Resolution keeps both bumps (newest of each); go.sum regenerated via go mod tidy. Verified locally: go build ./... and go test ./internal/... ./cmd/... all pass. |
|
@coderabbitai review |
|
|
Coordination note from the w2 shard (orchestrator rollout relay for cloud-commitments-go#218): upstream PR #218 merged to main as |
|
Round summary for head
@coderabbitai review |
|
|
Coordination note from the (now closed, superseded) PR #651 — I hit and fixed a CI failure that this PR will also hit, since its Symptom (reproduced locally on the identical pin; your Unit Tests job on 11fdc1f shows the same signature): Root cause: upstream Fix (verified locally: --- a/internal/server/handler_ri_exchange_test.go
+++ b/internal/server/handler_ri_exchange_test.go
@@ fixtures: in each of the 5 exchange.ExchangeQuoteSummary literals
IsValidExchange: true,
+ CurrencyCode: "USD",
PaymentDueUSD: new(big.Rat).SetFloat64(...),The literals are at current lines ~550 (5.00), ~640 and ~647 (3.50, AutoMode), ~744 and ~751 (8.00, DailyCapHitMidRun). A ready-to-apply patch with the exact hunks is on my closed PR's branch Note the |
|
Evidence on the current Unit Tests failure at 11fdc1f (from my superseded #652, which carried the same azure+pkg pin pair and hit the identical CI failure): Failing tests: Root cause: the pkg bump changes Implication: this PR can't go green until the new exchange refusals are mapped (the #521 work), or it merges with Unit Tests red. Suggest sequencing #521's fix into this branch or ahead of it. Not touching this PR per the deconfliction relay — posting evidence only. |
|
Fleet-relay note: this PR pins providers/azure at e24345c (#218), which predates cloud-commitments-go#207 (fail-closed Azure savings-plan inventory, merged as 58c25f0). Filed #654 to track the follow-up pin bump after this lands; consumer audit there shows no caller impact in this repo, so nothing needed in this PR. |
|
Fixed the dependency-rollout CI failure in Before the fix, the manual-mode, auto-mode and daily-cap reshape tests failed with expected 1, got 0. After the fix, those cases and all six reshape tests pass under The inherited 160-file worktree was compared with current main The PR dependency note now records that upstream #187 merged and identifies the current Azure/pkg pins. PR labels mirror closing issue #650. Exact @coderabbitai full review |
|
|
CI is green on 0b3480c: both workflow runs 37647177559 and 37647177592 completed successfully, including unit and integration tests. CodeRabbit refused the full review with an explicit rate-limit response; its next included review was stated as available in 49 minutes. No CR review is claimed. Exact Opus review and real-scenario acceptance remain required, and this worker has not merged. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Live reconciliation: CodeRabbit full review has now completed on exact head 0b3480c, covering go.mod and the exchange fixture test; go.sum was excluded by its configured path filter. It reports no actionable findings and there are no inline findings. The docstring-coverage advisory concerns existing self-describing test functions with only fixture fields changed; adding prose comments would conflict with the repository comments/scope rules, so it is declined. CI is green and the head is mergeable/CLEAN. Required independent claude-opus-5-5 review on this full head and real-account acceptance evidence are still missing. No CR waiver merge has occurred, no duplicate review trigger or watcher is needed, and the coordinator retains dispatch and merge coordination. |
0b3480c to
545d1e2
Compare
|
The final scope fix is rebased onto main after #535, at Independent local implementation and staged reviews are clean. The 14-case concrete Azure handler scope matrix passes on the fixed source; all four success cases fail with the original handler. Monthly/upfront fresh/retry purchase-manager probes pass with the current Azure pin and fail with the old pin's incorrect recorded costs. These are synthetic SDK/HTTP/local fixtures, with no live-cloud purchase or exchange performed. Before rebase, fresh build/vet and the full race-short suite passed (31 tested packages). The rebased build passed; a fresh combined suite/vet and independent exact-head replay/review are in progress under canonical locks. Fresh CI is being watched; this comment does not claim merge readiness. @coderabbitai review |
|
|
Fixed CI's sole CI-pinned golangci-lint v2.10.1 reports zero issues in both the root module and E2E module. The 14-case concrete Azure handler matrix and fresh build pass; two implementation and two staged review passes are clean, and normal commit hooks passed. Fresh full race-short suite/vet and independent exact-head replay are in progress; new CI is watched. The prior CR request was explicitly review-rate-limited at #475 (comment). Under the owner's current rule, a continued quota block will be waived only after independent adversarial review, adequate local verification and fresh exact-head green CI. No merge yet; fixture evidence has no live-cloud purchase/exchange coverage. @coderabbitai review |
|
|
Merge evidence for exact reviewed head
Commands independently reproduced on this head: CR waived: quota, adversarial review + local verification + green CI Exact-head CR reply explicitly reports Review rate limited. No available actionable findings remain. This waived head is recorded for retrospective full review when quota returns; no duplicate review timer is created. Evidence uses realistic fixtures and synthetic SDK/HTTP responses through affected handlers/provider/manager; no live-cloud purchase/exchange or account/catalog acceptance performed. Existing merge-triggered CI/development deployment is authorized by the owner. Final current head/main/green-check/clean-mergeability verification runs immediately under the sole coordinator canonical merge lock, with normal branch protections and exact match-head merge. No bypass or branch/worktree deletion. |
The previous pin 56555e1be095 diverged from cloud-commitments-go main and does not contain the merged fix for the Azure monthly run-rate being divided by the term months (e24345ce68f8, cloud-commitments-go#218). Bump providers/azure to v0.0.0-20261007130138-e24345ce68f8. The shared pkg pin advances to v0.0.0-20261006104817-90e61e668b99, the version the new Azure module requires; providers/aws and providers/gcp stay pinned. The delta also corrects Azure Consumption recommendation units: cost fields are now monthly run-rates and monthly variants carry the recurring charge in RecurringMonthlyCost with CommitmentCost zero. The platform mapping (scheduler.go: CommitmentCost -> UpfrontCost, RecurringMonthlyCost -> MonthlyCost) and recTotalCommitment (upfront plus monthly x term months) already match these semantics, so no call-site changes are needed. Refs #650
The shared pkg pin required by the Azure rollout rejects quotes with an absent currency. These five fixtures model USD quotes, so declare their currency explicitly and keep production currency validation intact. The three failing reshape scenarios reproduce before this change and pass afterward. The fresh backend race suite, build and vet also pass.
Reuse the owned reservation listing when pricing preview and execution. Keep applied scope metadata and overwrite only the requested quantity, so the upgraded Azure provider can preserve Shared and Single scopes. Billing ownership and all existing execution gates retain their order. Concrete-client regression tests fail on the original handler and pass after the mapping repair, including partial quantities and refusal cases.
Index the server-owned reservation slice while preserving the same case-insensitive lookup keys and by-value metadata mapping. This resolves the CI-pinned gocritic rangeValCopy finding without changing scope handling.
0ea0be8 to
5009085
Compare
|
Rebased onto #482's merged main at Independent combined source review is clean. #482's Azure resolver uses the shared HTTP client upgraded here; audited contracts are compatible, and existing Key Vault/metadata-refusal fixtures will be independently replayed alongside the 14 scope and four concrete purchase-manager cases. The new combined build passed; matching pinned lint/full race suite/vet and exact-head replay/CI are in progress under canonical locks. CR's previous exact-head reply explicitly reported a review rate limit. A continued quota block will use the owner's waiver only after all remaining review/local/CI/mergeability/coordination gates pass. Fixtures are synthetic SDK/HTTP/local, with no live-cloud purchase or exchange claimed. @coderabbitai review |
✅ Action performedReview finished.
|
|
Final merge evidence: reviewed head Independent adversarial review of the complete five-file diff and upstream contracts: APPROVE, no actionable findings. All five rebased patches are unchanged. The scope regression and pinned-lint range-copy finding are resolved. Fresh Go1.26.6 build, vet, CI-pinned golangci-lint2.10.1 root/E2E lint, normal hooks and full uncached serial race-short suite pass; 31 tested packages pass. Independent exact-head replay passes 32 scenarios: exchange scope14 (API2.738s), concrete Azure Manager4 (purchase1.889s), and inherited Key Vault TLS/metadata14 (secrets22.045s). SHA, clean tree and committed-source/fixture hashes were verified before/after under canonical locks. Original-handler and old-pin failing controls remain valid by verified source equivalence; they were reproduced earlier and retained, not presented as new executions. Commands: Fresh exact-head Build/Test CI and pre-commit CI SUCCESS; terminal watchers collected. CodeRabbit is available on this final head. Its review marker explicitly covers CR advisory triage: the scope warning is addressed by defining the required compatibility acceptance on linked issue #650. A pin-only upgrade reproduces four valid preview/execution failures because the mapper discards metadata required by the new provider; this repair is necessary for the rollout. The docstring-percentage warning is declined with justification: production helpers retain focused ownership/mapping comments, private test fixtures are self-describing, and unrelated comment expansion would conflict with repository scope/comment guidance. No missing public contract or actionable documentation defect was identified. Coverage is realistic synthetic Azure SDK/HTTP and local TLS fixtures, with no live-cloud purchase/exchange or account/catalog acceptance claimed. Existing merge-triggered CI/development deployments are owner-authorized. The normal merge checks current head, authoritative main, all green checks and CLEAN mergeability under the sole canonical git-merge lock, with |
|
To: codex-coordinator; platform655; all platform merge owners |
|
To: codex-coordinator; platform655; all platform merge owners |
… 500 (#1212) The pinned cloud-commitments-go versions already carry go#203 (exchange.ErrQuoteNotUSD) and go#209 (azurecompute.ErrUnsupportedAppliedScope), and toAzureExchangeSources already builds sources from the owned reservations (#475). But mapAWSExchangeError and mapAzureExchangeError only knew provider 4xx faults, so both refusals came back as a generic 500 ("exchange execution failed", "failed to price the exchange ...") and the AWS direct-execute audit row was recorded as unknown although the refusal happens before AcceptReservedInstancesExchangeQuote. Both mappers now check the exported refusal sentinels first and return a 422 carrying the go message, matching the Azure execute guardrails' currency-mismatch 422. openapi.yaml documents the Azure list fields (applied_scope_type, applied_scopes) and the new 422s. Refs #521 Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This rolls the Azure reservation fixes into the platform and preserves server-owned reservation scope through exchange preview and execution. The upgraded provider rejects missing scope metadata; both handlers now reuse the owned reservation listing and replace only the requested quantity.
Azure is pinned to
v0.0.0-20261007130138-e24345ce68f8, with shared pkgv0.0.0-20261006104817-90e61e668b99. AWS and GCP pins are unchanged. Five quote fixtures now supply the required USD currency.Verification uses realistic synthetic Azure SDK/HTTP responses through the actual authenticated handlers, concrete Azure client and purchase manager. Four Shared/Single preview/execution success cases and ten metadata/billing refusal cases pass; all four success cases fail with the original handler. An independent reviewer reproduced this evidence. Monthly/upfront purchases and retries pass through the concrete purchase manager; the old Azure pin reproduces incorrect recorded costs in all four control cases.
No live-cloud purchase or exchange was performed. Live account/catalog acceptance remains an honestly reported coverage gap, not a merge gate under the owner's current instructions. Final exact-head review, fresh build/full race-short suite/vet and CI evidence will be recorded in the merge evidence comment.
Closes #650
Refs LeanerCloud/cloud-commitments-go#76 and LeanerCloud/cloud-commitments-go#132
Summary by CodeRabbit