feat(commitment-options): probe AWS reserved-offerings live & validate saves#54
Merged
cristim merged 2 commits intoApr 25, 2026
Merged
Conversation
…ate saves
Replaces the hardcoded per-service (term × payment) rules in
frontend/src/commitmentOptions.ts with data harvested from the AWS
reserved-offerings APIs. The probe runs at most once per server lifetime
(persisted in new tables commitment_options_probe_runs +
commitment_options_combos); if the DB already has data, subsequent
Get()s short-circuit on the cache. Any failure along the chain — no
AWS account connected, probe denied, unknown payment shape — collapses
to ErrNoData and the frontend falls back to its hardcoded rules, so
this is wired unconditionally.
Backend:
- internal/commitmentopts/: new package with per-service Probers
(rds, elasticache, opensearch, redshift, memorydb, ec2), shared
duration+payment normalizers (tokenizes inputs to handle enum
stringers and string OfferingTypes, rejects pre-2014 legacy
utilization values), Postgres-backed Store with ON CONFLICT DO
NOTHING for cross-process races, and an errgroup-driven Service
that enforces all-or-nothing persistence — one probe failure
aborts the whole run so callers never see half-populated cache.
- GET /api/commitment-options returns {status:"ok", aws:{...}} with
term/payment tuples, or {status:"unavailable"} (HTTP 200) when
probe data is missing so the frontend's error-toast path stays
quiet.
- updateServiceConfig (PUT /api/config/service/{id}) now calls
Validate() via a new checkCommitmentOptionCombo helper before
persisting. Rejects invalid (aws, service, term, payment) combos
with 400; permissive-true when probe data is absent so the
frontend's hardcoded rules remain the gate.
- Handler wiring threads credStore + awsCfg through a BuildConfigFn
that calls credentials.ResolveAWSCredentialProvider for the first
enabled AWS account and builds a regional aws.Config for probes.
Frontend:
- fetchAndPopulateCommitmentOptions() fires from loadGlobalSettings
BEFORE syncAllServiceCommitmentConstraints(), overlays server
data onto commitmentConfigs.aws (diffing supported combos against
the full terms × payments product), and silently no-ops on
network/parse/unavailable.
DB migration 000039 adds the two tables. Safe to drop and re-add
(ON CONFLICT-tolerant; no external references).
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…y-error, stable frontend overlay Follow-up to f315a03. Two parallel review agents surfaced six concrete gaps against PR #54; this commit closes the material ones. - service.go: probe errors are no longer silently discarded into ErrNoData. Both the buildConfig failure and the errgroup.Wait() error are logged with account ID + the originating probe name so an operator can tell "no AWS account connected" from "IAM permission missing on opensearch:DescribeReservedInstanceOfferings". - handler_commitment_options.go: the endpoint now returns {status:"unavailable"} (HTTP 200) for ANY error from the service — not just ErrNoData. A transient DB blip or context cancellation no longer 500s the Settings overlay fetch. Non-ErrNoData errors are logged so telemetry is preserved. The existing "UnexpectedError_Propagates" test is rewritten to assert the new collapse-to-unavailable contract. - commitmentOptions.ts: the overlay now diffs against the canonical STANDARD_TERMS × AWS_PAYMENTS product instead of the (possibly already-narrowed) existing entry. Previously, if the server widened its supported set between two Settings visits, the second overlay computed the intersection against the stale narrow base and the UI stayed restrictive. New test re-widens after a narrow overlay to regress-guard. - openapi.yaml: /api/commitment-options is documented with its CommitmentOptionsResponse + CommitmentOptionCombo schemas, matching the convention used for every other route. - app.go: a short comment explains why us-east-1 is hardcoded (reserved offerings are global facts; widest coverage; GovCloud / China fall back to hardcoded rules via ErrNoData). - handler_commitment_options_test.go: new TestNewHandler_CommitmentOptsWired asserts that HandlerConfig.CommitmentOpts actually lands on the Handler — catches a future silent field-rename regression. Three out-of-scope items (router-level AuthUser enforcement, MemoryDB probe target verification, page-cap tests for the other 5 probers) tracked as separate follow-up GitHub issues.
This was referenced Apr 24, 2026
8 tasks
cristim
added a commit
that referenced
this pull request
Apr 25, 2026
…test coverage (#69) Combined follow-up to PR #54: addresses two issues surfaced during review that both touch internal/commitmentopts/. #62 — extract pagination loop into a shared walkPaginated helper The 6 per-service probers (RDS, ElastiCache, OpenSearch, Redshift, MemoryDB, EC2) all ran the same paginated-Describe loop body: page 0..N, fetch, accumulate, check next-page token, break/loop, with a hard maxPages=5 cap. The cap was previously exercised by a single TestRDSProber_PageCap test, leaving the other five probers' cap behaviour silently unverified — a refactor that broke one prober's loop in isolation could regress the cap without any test failing. Extracted the shared loop into walkPaginated(ctx, service, fetchPage) where fetchPage is a per-prober closure that handles each AWS API's specific quirks (Marker vs NextToken field name, per-item shape conversion, optional client-side instance-type filter for OpenSearch and Redshift). Error wrapping ("<service>: <err>") moves to the helper too. Chose extraction over mirroring TestRDSProber_PageCap five times because the loops were genuinely identical — only the per-item conversion differed, which is exactly what the closure now isolates. The new TestWalkPaginated_* tests cover page-cap, nil-token termination, empty-string-token termination, token threading across pages, error wrapping, and item accumulation in a single place. Per-prober Probe tests still exercise end-to-end wiring (which token field, per-item conversion, filters) so a wiring break in one prober is still caught. TestRDSProber_PageCap is retained as an integration smoke test. Closes #62. #61 — MemoryDB probe target db.t4g.small empirical risk Issue flagged that MemoryDB reserved-node coverage has historically skewed to db.r6g.* tiers, so db.t4g.small may return an empty offerings list in some regions, causing the cache to persist a probe run with zero MemoryDB combos. Resolution path was either: verify with AWS creds and switch to db.r6g.large if sparse, OR document the risk if no creds are available. This automated session had no AWS credentials (sts get-caller-identity NoCredentials), so the probe target stays db.t4g.small. Added a code comment in probe.go documenting the empirical risk, the verification command, and the safe fallback (db.r6g.large) so a future engineer with creds can finish the verification without re-discovering the context. A follow-up comment will be posted on issue #61 noting that AWS verification is still needed. Closes #61. Local verification: go build ./..., go vet ./..., go test ./... (all pass; commitmentopts package: ok 3.666s).
This was referenced Apr 27, 2026
cristim
added
triaged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
(term, payment)combos to two new Postgres tables; probe runs at most once across server restarts.GET /api/commitment-optionsexposes the live data; frontend overlays it ontocommitmentOptions.tsat Settings load, silently falling back to hardcoded rules on any failure.PUT /api/config/service/{id}rejects term/payment combos the cloud doesn't sell (belt-and-braces against the frontend gate).Design
ErrNoData→ frontend fallback. Never persists a partial cache.CloudAccount; no AWS connected meansErrNoData.ON CONFLICT DO NOTHINGon both new tables tolerates two Lambda instances racing the first probe.Validate()returns true when no probe data exists so saves aren't blocked on a fresh install.DescribeSavingsPlansOfferings), no current restrictions, stays hardcoded.Test plan
go build ./...+go test ./...green across root module andpkg/(22 packages pass).ErrNoData+ unexpected-error + empty-AWS-collapses-to-unavailable paths.jestfull suite (1247 tests) green; newfetchAndPopulateCommitmentOptionsdescribe block covers overlay, unavailable, 5xx, network error, all-combos-supported.tsc --noEmitclean;npm run buildproduces a working bundle./api/commitment-optionsfrom the preview environment and confirm the Settings page reflects the server response (pending CI + deploy).Review-follow-up fixes (second commit)
Commit
1dcbef7addresses gaps surfaced by a pre-coderabbit self-review:service.go: probe errors now logged with account ID + originating probe name before collapsing toErrNoData, so operators can diagnose IAM-permission issues without the cache silently returning "unavailable".handler_commitment_options.go: any error (not justErrNoData) now collapses to{"status":"unavailable"}at HTTP 200, matching the comment's original contract; non-ErrNoDataerrors still logged for telemetry.commitmentOptions.ts: overlay now diffs against the canonicalSTANDARD_TERMS × AWS_PAYMENTSproduct (not the possibly-already-narrowed existing entry), so re-entering Settings after the server widens its supported set no longer leaves the UI stuck on the stale narrow intersection. New re-widening regression test.openapi.yaml:/api/commitment-optionsdocumented withCommitmentOptionsResponse+CommitmentOptionComboschemas.app.go: comment explains whyus-east-1is hardcoded (reserved offerings are global; widest coverage; GovCloud/China fall back viaErrNoData).handler_commitment_options_test.go: newTestNewHandler_CommitmentOptsWiredregression guard.Follow-up issues (out of this PR's scope)
security: Router.Route ignores AuthUser(pre-existing, affects everyAuthUserroute)commitmentopts: confirm MemoryDB probe target db.t4g.small has reserved-node offerings(needs live AWS data)commitmentopts: add page-cap unit tests for ElastiCache/OpenSearch/Redshift/MemoryDB/EC2 probers🤖 Generated with claude-flow