feat(settings): configurable EC2 RI OfferingClass (closes #694) - #847
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds EC2 Reserved Instance offering-class selection across the admin UI, typed configuration, PostgreSQL persistence, validation, purchase execution, and AWS offering lookup. Convertible remains the default; standard is accepted as an alternative. ChangesOffering Class Feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant SettingsAPI
participant ConfigStore
participant PurchaseExecution
participant EC2Client
Admin->>SettingsAPI: save offering_class
SettingsAPI->>ConfigStore: persist GlobalConfig
PurchaseExecution->>ConfigStore: load GlobalConfig
ConfigStore-->>PurchaseExecution: OfferingClass
PurchaseExecution->>EC2Client: pass PurchaseOptions.OfferingClass
EC2Client->>EC2Client: resolve offering class
EC2Client-->>PurchaseExecution: DescribeReservedInstancesOfferings result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
pr-iterate: CR appears to be blocked by org-level usage limits ( |
|
@coderabbitai review |
Rate Limit Exceeded
|
|
Rebased on feat/multicloud-web-frontend. Resolved additive conflict in providers/aws/services/ec2/client_test.go (kept both HEAD's Name-tag tests and PR's OfferingClass tests). @coderabbitai review |
|
🧠 Learnings used✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql (1)
5-6: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueOptional: consider a CHECK constraint for defense-in-depth.
The column currently has no DB-level constraint on allowed values, relying entirely on app-layer validation (
validateOfferingClass). A directUPDATE global_config SET offering_class = 'foo'via psql would silently persist an invalid value that only surfaces as an error later at purchase time (perresolveOfferingClassType).🛡️ Optional CHECK constraint
ALTER TABLE global_config ADD COLUMN IF NOT EXISTS offering_class TEXT NOT NULL DEFAULT 'convertible'; + +ALTER TABLE global_config + ADD CONSTRAINT global_config_offering_class_check + CHECK (offering_class IN ('convertible', 'standard'));Skip if the team intends to keep the value set open-ended without a migration for future classes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql` around lines 5 - 6, Add a DB-level CHECK constraint for global_config.offering_class in this migration so only the supported offering class values can be stored, instead of relying solely on validateOfferingClass and resolveOfferingClassType. Update the migration that alters global_config to include the constraint alongside the existing offering_class column definition, using the same allowed values enforced by the app.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sql`:
- Around line 5-6: Add a DB-level CHECK constraint for
global_config.offering_class in this migration so only the supported offering
class values can be stored, instead of relying solely on validateOfferingClass
and resolveOfferingClassType. Update the migration that alters global_config to
include the constraint alongside the existing offering_class column definition,
using the same allowed values enforced by the app.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b979982a-e00d-4265-bddd-753143d78382
📒 Files selected for processing (19)
frontend/src/__tests__/settings.test.tsfrontend/src/api/types.tsfrontend/src/index.htmlfrontend/src/settings.tsfrontend/src/types.tsinternal/config/store_postgres.gointernal/config/store_postgres_coverage_test.gointernal/config/store_postgres_pgxmock_test.gointernal/config/types.gointernal/config/validation.gointernal/config/validation_test.gointernal/database/postgres/migrations/000078_ec2_ri_offering_class.down.sqlinternal/database/postgres/migrations/000078_ec2_ri_offering_class.up.sqlinternal/purchase/execution.gointernal/purchase/execution_test.gointernal/purchase/money_path_regression_test.gopkg/common/types.goproviders/aws/services/ec2/client.goproviders/aws/services/ec2/client_test.go
💤 Files with no reviewable changes (2)
- providers/aws/services/ec2/client.go
- providers/aws/services/ec2/client_test.go
✅ Files skipped from review due to trivial changes (2)
- frontend/src/types.ts
- frontend/src/index.html
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/purchase/money_path_regression_test.go
- internal/config/validation_test.go
- internal/purchase/execution_test.go
- internal/config/types.go
- pkg/common/types.go
- frontend/src/settings.ts
- internal/config/validation.go
- frontend/src/api/types.ts
- frontend/src/tests/settings.test.ts
- internal/config/store_postgres.go
- internal/purchase/execution.go
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Add a GlobalConfig.OfferingClass field ("convertible" | "standard") that
controls which EC2 Reserved Instance offering class is purchased. Empty /
absent values default to "convertible" to preserve pre-694 behaviour.
Backend:
- pkg/common/types.go: OfferingClass field on PurchaseOptions
- internal/config/types.go: OfferingClass on GlobalConfig
- internal/config/store_postgres.go: read/write offering_class column ($20)
- internal/purchase/execution.go: load GlobalConfig and propagate
OfferingClass into PurchaseOptions before the fan-out
- providers/aws/services/ec2/client.go: resolveOfferingClassType() fails
loudly on unknown values (feedback_empty_string_vs_error.md); empty
string maps to convertible so the DB default is safe
- internal/database/postgres/migrations/000064_ec2_ri_offering_class: ADD
COLUMN offering_class TEXT NOT NULL DEFAULT 'convertible'
Frontend:
- frontend/src/index.html: fieldset with <select> (Convertible / Standard)
- frontend/src/settings.ts: load, save, and reset handlers
- frontend/src/api/types.ts + types.ts: offering_class optional field
Tests:
- TestResolveOfferingClassType: unknown values error, "" = convertible
- TestDescribeInputFromQuery_OfferingClass: empty / explicit convertible / standard
- All findOfferingID call sites updated to 4-arg signature
- store_postgres_pgxmock_test.go: +offering_class column in mock rows
- settings.test.ts: offering_class: 'convertible' in saveGlobalSettings
expected payload
…EC2 SDK call (refs #694) - Add validateOfferingClass() to GlobalConfig.Validate() so an invalid offering_class is rejected at PUT time with a clear error rather than silently persisting and only failing at purchase time. Accepts "" | "convertible" | "standard" (case-sensitive, matching resolveOfferingClassType). - Add ValidOfferingClasses exported var for reference/documentation. - Add six table-driven test cases in TestGlobalConfig_Validate covering empty (valid), both valid values, wrong-case "Convertible", "STANDARD", and an unknown value. - Add capturingMockEC2Client embedding MockEC2Client that records the last DescribeReservedInstancesOfferingsInput to enable SDK-level assertions. - Add TestFindOfferingID_OfferingClassReachesSDKCall: three subtests ("", "convertible", "standard") each calling findOfferingID and asserting that the captured OfferingClass on the outbound SDK call equals the expected types.OfferingClassType. This test fails if the wiring from offeringClassStr through resolveOfferingClassType to describeInputFromQuery regresses.
…cyclomatic complexity Validate had complexity 11 after adding the OfferingClass check in #694. Extract the CollectionSchedule + NotificationDaysBefore + GracePeriodDays checks into validateScheduleAndNotifications, keeping Validate under 10.
…sSDKCall Construct capturingMockEC2Client directly instead of copying *MockEC2Client to silence the go vet copylocks warning (MockEC2Client embeds mock.Mock which contains sync.Mutex).
… defaulting OfferingClass When GetGlobalConfig returns an error, the old code logged it and continued with an empty OfferingClass, which resolveOfferingClassType maps to "convertible". This caused a transient DB failure to silently buy the wrong (and more expensive, irreversible) RI class instead of the operator-configured "standard". Violates the no-silent-fallbacks-on-money-paths rule. Fix: change processPurchaseRecommendations to return (float64, float64, []string, error) and propagate a hard error on GetGlobalConfig failure. Both call sites (executeSingleAccount and executeForAccount) check the new error return and abort without making any cloud purchase. Regression test (HOLE 1): TestProcessPurchaseRecommendations_GlobalConfigError_FailsInsteadOfDefaulting confirms the function returns an error and never calls the provider factory when GetGlobalConfig fails. Verified red on pre-fix code, green after. Also add TestSaveGlobalConfig_OfferingClassBindsAt21 (HOLE 2): a pgxmock test against the real PostgresStore.SaveGlobalConfig verifying offering_class binds as the 21st positional arg. The hand-maintained testablePostgresStore in store_postgres_mock_test.go omits this field entirely, so no fast test previously guarded the 21-placeholder write query.
…ount below gocyclo 10 The gocyclo pre-commit hook (-over 10) failed: executeForAccount was at cyclomatic complexity 11. Extract the per-account status-resolution switch (partially_completed / failed / completed stamping, #642) and the committed gate (#1014) into a cohesive applyAccountOutcome helper, dropping the function below the threshold without a //nolint or a threshold bump. Behavior is unchanged: same status transitions, same error-note appending, same committed semantics (anyRecPurchased is now evaluated once and reused for the partial flag). The OfferingClass typed-enum validation and no-silent-fallback behavior are untouched.
Rebasing onto main added laddering_enabled as $21 in global_config INSERT, shifting offering_class from $21 to $22. Update the pgxmock and coverage tests to reflect the new column count and binding positions.
Resolve the three golangci-lint findings that --new-from-rev=origin/main attributes to the #694 OfferingClass changes: - errcheck: replace the silent `_ =` discard of SavePurchaseExecution on the processPurchaseRecommendations error path with the existing saveExecutionStatusBestEffort helper, which persists and logs the audit-save failure instead of dropping it. - gocritic unnamedResult: name the four results of processPurchaseRecommendations (its return set grew to include error); switch the trailing assignment from := to = accordingly. - misspell: pre-694 behaviour -> behavior in the ValidOfferingClasses doc. No behavior change to the purchase path; base-debt Lint/Security failures are pre-existing on main and out of scope.
Migration 000083 is now taken by ladder_execution_enabled (merged to main while this PR was in review). Next free slot after 000089_rename_cancelled_to_canceled is 000090.
…uard test Two issues introduced when rebasing onto main (which added ladder_execution_enabled in parallel): 1. saveExecutionStatusBestEffort helper was dropped during conflict resolution of the applyAccountOutcome refactor commit; restored from the pre-rebase branch. 2. TestSaveGlobalConfig_OfferingClassBindsAt22 expected 22 args; with ladder_execution_enabled now at $22, offering_class moved to $23. Renamed the test to TestSaveGlobalConfig_OfferingClassBindsAt23 and added the $22 AnyArg placeholder.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
GlobalConfig.OfferingClasssetting ("convertible" | "standard") that controls which EC2 Reserved Instance class is purchased; empty / absent values default to "convertible" preserving pre-694 behaviouroffering_class TEXT NOT NULL DEFAULT 'convertible'toglobal_config<select>values hardcoded in HTML so no innerHTML XSS surfaceresolveOfferingClassTypereturns an explicit error) perfeedback_empty_string_vs_error.mdTest plan
go build ./...cleango test github.com/LeanerCloud/CUDly/providers/aws/services/ec2/... github.com/LeanerCloud/CUDly/internal/config/...-- 601 passednpx tsc --noEmit-- no errorsnpx jest-- 2142 passed, 0 failedresolveOfferingClassType("")returnsOfferingClassTypeConvertible, nilresolveOfferingClassType("unknown")returns"", errorsaveGlobalSettingstest updated to assertoffering_class: 'convertible'in payloadSummary by CodeRabbit
New Features
Bug Fixes
Tests