Skip to content

fix(aws/savingsplans): error on a plan with no start or end date - #620

Merged
cristim merged 1 commit into
mainfrom
fix/53-sp-nil-start
Oct 25, 2026
Merged

cristim merged 1 commit into
mainfrom
fix/53-sp-nil-start

Conversation

@cristim

@cristim cristim commented Oct 25, 2026 •

Copy link
Copy Markdown
Member

Refs #53.

toCommitment left StartDate/EndDate at the zero time when Start or End was nil. dedupe.go never treats a zero StartDate as recent, so a plan with a nil Start could let a duplicate purchase through. A nil Start or End now returns an error naming the plan ID, matching the unparseable-date error from #619.

The SDK documents Start and End only as "The start time" / "The end time" (savingsplans@v1.31.0/types/types.go:49,80), with no state where they are legitimately absent. Existing test fixtures that omitted dates now set them.

Tests: new TestClient_GetExistingCommitments_MissingDateNamesPlan fails without the fix (mutation probe: reverting client.go fails both subtests). go test -race -count=1 ./... passes in providers/aws with GOWORK=off and with the workspace; golangci-lint v2.10.1 reports 0 issues.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • AWS Savings Plans with a missing start or end date now return a clear error identifying the plan and the missing date, rather than being accepted with incomplete date information.
    • Improved validation helps prevent plans with missing dates from being treated as existing commitments during purchase deduplication.

GetExistingCommitments left StartDate/EndDate at the zero time when the
API returned a nil Start or End. The purchase dedupe never treats a zero
StartDate as recent, so a plan with a nil Start could let a duplicate
purchase through. A missing date now fails with an error naming the plan
ID, like an unparseable one. The SDK documents both fields only as "The
start time" / "The end time" (savingsplans@v1.31.0 types/types.go:49,80)
with no absent-state exception.

Refs #53

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 75f4d27 and cf56b05.

Files selected for processing: 3
  • CHANGELOG.md
  • providers/aws/services/savingsplans/client.go
  • providers/aws/services/savingsplans/client_test.go

Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour.

Walkthrough

toCommitment now returns an error when a Savings Plan lacks a start or end date. The error identifies the plan and missing date. Tests update existing fixtures and cover missing dates.

Changes: Savings Plans date validation

Layer / File(s) Summary
Reject missing plan dates
providers/aws/services/savingsplans/client.go, providers/aws/services/savingsplans/client_test.go, CHANGELOG.md
toCommitment now rejects missing start and end dates. Tests cover missing dates and provide valid dates in existing fixtures. The changelog documents the error behavior.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related PRs

Merge Risk: Minimal · up to cf56b

The date validation change has no identified merge-blocking issue and is ready for normal checks.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title names the primary change: Savings Plans toCommitment now returns an error when a plan has no start or end date. The changeset matches this. The title is specific and follows the conventional…
Docstring Coverage Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 u…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the primary change: Savings Plans toCommitment now returns an error when a plan has no start or end date. The changeset matches this. The title is specific and follows the conventional-commit style. It does not cover the test additions, but that is expected.

Full details: Docstring Coverage

Explanation

Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.)

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim
cristim merged commit d1c5dbb into main Oct 25, 2026
15 checks passed
@cristim
cristim deleted the fix/53-sp-nil-start branch October 25, 2026 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant