Skip to content

chore(commitmentopts): MemoryDB probe target verification + page-cap test coverage#69

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
chore/commitmentopts-probe-followups
Apr 25, 2026
Merged

chore(commitmentopts): MemoryDB probe target verification + page-cap test coverage#69
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
chore/commitmentopts-probe-followups

Conversation

@cristim
Copy link
Copy Markdown
Member

@cristim cristim commented Apr 25, 2026

Summary

Combined follow-up to PR #54 addressing two issues that both touch internal/commitmentopts/. Bundled into one atomic PR per the issue surface area + bot economy guideline.

#62 — page-cap test coverage via shared helper extraction

The 6 per-service probers (RDS, ElastiCache, OpenSearch, Redshift, MemoryDB, EC2) ran the same paginated-Describe loop with a hard maxPages=5 cap, but only RDS had a page-cap test. A refactor that broke one prober's loop in isolation could regress the cap silently.

Extracted the 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, per-item shape conversion, optional client-side filter for OpenSearch and Redshift). Error wrapping (<service>: <err>) moves to the helper too.

Chose helper extraction over mirroring TestRDSProber_PageCap 5 times because the loops were genuinely identical — only per-item conversion differed, which is what the closure now isolates. New TestWalkPaginated_* covers: page-cap, nil-token termination, empty-string-token termination, token threading, error wrapping, item accumulation. Per-prober Probe tests still exercise end-to-end wiring (which token field, filters) so per-prober wiring breaks remain caught. TestRDSProber_PageCap retained as integration smoke.

Closes #62.

#61 — MemoryDB probe target empirical risk

Issue flagged that db.t4g.small may return empty offerings in some regions because MemoryDB RI coverage skews to db.r6g.*. Resolution path was: verify with AWS creds and switch to db.r6g.large if sparse, OR document the risk if no creds.

This automated session had no AWS credentials (sts get-caller-identity returned 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 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 AWS verification is still needed.

Closes #61.

Test plan

  • go build ./...
  • go vet ./...
  • go test ./internal/commitmentopts/... — passes (ok 3.666s)
  • go test ./... — full suite passes
  • All 6 probers' _Probe tests still pass (validates per-prober wiring, including OpenSearch/Redshift client-side filters and EC2 IncludeMarketplace)
  • TestRDSProber_PageCap still passes (integration smoke for Marker-based wiring)
  • New TestWalkPaginated_* covers cap + termination + threading + error wrap + accumulation
  • AWS verification of MemoryDB db.t4g.small offerings remains open in commitmentopts: confirm MemoryDB probe target db.t4g.small has reserved-node offerings #61 (no creds available to this session)

Out of scope

  • Other commitmentopts behaviour changes
  • Frontend changes

…test coverage

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).
@cristim
Copy link
Copy Markdown
Member Author

cristim commented Apr 25, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 25, 2026

Warning

Rate limit exceeded

@cristim has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 34 minutes and 48 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 34 minutes and 48 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b787188a-8045-48d9-aa6d-c15dd21f7f48

📥 Commits

Reviewing files that changed from the base of the PR and between c8f58e9 and 688db43.

📒 Files selected for processing (2)
  • internal/commitmentopts/probe.go
  • internal/commitmentopts/probe_test.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/commitmentopts-probe-followups

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.

❤️ Share

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

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 25, 2026

✅ Actions performed

Review triggered.

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
Copy link
Copy Markdown
Member Author

cristim commented Apr 25, 2026

Note for triage clarity — this PR's effect on the two issues it referenced:

Resolves #62. The page-cap concern was "five non-RDS probers lack equivalent coverage; if any of them silently lost the cap on a refactor, no test would catch it". The shared walkPaginated helper here means all six probers now go through one loop with one cap, exercised by TestWalkPaginated_StopsAtPageCap. The issue body's own preferred path ("extract the pagination loop into a shared helper and test it once") is exactly what landed. Closing #62.

Does NOT resolve #61. The MemoryDB probe target is still db.t4g.small. This PR only added a comment on probeTargetMemoryDB documenting the empirical risk and the verification command (aws memorydb describe-reserved-nodes-offerings --region us-east-1 --node-type db.t4g.small). The actual confirm-or-flip-to-db.r6g.large step still requires a human with AWS creds. #61 stays open.

@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/this-sprint Within the current sprint impact/internal Team-internal only effort/m Days type/chore Maintenance / non-user-visible labels Apr 28, 2026
@cristim cristim deleted the chore/commitmentopts-probe-followups branch April 29, 2026 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/m Days impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant