Repository navigation
Commit bc4d3ae
authored
* feat(settings): configurable EC2 RI OfferingClass (closes #694)
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
* fix(settings): validate OfferingClass on PUT + assert it reaches the 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.
* refactor(config): extract validateScheduleAndNotifications to reduce 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.
* fix(test): avoid mutex copy in TestFindOfferingID_OfferingClassReachesSDKCall
Construct capturingMockEC2Client directly instead of copying *MockEC2Client
to silence the go vet copylocks warning (MockEC2Client embeds mock.Mock
which contains sync.Mutex).
* fix(purchase): fail EC2 RI recs on GlobalConfig load error instead of 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.
* refactor(purchase): extract applyAccountOutcome to drop executeForAccount 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.
* fix(test): update SQL parameter positions after laddering_enabled shift
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.
* style(purchase,config): clear new-from-rev lint on OfferingClass diff
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.
* chore(migration): renumber ec2_ri_offering_class from 000078 to 000083
Migration 000078 collided with PR #828. Main's highest is 000081;
#1277 reserves 000082; 000083 is the next free slot for this PR.
Renamed via git mv (up + down); no Go code referenced the number directly.
* chore(migration): renumber ec2_ri_offering_class from 000083 to 000090
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.
* fix(rebase): restore saveExecutionStatusBestEffort; update param-23 guard 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.
1 parent 1fca7fd commit bc4d3ae
19 files changed
Lines changed: 579 additions & 55 deletions
File tree
- frontend/src
- __tests__
- api
- internal
- config
- database/postgres/migrations
- purchase
- pkg/common
- providers/aws/services/ec2
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
539 | 539 | | |
540 | 540 | | |
541 | 541 | | |
542 | | - | |
| 542 | + | |
543 | 543 | | |
544 | 544 | | |
545 | 545 | | |
| 546 | + | |
| 547 | + | |
| 548 | + | |
546 | 549 | | |
547 | 550 | | |
548 | 551 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
289 | 289 | | |
290 | 290 | | |
291 | 291 | | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
292 | 296 | | |
293 | 297 | | |
294 | 298 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
497 | 497 | | |
498 | 498 | | |
499 | 499 | | |
| 500 | + | |
| 501 | + | |
| 502 | + | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
| 514 | + | |
| 515 | + | |
500 | 516 | | |
501 | 517 | | |
502 | 518 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3202 | 3202 | | |
3203 | 3203 | | |
3204 | 3204 | | |
| 3205 | + | |
| 3206 | + | |
| 3207 | + | |
| 3208 | + | |
| 3209 | + | |
| 3210 | + | |
| 3211 | + | |
| 3212 | + | |
| 3213 | + | |
| 3214 | + | |
3205 | 3215 | | |
3206 | 3216 | | |
3207 | 3217 | | |
| |||
3476 | 3486 | | |
3477 | 3487 | | |
3478 | 3488 | | |
| 3489 | + | |
| 3490 | + | |
| 3491 | + | |
3479 | 3492 | | |
3480 | 3493 | | |
3481 | 3494 | | |
| |||
3488 | 3501 | | |
3489 | 3502 | | |
3490 | 3503 | | |
| 3504 | + | |
3491 | 3505 | | |
3492 | 3506 | | |
3493 | 3507 | | |
| |||
3625 | 3639 | | |
3626 | 3640 | | |
3627 | 3641 | | |
| 3642 | + | |
| 3643 | + | |
| 3644 | + | |
3628 | 3645 | | |
3629 | 3646 | | |
3630 | 3647 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
373 | 373 | | |
374 | 374 | | |
375 | 375 | | |
| 376 | + | |
| 377 | + | |
376 | 378 | | |
377 | 379 | | |
378 | 380 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
81 | 81 | | |
82 | 82 | | |
83 | 83 | | |
84 | | - | |
| 84 | + | |
| 85 | + | |
85 | 86 | | |
86 | 87 | | |
87 | 88 | | |
| |||
113 | 114 | | |
114 | 115 | | |
115 | 116 | | |
| 117 | + | |
116 | 118 | | |
117 | 119 | | |
118 | 120 | | |
| |||
135 | 137 | | |
136 | 138 | | |
137 | 139 | | |
| 140 | + | |
138 | 141 | | |
139 | 142 | | |
140 | 143 | | |
| |||
211 | 214 | | |
212 | 215 | | |
213 | 216 | | |
214 | | - | |
215 | | - | |
| 217 | + | |
| 218 | + | |
216 | 219 | | |
217 | 220 | | |
218 | 221 | | |
| |||
236 | 239 | | |
237 | 240 | | |
238 | 241 | | |
| 242 | + | |
239 | 243 | | |
240 | 244 | | |
241 | 245 | | |
| |||
259 | 263 | | |
260 | 264 | | |
261 | 265 | | |
262 | | - | |
| 266 | + | |
263 | 267 | | |
264 | 268 | | |
265 | 269 | | |
| |||
271 | 275 | | |
272 | 276 | | |
273 | 277 | | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
274 | 286 | | |
275 | 287 | | |
276 | 288 | | |
| |||
294 | 306 | | |
295 | 307 | | |
296 | 308 | | |
| 309 | + | |
297 | 310 | | |
298 | 311 | | |
299 | 312 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
| 8 | + | |
8 | 9 | | |
| 10 | + | |
9 | 11 | | |
10 | 12 | | |
11 | 13 | | |
| |||
535 | 537 | | |
536 | 538 | | |
537 | 539 | | |
| 540 | + | |
| 541 | + | |
| 542 | + | |
| 543 | + | |
| 544 | + | |
| 545 | + | |
| 546 | + | |
| 547 | + | |
| 548 | + | |
| 549 | + | |
| 550 | + | |
| 551 | + | |
| 552 | + | |
| 553 | + | |
| 554 | + | |
| 555 | + | |
| 556 | + | |
| 557 | + | |
| 558 | + | |
| 559 | + | |
| 560 | + | |
| 561 | + | |
| 562 | + | |
| 563 | + | |
| 564 | + | |
| 565 | + | |
| 566 | + | |
| 567 | + | |
| 568 | + | |
| 569 | + | |
| 570 | + | |
| 571 | + | |
| 572 | + | |
| 573 | + | |
| 574 | + | |
| 575 | + | |
| 576 | + | |
| 577 | + | |
| 578 | + | |
| 579 | + | |
| 580 | + | |
| 581 | + | |
| 582 | + | |
| 583 | + | |
| 584 | + | |
| 585 | + | |
| 586 | + | |
| 587 | + | |
| 588 | + | |
| 589 | + | |
| 590 | + | |
| 591 | + | |
| 592 | + | |
| 593 | + | |
| 594 | + | |
| 595 | + | |
| 596 | + | |
| 597 | + | |
| 598 | + | |
| 599 | + | |
| 600 | + | |
| 601 | + | |
| 602 | + | |
| 603 | + | |
| 604 | + | |
| 605 | + | |
| 606 | + | |
| 607 | + | |
| 608 | + | |
| 609 | + | |
| 610 | + | |
| 611 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
61 | 61 | | |
62 | 62 | | |
63 | 63 | | |
| 64 | + | |
64 | 65 | | |
65 | 66 | | |
66 | 67 | | |
| |||
73 | 74 | | |
74 | 75 | | |
75 | 76 | | |
| 77 | + | |
76 | 78 | | |
77 | 79 | | |
78 | 80 | | |
| |||
83 | 85 | | |
84 | 86 | | |
85 | 87 | | |
| 88 | + | |
86 | 89 | | |
87 | 90 | | |
88 | 91 | | |
| |||
115 | 118 | | |
116 | 119 | | |
117 | 120 | | |
| 121 | + | |
118 | 122 | | |
119 | 123 | | |
120 | 124 | | |
| |||
128 | 132 | | |
129 | 133 | | |
130 | 134 | | |
| 135 | + | |
131 | 136 | | |
132 | 137 | | |
133 | 138 | | |
| |||
186 | 191 | | |
187 | 192 | | |
188 | 193 | | |
| 194 | + | |
189 | 195 | | |
190 | 196 | | |
191 | 197 | | |
| |||
213 | 219 | | |
214 | 220 | | |
215 | 221 | | |
216 | | - | |
217 | | - | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
218 | 225 | | |
219 | 226 | | |
220 | 227 | | |
| |||
223 | 230 | | |
224 | 231 | | |
225 | 232 | | |
226 | | - | |
| 233 | + | |
227 | 234 | | |
228 | 235 | | |
229 | 236 | | |
| |||
270 | 277 | | |
271 | 278 | | |
272 | 279 | | |
| 280 | + | |
273 | 281 | | |
274 | 282 | | |
275 | 283 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
50 | 50 | | |
51 | 51 | | |
52 | 52 | | |
53 | | - | |
| 53 | + | |
54 | 54 | | |
55 | 55 | | |
56 | 56 | | |
| |||
78 | 78 | | |
79 | 79 | | |
80 | 80 | | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
81 | 89 | | |
82 | 90 | | |
83 | 91 | | |
| |||
0 commit comments