Skip to content

feat(exchange)!: pass the source RI's availability zone to LookupOffering - #428

Merged
cristim merged 1 commit into
mainfrom
feat/310-exchange-lookup-az
Oct 16, 2026
Merged

cristim merged 1 commit into
mainfrom
feat/310-exchange-lookup-az

Conversation

@cristim

@cristim cristim commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Summary

pkg half of #310. resolveOffering (pkg/exchange/auto.go) called the offering lookup with instance type, product, tenancy, scope and duration but no availability zone, so the provider could not keep an exchange target in the source RI's zone.

  • RIMetadataInfo gains AvailabilityZone.
  • LookupOfferingFunc now takes a LookupOfferingParams struct (the five existing values plus AvailabilityZone) instead of positional strings, and resolveOffering forwards the AZ.

Breaking for the platform (the only consumer; nothing in this repo outside tests assigns LookupOffering or fills RIMetadata): after bumping its pkg pin it must adapt its LookupOffering closure to the struct, pass the AZ into the provider's FindConvertibleOfferingParams.AvailabilityZone (#427), and set RIMetadataInfo.AvailabilityZone from the source RI. Until it does, a zonal RI reaches the provider with a blank AZ and the lookup fails loud (no silent cross-zone exchange). Platform follow-up: LeanerCloud/cloud-commitments-platform#921.

Closes #310. Merge after #427 (the provider half, independent code) so the issue does not close before both halves land.

Verification (macOS, Go 1.26.9)

  • (cd pkg && GOWORK=off go test -race -short ./...): pass. make build, make vet: pass. golangci-lint v2.10.1 (CI pin) from pkg: 0 issues. GOWORK=off go build ./... in all five modules: pass (providers still use their pinned pkg).
  • Fail-before: a signature change can only "fail" by not compiling, so the behavioural proof removes the AZ forwarding line; TestResolveOffering_ForwardsAvailabilityZone then fails (AvailabilityZone: "", want "us-east-1b"). It asserts full struct equality with distinct values per field.
  • Independent non-author review (same reviewer as the plan and fix(ec2): filter RI exchange offering lookups by availability zone #427): no actionable findings on tree 9dd3761a. Its mutation swapping Tenancy and Scope in resolveOffering fails the test.

Unit tests with stub lookups only; no AWS call, exchange or purchase.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 2 billable files and costs up to $0.50.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 17 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 64 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 6011ed71-e156-4c7b-8c6f-857ce2b40b94
📥 Commits

Reviewing files that changed from the base of the PR and between 0e95e9f and d0495fb.

📒 Files selected for processing: 2
  • pkg/exchange/auto.go
  • pkg/exchange/auto_test.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

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 Oct 16, 2026

Copy link
Copy Markdown
Member Author

CI is green on head a2c5c0ca: pre-commit 38836505833 and Build & Test 38836505864. CodeRabbit reached its free review limit on this PR and only offers a paid on-demand review, which I'm not using. Per the review gate, the head is instead covered by the independent non-author review recorded in the body (no actionable findings on tree 9dd3761a, field-swap mutation caught) and by the local verification listed there.

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Independent adversarial review: CLEAN at a2c5c0cabbc070c5be19c41222a9097a1a27d4a9 (claude-go-pr-merge-2; non-author reviewer, cold brief, full diff vs 740dfbd).

resolveOffering forwards meta.AvailabilityZone through LookupOfferingParams; the provider already fills ConvertibleRI.AvailabilityZone from the RI (providers/aws/services/ec2/client.go ~807), regional RIs empty. Nothing in providers/*, ci_cd_sanity_tests or pkg outside pkg/exchange references LookupOfferingFunc/RIMetadataInfo/LookupOfferingParams; the break is confined to the platform (LeanerCloud/cloud-commitments-platform#921), matching the ! title. Until the platform forwards the AZ, a zonal RI fails loud at the provider (intended, no silent cross-zone exchange).

Mutation probe: dropping AvailabilityZone: meta.AvailabilityZone makes TestResolveOffering_ForwardsAvailabilityZone fail.

Local verification (macOS, unit/mock evidence) at the exact head: (cd pkg && go test -race -short ./exchange/...) and (cd providers/aws && go test -race -short ./services/ec2/...) pass; go build ./... and go vet ./... clean in all five modules under go.work; make build, make vet pass; scratch merge with #427 clean.

CI green on a2c5c0ca (14/14). Labels mirror #310. Merge order: after #427. CR skipped: quota. CR waived: quota, adversarial review + local verification + green CI

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Automated review of a2c5c0c: no blocking findings and CI is green. Ready to merge.
Small, correct signature change. The AZ is forwarded from RIMetadataInfo through the new LookupOfferingParams struct. No other callers in the repo, and the new test checks full struct equality with distinct values per field. The breaking change is documented, with platform follow-up tracked.

  • (optional) pkg/exchange/auto_test.go:1089: The new test is the only one with a non-empty AZ. Add a regional-RI case with an empty AZ to pin the empty-string pass-through, though this is optional.

Posted by a read-only review sweep. It stays silent on this commit and re-reviews only after a new push.

…ring

resolveOffering could not tell the offering lookup which zone a zonal
convertible RI lives in, so the provider had no way to keep the target in
that zone. RIMetadataInfo gains AvailabilityZone, and LookupOfferingFunc
now takes a LookupOfferingParams struct carrying it alongside the existing
fields.

BREAKING CHANGE: LookupOfferingFunc takes LookupOfferingParams instead of
six positional arguments; callers must also fill RIMetadataInfo.AvailabilityZone
for zonal RIs.

Closes #310

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristim
cristim force-pushed the feat/310-exchange-lookup-az branch from a2c5c0c to d0495fb Compare October 16, 2026 14:50
@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Rebased onto main 0e95e9f (after #425 and #427 merged; main also has 25bffb5, which touched pkg/exchange/auto.go). No conflicts. git range-diff 740dfbd..a2c5c0ca 0e95e9f..d0495fb0 is 1: a2c5c0c = 1: d0495fb, so the CLEAN verdict at a2c5c0ca stands for d0495fb0c99ed8b548f0724df42c612f79cfec91. Local check at the new head (macOS, unit/mock): (cd pkg && GOWORK=off go test -race -short ./exchange/...) ok; go build ./... ok in all five modules; (cd providers/aws && go test -race -short ./services/ec2/...) ok. Waiting for CI.

@cristim

cristim commented Oct 16, 2026

Copy link
Copy Markdown
Member Author

Merge evidence at d0495fb0c99ed8b548f0724df42c612f79cfec91 (claude-go-pr-merge-2, owner-authorized): independent review CLEAN at a2c5c0ca; d0495fb0 is a pure rebase onto 0e95e9f (range-diff =), merged after #427 as required. No open review threads. CI green on d0495fb0 (pre-commit 38919057756, Build & Test 38919058031). Local verification (macOS, unit/mock) recorded above. Labels mirror #310. Platform follow-up: LeanerCloud/cloud-commitments-platform#921. CR waived: quota, adversarial review + local verification + green CI

@cristim
cristim merged commit 551be56 into main Oct 16, 2026
15 checks passed
@cristim
cristim deleted the feat/310-exchange-lookup-az branch October 16, 2026 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/few Limited audience priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(ec2): RI exchange offering lookups ignore the availability zone

1 participant