Skip to content

feat(commitmentopts): use AWS SP probe in Validate path (closes #134) - #825

Merged
cristim merged 2 commits into
mainfrom
fix/134-wave9
Jul 19, 2026
Merged

cristim merged 2 commits into
mainfrom
fix/134-wave9

Conversation

@cristim

@cristim cristim commented May 28, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Removes stale comment in commitmentopts.Validate that described Savings Plans as "not commitment-capable in our probe set" -- PR feat(commitmentopts): probe Savings Plans offerings live (closes #108) #569 added SavingsPlansProber to DefaultProbers, making that comment incorrect.
  • Adds four Validate tests covering SP service keys (savings-plans-compute, -ec2instance, -sagemaker, -database) to close the coverage gap that the chunk-0 bucket-A verification flagged.

Precedence chain

probe snapshot present for service key -> reject invalid combos
service key absent from snapshot -> permissive fallback (plan type not offered in region)
ErrNoData (cold store / no AWS account) -> permissive fallback (never block saves)

What PR #569 left incomplete vs claimed

The probe code and DefaultProbers wiring were complete. The gap was:

  1. A stale comment in Validate still implied SP was unprobed.
  2. No tests exercised the Validate path with the real SP service keys, so the probe-first behavior was unverified.

The Validate logic itself was already correct post #569 -- Get routes through probeAndPersist which fans out all DefaultProbers including SavingsPlansProber. This PR closes the documentation and test coverage gap.

Test plan

  • go test github.com/LeanerCloud/CUDly/internal/commitmentopts/... -- 83 pass (79 pre-existing + 4 new)
  • go vet ./internal/commitmentopts/... -- clean

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced error handling to properly handle wrapped errors during validation, improving system resilience.
  • Tests

    • Expanded test coverage for savings plans validation behavior and edge cases.

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/many Affects most users priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/feat New capability urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant