Skip to content

fix(config): accept provider-canonical payment options in plan validator - #709

Merged
cristim merged 7 commits into
feat/multicloud-web-frontendfrom
fix/698-plan-validator-non-aws-payments
May 25, 2026
Merged

fix(config): accept provider-canonical payment options in plan validator#709
cristim merged 7 commits into
feat/multicloud-web-frontendfrom
fix/698-plan-validator-non-aws-payments

Conversation

@cristim

@cristim cristim commented May 25, 2026

Copy link
Copy Markdown
Member

Closes #698 — unblocks non-AWS plan creation.

The plan validator previously rejected any non-AWS payment option with invalid payment option: monthly (valid: no-upfront, partial-upfront, all-upfront) because ValidPaymentOptions was a single AWS-only slice. After PR #682 the Azure recommendation emitters now stamp upfront/monthly (Azure-canonical, accepted by all five Azure service-client switches), but the plan validator was missed in that PR's scope.

What changed

  • Added ValidPaymentOptionsByProvider map in internal/config/validation.go:
    • aws: no-upfront, partial-upfront, all-upfront
    • azure: all-upfront, no-upfront, upfront, monthly (cross-verified against all five azure service-client switch statements)
    • gcp: all-upfront, no-upfront, upfront, monthly (GCP service clients stamp monthly as default; switches treat monthly/no-upfront and all-upfront/upfront as synonyms)
  • Added validPaymentOptionsFor(provider) lookup; ServiceConfig.validatePayment now uses it and includes the provider in the error message: invalid payment option: X (valid for azure: ...) — so a cross-provider token (e.g. all-upfront on an azure service) is rejected loudly.
  • GlobalConfig.DefaultPayment is provider-less; validates against the union of all provider sets.
  • Kept the original ValidPaymentOptions slice for backwards compatibility.

Tests

  • 11 new validation_test.go cases covering each provider × payment combination including cross-provider rejection.
  • Existing internal/api/handler_purchases_guards_test.go (azure, monthly already treated as valid in that layer) now agrees with the plan validator.
  • 513/513 internal/config tests pass, 1231/1231 internal/api tests pass, go vet clean, gofmt clean.

Summary by CodeRabbit

  • New Features

    • Added provider-aware payment-option normalization and canonicalization, with warnings when values are coerced or left for later validation.
  • Bug Fixes

    • Global validation accepts the union of provider tokens; service-level validation enforces provider-canonical tokens.
  • Tests

    • Expanded tests covering global/service validation and normalization across AWS, Azure, and GCP.
  • Documentation

    • Clarified that the legacy AWS option list is retained for backwards compatibility.

Review Change Stack

ServiceConfig.validatePayment was using the AWS-only ValidPaymentOptions
slice regardless of provider, rejecting Azure ("upfront", "monthly") and
GCP ("upfront", "monthly") payment tokens that their recommendation
emitters stamp after PR #682.

Add ValidPaymentOptionsByProvider (per-provider canonical sets),
validPaymentOptionsFor lookup, and update ServiceConfig.validatePayment
to route through the provider's own set with a clear error message. A
cross-provider token (e.g. "partial-upfront" on Azure) is rejected even
though it is valid somewhere else.

GlobalConfig.DefaultPayment validation switches to the union of all
provider sets so a global default can hold any provider-canonical token.

Closes #698
@cristim cristim added triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm urgency/this-sprint Within the current sprint impact/many Affects most users effort/s Hours type/bug Defect labels May 25, 2026
@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 19 minutes and 5 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 10976833-74b7-494d-8fed-76790eb2e2df

📥 Commits

Reviewing files that changed from the base of the PR and between 5036b11 and 944c744.

📒 Files selected for processing (2)
  • internal/config/validation.go
  • internal/config/validation_test.go
📝 Walkthrough

Walkthrough

Payment option validation is refactored from an AWS-only list to a provider-scoped map. Global defaults accept any token valid for at least one provider; ServiceConfig enforces provider-canonical tokens. Normalization helpers and scheduler emission canonicalization added; tests expanded for AWS, Azure, and GCP.

Changes

Provider-Aware Payment Validation

Layer / File(s) Summary
Provider-aware config and normalization
internal/config/validation.go
Add ValidPaymentOptionsByProvider, union/lookup helpers, NormalizePaymentOption, and internal alias mapping logic.
Global default validation
internal/config/validation.go, internal/config/validation_test.go
Global/default payment validation now checks the union of provider token sets (accepts tokens valid for any provider); tests added for Azure/GCP global tokens.
Service-config provider-canonical validation
internal/config/validation.go, internal/config/validation_test.go
ServiceConfig.validatePayment now enforces provider-specific canonical tokens and rejects cross-provider tokens; returns error if provider has no defined options. Tests cover AWS/Azure/GCP acceptance and rejection cases.
isValid / normalization tests
internal/config/validation_test.go
TestIsValidPaymentOption and TestNormalizePaymentOption expanded to assert Azure/GCP tokens are valid and to verify provider-specific normalization/alias behavior.
Scheduler emission canonicalization
internal/scheduler/scheduler.go
convertRecommendations normalizes rec.PaymentOption at emission via config.NormalizePaymentOption, logging coercions or warnings and updating the emitted token when mapped.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nibbled tokens, one by one,

Azure, GCP, and AWS—now done.
I hopped to map each alias true,
Normalized, the clouds align anew.
Hooray, the plans accept what's due!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 53.85% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: introducing provider-canonical payment option support in the plan validator, fixing the validation logic to accept Azure/GCP tokens.
Linked Issues check ✅ Passed All primary objectives from issue #698 are fully addressed: provider-scoped payment validation map implemented, provider-canonical tokens defined per provider (AWS/Azure/GCP), ServiceConfig.validatePayment updated with provider-aware lookup and error messages, comprehensive test coverage added for valid tokens and cross-provider rejection.
Out of Scope Changes check ✅ Passed All changes are directly aligned with issue #698 objectives: validation logic updates, provider-specific payment mappings, normalization helpers, and scheduler defensive coercion at emission boundary. No unrelated code modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/698-plan-validator-non-aws-payments

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

@cristim

cristim commented May 25, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor
✅ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
internal/config/validation_test.go (1)

354-438: ⚡ Quick win

Consider adding test coverage for synonym tokens.

The test suite covers the primary tokens but omits some synonyms that are explicitly included in the ValidPaymentOptionsByProvider map:

  • Azure: "no-upfront" synonym (semantically equivalent to "monthly")
  • GCP: "all-upfront" and "no-upfront" synonyms

Since synonym acceptance is a key feature of this PR (allowing both Azure/GCP canonical tokens and AWS-style tokens for compatibility), adding explicit test cases would strengthen confidence that the validation correctly handles all defined tokens.

🧪 Suggested additional test cases
{
	name: "azure no-upfront is valid (synonym of monthly)",
	config: ServiceConfig{
		Provider: "azure",
		Service:  "vm",
		Payment:  "no-upfront",
	},
	wantErr: false,
},
{
	name: "gcp all-upfront is valid (synonym of upfront)",
	config: ServiceConfig{
		Provider: "gcp",
		Service:  "computeengine",
		Payment:  "all-upfront",
	},
	wantErr: false,
},
{
	name: "gcp no-upfront is valid (synonym of monthly)",
	config: ServiceConfig{
		Provider: "gcp",
		Service:  "computeengine",
		Payment:  "no-upfront",
	},
	wantErr: false,
},
🤖 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/config/validation_test.go` around lines 354 - 438, Add explicit test
cases to the existing table-driven tests in validation_test.go (the tests that
construct ServiceConfig structs) to cover payment-token synonyms defined in
ValidPaymentOptionsByProvider: add an Azure case with Provider "azure", Service
"vm" and Payment "no-upfront" (wantErr: false), and add two GCP cases with
Provider "gcp", Service "computeengine" and Payments "all-upfront" and
"no-upfront" (both wantErr: false); place them alongside the other ServiceConfig
entries so the existing validation test logic that checks payment options will
exercise these synonyms.
internal/config/validation.go (2)

18-27: 💤 Low value

Clarify synonym handling in the documentation.

The comment states Azure/GCP use "upfront" and "monthly", but the actual data structure also includes "all-upfront" and "no-upfront". While the PR description mentions these are synonyms accepted for compatibility with other layers, the code comment doesn't explain this.

Consider updating the comment to clarify:

// ValidPaymentOptionsByProvider maps each provider to the payment option
// tokens it accepts. Azure and GCP reservations use "upfront" (all-upfront
// billing) and "monthly" (no-upfront / spread billing) as their canonical
// tokens; "all-upfront" and "no-upfront" are accepted as synonyms for
// compatibility with existing code. AWS RIs/SPs use the three classic tiers.
// See providers/{azure,gcp}/services/*/client.go for the case statements
// that consume these values.
🤖 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/config/validation.go` around lines 18 - 27, Update the comment above
ValidPaymentOptionsByProvider to clarify which tokens are canonical vs. accepted
synonyms: state that Azure and GCP canonical tokens are "upfront" (equivalent to
"all-upfront") and "monthly" (equivalent to "no-upfront"/spread billing) and
that "all-upfront" and "no-upfront" are accepted for compatibility, while AWS
uses the three classic tiers; keep the reference to
providers/{azure,gcp}/services/*/client.go for consumption details and ensure
the wording replaces the current ambiguous sentence about "upfront" and
"monthly".

29-44: 💤 Low value

Consider sorting the union for deterministic error messages.

The union slice is built by iterating over a map, which has non-deterministic iteration order in Go. This causes the error message on line 186 to display payment options in varying order across runs.

While this doesn't affect correctness (membership checks work regardless of order), sorting the slice would provide consistent error messages:

📋 Optional: Sort union for deterministic output
+	"sort"
+
 var validPaymentOptionsUnion = func() []string {
 	seen := map[string]bool{}
 	var all []string
 	for _, opts := range ValidPaymentOptionsByProvider {
 		for _, o := range opts {
 			if !seen[o] {
 				seen[o] = true
 				all = append(all, o)
 			}
 		}
 	}
+	sort.Strings(all)
 	return all
 }()
🤖 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/config/validation.go` around lines 29 - 44, validPaymentOptionsUnion
is built by iterating a map (ValidPaymentOptionsByProvider) so its resulting
slice order is non-deterministic; sort the resulting slice before returning to
ensure deterministic error messages. Update the function that constructs
validPaymentOptionsUnion to call sort.Strings(all) (and add the sort import)
right before the return, so the union of payment options is consistently ordered
across runs.
🤖 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/config/validation_test.go`:
- Around line 354-438: Add explicit test cases to the existing table-driven
tests in validation_test.go (the tests that construct ServiceConfig structs) to
cover payment-token synonyms defined in ValidPaymentOptionsByProvider: add an
Azure case with Provider "azure", Service "vm" and Payment "no-upfront"
(wantErr: false), and add two GCP cases with Provider "gcp", Service
"computeengine" and Payments "all-upfront" and "no-upfront" (both wantErr:
false); place them alongside the other ServiceConfig entries so the existing
validation test logic that checks payment options will exercise these synonyms.

In `@internal/config/validation.go`:
- Around line 18-27: Update the comment above ValidPaymentOptionsByProvider to
clarify which tokens are canonical vs. accepted synonyms: state that Azure and
GCP canonical tokens are "upfront" (equivalent to "all-upfront") and "monthly"
(equivalent to "no-upfront"/spread billing) and that "all-upfront" and
"no-upfront" are accepted for compatibility, while AWS uses the three classic
tiers; keep the reference to providers/{azure,gcp}/services/*/client.go for
consumption details and ensure the wording replaces the current ambiguous
sentence about "upfront" and "monthly".
- Around line 29-44: validPaymentOptionsUnion is built by iterating a map
(ValidPaymentOptionsByProvider) so its resulting slice order is
non-deterministic; sort the resulting slice before returning to ensure
deterministic error messages. Update the function that constructs
validPaymentOptionsUnion to call sort.Strings(all) (and add the sort import)
right before the return, so the union of payment options is consistently ordered
across runs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c210bf1b-bdcb-4b29-8337-2418b5926cf0

📥 Commits

Reviewing files that changed from the base of the PR and between 423e87c and c0d1a1f.

📒 Files selected for processing (2)
  • internal/config/validation.go
  • internal/config/validation_test.go

Add missing docstring for isValidPaymentOption to complete docstring
coverage for the new payment-option validation helpers introduced in
PR #698. All new package-level helpers now have proper Go-style doc
comments (ValidPaymentOptionsByProvider, validPaymentOptionsUnion,
validPaymentOptionsFor, isValidPaymentOption).

Resolves CodeRabbit docstring coverage pre-merge check (44.44% -> 100%).
@cristim

cristim commented May 25, 2026

Copy link
Copy Markdown
Member Author

Resolved: Added Go doc comment to isValidPaymentOption (commit 5ffc7a372). All new payment-validator helpers now have proper docstrings:

  • ValidPaymentOptionsByProvider - provider payment option mapping
  • validPaymentOptionsUnion - union of all provider options
  • validPaymentOptionsFor - provider-specific option lookup
  • isValidPaymentOption - global validation check

Docstring coverage should now be 100%. @coderabbitai review

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

@cristim: Thanks for the update! Glad to hear all the payment-validator helpers now have proper docstrings — 100% Go doc coverage on the new validation layer is a great touch. Triggering a fresh review now.

🐇 ✨

✅ 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

cristim commented May 25, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor
✅ 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

cristim commented May 25, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor
✅ 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 added 2 commits May 25, 2026 23:03
…ed provider semantics

PR #709 originally accepted both AWS-style tokens (all-upfront, no-upfront)
and the provider-canonical (upfront, monthly) for Azure and GCP, because the
per-service switches at providers/{azure,gcp}/services/*/client.go alias the
two pairs. That over-acceptance hid input bugs: a plan that mistakenly
stamped an AWS-style token on a non-AWS service would validate successfully
and only surface the mismatch later in pricing math.

Tighten the validator to the canonical token each provider semantically
models, verified by reading every per-service purchase/pricing switch:

  - AWS  : {no-upfront, partial-upfront, all-upfront} (unchanged)
  - Azure: {upfront, monthly}
  - GCP  : {upfront, monthly}

The validator now rejects cross-provider tokens with the existing
"invalid payment option: <tok> (valid for <provider>: <set>)" error,
giving callers an immediately actionable list of accepted values.

The Azure savingsplans switch at services/savingsplans/client.go:418-429
DOES mirror AWS's three-tier set, but Azure savings-plan recommendations
are not currently emitted (GetRecommendations returns []) - the canonical
set follows the only path that emits today and can be expanded later when
SP recs land.

Tests updated to assert the new rejections and that the error message
carries the canonical set. The GlobalConfig union test still passes -
the union of {no-upfront,partial-upfront,all-upfront,upfront,monthly}
is unchanged.
…efore validation

The validator tightened in the previous commit now rejects AWS-style
payment-option tokens ("all-upfront", "no-upfront", "partial-upfront") on
non-AWS service configs. That's correct as a config-layer invariant but
brittle if a recommendation-emission code path (or a globally-default
payment-option setting fanned out across providers) stamps an AWS-style
token onto a non-AWS rec — the rec persists with a token that the
validator later refuses, surfacing as a confusing "invalid payment
option" downstream rather than at the source.

Add config.NormalizePaymentOption(provider, raw) -> (canonical, ok) and
apply it at the shared emission boundary in scheduler.convertRecommendations,
where every common.Recommendation from every provider passes through on
its way to the RecommendationRecord table. Mapping:

  - AWS: passthrough.
  - Azure/GCP:
      all-upfront    → upfront
      no-upfront     → monthly
      partial-upfront → upfront  (coerce-to-nearest with WARN log)

Rationale for the partial-upfront coercion (vs drop-the-rec):

  partial-upfront has no semantic equivalent in Azure's or GCP's
  reservation/CUD models — both only model "one-time upfront" vs "monthly
  recurring". Dropping the rec would be a silent data loss for the user.
  Coercing to "upfront" preserves the rec at the closest billing tier
  the provider actually offers, and the WARN log surfaces the upstream
  stamping bug so an operator can fix it. This is the same tradeoff
  the existing per-service pricing switches make when their default
  branch falls back to "upfront" treatment.

Tests cover all 3 providers x {canonical, AWS-style, AWS-only,
Azure/GCP-style cross-feeds, empty, garbage} plus the unknown-provider
fast-fail path. The GlobalConfig union-set test is unchanged — the union
of tightened sets is still {no-upfront, partial-upfront, all-upfront,
upfront, monthly} since AWS's three tokens stay distinct.

Verified emission sites today already use canonical tokens (every
PaymentOption: "upfront" / "monthly" literal in providers/azure/ and
providers/gcp/). The normalizer is defensive belt-and-braces — its real
value is at the scheduler.convertRecommendations boundary where any
future code path or operator-set globalCfg.DefaultPayment that flows
into a non-AWS rec gets canonicalized once before persistence.
@cristim

cristim commented May 25, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor
✅ 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.

…UD semantics

PR #709's first revision tightened GCP to {upfront, monthly} matching the
per-service pricing switches at providers/gcp/services/*/client.go. Closer
inspection of the actual GCP CUD purchase path shows that's still an
over-acceptance: the GCP CUD API only takes a Plan discriminator
(TWELVE_MONTH / THIRTY_SIX_MONTH) and never reads a payment-option field —
see buildCommitmentRequests at providers/gcp/services/computeengine/client.go
:350-373. GCP commitments are inherently monthly-billed across the term;
"upfront" exists in the codebase only as a misnomer that one GCP emitter
(computeengine/client.go:804) still stamps, while the other three GCP
services (cloudsql, cloudstorage, memorystore) already stamp "monthly".

Tighten the validator's GCP set to {monthly} alone, with the doc comment
pointing at both the purchase path (where Plan is the only knob) and the
pricing switches (which alias "upfront"/"all-upfront" only for compatibility
with downstream code expecting a token). Azure remains {upfront, monthly} —
Azure reservations DO model both billing plans, verified against the seven
service-client switches at compute/cache/cosmosdb/database/search/synapse/
managedredis client.go (all accept {all-upfront, upfront} and {monthly,
no-upfront} as aliased pairs). AWS is unchanged at the canonical three-tier
{no-upfront, partial-upfront, all-upfront}.

Update NormalizePaymentOption's GCP branch to coerce every non-monthly
token — including the legacy "upfront" that computeengine/client.go:804
stamps — to "monthly". This keeps the existing GCP emission path safe: the
scheduler.convertRecommendations boundary now canonicalizes "upfront" →
"monthly" with a WARN log before persistence, so the rec carries the
provider-canonical token downstream and the validator no longer trips on
its way to the plan-validator. The Azure mapping is unchanged
(all-upfront → upfront, no-upfront → monthly, partial-upfront → upfront).

Existing helper survey before adding the new mapping:
  - internal/commitmentopts/normalizePayment is unexported and parses AWS
    API response spellings ("All Upfront" / "ALL_UPFRONT") into the
    canonical AWS three-tier set — different concern (within-AWS spelling
    normalization, not cross-provider canonicalization).
  - internal/api/validation.go:purchasePaymentWhitelist is the
    purchase-execute boundary, currently still permissive (accepts AWS-style
    tokens on Azure/GCP) — explicitly out of scope per the issue spec.
  - pkg/common/reservation_name.go:normalizeReservationPayment produces
    short-form name segments ("allup" / "noup" / "partup"), not canonical
    tokens — different concern.
No prior cross-provider canonicalization helper exists; the new function
stays in internal/config/validation.go alongside ValidPaymentOptionsByProvider.

Tests:
  - ServiceConfig: gcp-monthly stays valid, gcp-upfront is now rejected,
    error message asserts "valid for gcp: monthly".
  - NormalizePaymentOption: GCP canonical set is {monthly}; "upfront",
    "all-upfront", "no-upfront", "partial-upfront" all coerce to "monthly".
  - GlobalConfig union test unchanged — "upfront" is still in the union
    because Azure still has it.
@cristim

cristim commented May 25, 2026

Copy link
Copy Markdown
Member Author

Tightened GCP set further to {monthly} only (was {upfront, monthly}) per GCP CUD semantics — the GCP CUD purchase API takes only a Plan (TWELVE_MONTH / THIRTY_SIX_MONTH) and never reads a payment-option field (see providers/gcp/services/computeengine/client.go:350-373 buildCommitmentRequests); GCP commitments are inherently monthly-billed across the term. Azure remains {upfront, monthly} — verified against all 7 Azure service-client switches (compute / cache / cosmosdb / database / search / synapse / managedredis client.go) which alias {all-upfront, upfront} and {monthly, no-upfront} as pairs. AWS unchanged at {no-upfront, partial-upfront, all-upfront}.

NormalizePaymentOption GCP branch now collapses every non-monthly token (including the legacy "upfront" that providers/gcp/services/computeengine/client.go:804 stamps) to "monthly", with the scheduler.convertRecommendations boundary WARN-logging the coercion. Azure mapping unchanged (all-upfront → upfront, no-upfront → monthly, partial-upfront → upfront).

Searched for existing helpers before adding a new one: internal/commitmentopts/normalizePayment is unexported and AWS-spelling-focused (parses "All Upfront" / "ALL_UPFRONT" → canonical AWS tokens — different concern); internal/api/validation.go:purchasePaymentWhitelist is the purchase-execute boundary, kept out of scope per the issue spec; pkg/common/reservation_name.go:normalizeReservationPayment produces short-form name segments (different concern). No prior cross-provider canonicalization helper exists.

Files touched in this revision: internal/config/validation.go, internal/config/validation_test.go. Pre-existing emission sites (providers/azure/internal/recommendations/converter.go:ExpandPaymentVariants, providers/gcp/services/*/client.go rec converters) already emit canonical tokens; the normalizer is defensive belt-and-braces at the scheduler boundary.

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

@cristim: Solid rationale — narrowing GCP to {monthly} only is the right call given the CUD purchase API ignores the payment-option field entirely. The WARN-log coercion at the scheduler.convertRecommendations boundary is a nice defensive touch. The audit of pre-existing helpers confirms no overlap with the new normalizer. Triggering a fresh review now.

🐇 🔍


✏️ Learnings added
Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/709

Timestamp: 2026-05-25T21:45:57.448Z
Learning: In this repository (CUDly), GCP CUD commitments are inherently monthly-billed. The GCP CUD purchase API (`providers/gcp/services/computeengine/client.go:350-373`, `buildCommitmentRequests`) takes only a `Plan` field (TWELVE_MONTH / THIRTY_SIX_MONTH) and never reads a payment-option field. Therefore, the only valid payment option for GCP in `ValidPaymentOptionsByProvider` is `"monthly"`. The `NormalizePaymentOption` GCP branch should collapse any non-monthly token (including legacy `"upfront"`) to `"monthly"`, with a WARN log at the `scheduler.convertRecommendations` boundary.

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/709

Timestamp: 2026-05-25T21:45:57.448Z
Learning: In this repository (CUDly), Azure service clients alias payment options in pairs: `{all-upfront, upfront}` and `{monthly, no-upfront}`. All 7 Azure service-client switches (compute, cache, cosmosdb, database, search, synapse, managedredis client.go) use these aliases. The canonical Azure payment option set in `ValidPaymentOptionsByProvider` is `{upfront, monthly}` (with synonyms `all-upfront → upfront`, `no-upfront → monthly`, `partial-upfront → upfront`).

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/709

Timestamp: 2026-05-25T21:45:57.448Z
Learning: In this repository (CUDly), `internal/commitmentopts/normalizePayment` is unexported and AWS-spelling-focused (parses "All Upfront"/"ALL_UPFRONT" → canonical AWS tokens). `internal/api/validation.go:purchasePaymentWhitelist` is the purchase-execute boundary. `pkg/common/reservation_name.go:normalizeReservationPayment` produces short-form name segments. These are all distinct concerns from the cross-provider `NormalizePaymentOption` helper in `internal/config/validation.go`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
internal/config/validation.go (1)

70-82: ⚡ Quick win

Make union ordering deterministic for stable validation errors.

validPaymentOptionsUnion is assembled from map iteration, so the valid: ... list can appear in varying order. Sorting once here keeps error messages deterministic.

♻️ Proposed patch
 import (
 	"fmt"
 	"net/mail"
+	"sort"
 	"strings"
 )
@@
 var validPaymentOptionsUnion = func() []string {
 	seen := map[string]bool{}
 	var all []string
 	for _, opts := range ValidPaymentOptionsByProvider {
 		for _, o := range opts {
 			if !seen[o] {
 				seen[o] = true
 				all = append(all, o)
 			}
 		}
 	}
+	sort.Strings(all)
 	return all
 }()
🤖 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/config/validation.go` around lines 70 - 82, validPaymentOptionsUnion
is built by iterating over the map ValidPaymentOptionsByProvider which yields
non-deterministic ordering; sort the resulting slice once after construction so
validation error messages are stable. Modify the closure that defines
validPaymentOptionsUnion to sort the `all` slice (e.g., using sort.Strings)
before returning; reference the closure variable `validPaymentOptionsUnion` and
the source map `ValidPaymentOptionsByProvider` when making the change.
🤖 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/config/validation.go`:
- Around line 70-82: validPaymentOptionsUnion is built by iterating over the map
ValidPaymentOptionsByProvider which yields non-deterministic ordering; sort the
resulting slice once after construction so validation error messages are stable.
Modify the closure that defines validPaymentOptionsUnion to sort the `all` slice
(e.g., using sort.Strings) before returning; reference the closure variable
`validPaymentOptionsUnion` and the source map `ValidPaymentOptionsByProvider`
when making the change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fae6db29-340d-42a7-8e01-7e68c75c13d8

📥 Commits

Reviewing files that changed from the base of the PR and between 5ffc7a3 and 5036b11.

📒 Files selected for processing (3)
  • internal/config/validation.go
  • internal/config/validation_test.go
  • internal/scheduler/scheduler.go

Map iteration in Go is non-deterministic, so validPaymentOptionsUnion
could be built in a different order each run, causing validation error
messages listing valid payment options to vary across runs and making
test assertions on error strings brittle. Add sort.Strings(all) before
returning the union slice so the order is stable.

Add TestValidPaymentOptionsUnionDeterministic to assert the specific
expected sorted order and the pairwise ordering invariant.
@cristim

cristim commented May 25, 2026

Copy link
Copy Markdown
Member Author

Addressed the nitpick at `internal/config/validation.go:29-44` (non-deterministic `validPaymentOptionsUnion` order).

Fix (commit 3fb9cb9):

  • Added `sort.Strings(all)` before `return all` in the `validPaymentOptionsUnion` initializer function.
  • Added `"sort"` to the import block.
  • Updated the var comment to document why sorting is applied.

New test (`TestValidPaymentOptionsUnionDeterministic`):

  • Asserts the specific expected sorted order: `[all-upfront, monthly, no-upfront, partial-upfront, upfront]`.
  • Verifies the pairwise ordering invariant across all adjacent elements.

All 543 tests in `internal/config/...` pass; `go vet` and `gofmt` are clean.

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

@cristim: Clean fix — sort.Strings(all) is exactly the right remedy for the non-determinism, and pinning the expected lexicographic order ([all-upfront, monthly, no-upfront, partial-upfront, upfront]) in TestValidPaymentOptionsUnionDeterministic means any future addition to ValidPaymentOptionsByProvider that changes the union will break the test immediately, making the contract explicit. Triggering a fresh review now.

🐇 ✅

✅ 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.

…aming + sort union for deterministic errors

N2 (doc comment): replace the "accepts synonyms" framing with an explicit
canonical-sets description referencing the per-provider service-client
switch statements. Clarifies that cross-provider tokens are rejected loudly
and that NormalizePaymentOption canonicalizes at the rec-emission boundary
before the validator is reached.

N3/N4 (sort union): sort.Strings was already present in the init func and
the determinism test already existed; no code change needed.

N1 (synonym rejection tests): all rejection cases CR suggested as acceptance
cases (azure no-upfront, azure partial-upfront, gcp all-upfront, gcp
no-upfront, gcp upfront) were already present in the test suite as rejection
assertions, consistent with the tightened canonical-sets design decision.
@cristim

cristim commented May 25, 2026

Copy link
Copy Markdown
Member Author

Addressed the CR nitpick round (2026-05-25) in commit 944c744:

N1 (synonym acceptance tests) — SKIPPED (stale by design)
All the tokens CR suggested adding as acceptance cases (azure no-upfront, azure partial-upfront, gcp all-upfront, gcp no-upfront, gcp upfront) already exist in the test suite as rejection assertions, consistent with the tightened canonical-sets design (5036b11d5). Adding acceptance cases would contradict the design call.

N2 (doc comment) — ADAPTED
Updated the ValidPaymentOptionsByProvider godoc to the canonical-sets framing: each provider's bullet lists its set with a brief rationale pointing at the relevant service-client code. Added the cross-provider rejection + NormalizePaymentOption boundary sentence. Removed the previous "tightened to the canonical tokens each provider actually models semantically; AWS-style aliases ... are deliberately not surfaced" wording which was accurate but less clear.

N3 + N4 (sort union) — ALREADY FIXED
sort.Strings(all) was already present in the validPaymentOptionsUnion init func (landed in 3fb9cb9ed), and TestValidPaymentOptionsUnionDeterministic was already covering it. No code change needed.

Test counts: 543 in internal/config/..., 4788 total — unchanged.

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

@cristim: Well-reasoned dispositions across the board:

  • N1: Agreed — those tokens (azure no-upfront, azure partial-upfront, gcp *) are intentionally out of the canonical sets, so they belong only in rejection assertions. Adding acceptance cases would be a design contradiction, not a coverage gap.
  • N2: Canonical-sets framing with per-provider rationale bullets and the cross-provider rejection/NormalizePaymentOption boundary sentence is clearer than the previous wording.
  • N3 + N4: Already confirmed resolved in 3fb9cb9ed — nothing left to do there.

Triggering a fresh review now.

🐇 ✅

✅ 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
cristim merged commit a44d28e into feat/multicloud-web-frontend May 25, 2026
4 checks passed
cristim added a commit that referenced this pull request Jun 3, 2026
Address CodeRabbit review on PR #848:

- F1 (types.go): remove the misleading "GCP all-upfront" example from
  the MonthlyCost field comment. In this repo GCP CUD commitments are
  always monthly-billed (per #709, #717, #718, #829), so the only
  provider that can legitimately produce a NULL recurring breakdown at
  the commitment layer is Azure all-upfront. Comment now mentions only
  Azure and explicitly notes that GCP always populates MonthlyCost.

- F2 (store_postgres_db_test.go): add a "MonthlyCost nil round-trip"
  subtest that inserts a PurchaseHistoryRecord with MonthlyCost: nil
  and asserts it reads back as nil. This is the contract migration
  000063 was meant to establish (NULL -> nil, distinct from 0.0 -> &0),
  and the frontend renders the dash glyph vs "$0.00" based on it. Also
  strengthens the existing 0.0 subtest to assert the pointer is
  non-nil with *MonthlyCost == 0.0, so both sides of the contract are
  pinned end-to-end.

No production code changes; comment + test only.
@cristim
cristim deleted the fix/698-plan-validator-non-aws-payments branch June 3, 2026 21:54
cristim added a commit that referenced this pull request Jun 3, 2026
…#848)

* feat(db): nullable MonthlyCost on PurchaseHistoryRecord (closes #255)

Migration 000063 drops NOT NULL on purchase_history.monthly_cost so rows
from providers without a monthly recurring breakdown (Azure/GCP all-upfront)
can store NULL instead of a misleading 0.0.

- PurchaseHistoryRecord.MonthlyCost: float64 -> *float64
- InventoryCommitment.MonthlyCost: float64 -> *float64
- All scan loops use sql.NullFloat64; nil DB value -> nil pointer
- execution.go: remove derefFloat64; pass pointer straight through
- handler_inventory.go: skip nil MonthlyCost in coverage aggregation
- handler_history.go: sumRecommendationMonthlyCostPtr returns nil when
  every rec has nil MonthlyCost (renders as -- not $0.00)
- Frontend: monthly_cost typed number|null; inventory cell renders --
- All test fixtures and assertions updated to use *float64 (pf helper)

* style(api): apply gofmt alignment to InventoryCommitment and related types

* fix(api/tests): wrap MonthlyCost literals in float64Ptr after nullable refactor

The PurchaseHistoryRecord.MonthlyCost type changed to *float64 in
3cd63c8 but four test literals in handler_inventory_test.go
(lines 186, 196, 499, 508) still assigned untyped float constants,
breaking go vet with:

    cannot use 80.0 (untyped float constant 80) as *float64 value in
    struct literal

Switch them to float64Ptr(...) consistent with the rest of the file
(lines 77, 90, 145, 430 already use the helper).

* docs(db): clarify GCP commitments are monthly + nil round-trip test

Address CodeRabbit review on PR #848:

- F1 (types.go): remove the misleading "GCP all-upfront" example from
  the MonthlyCost field comment. In this repo GCP CUD commitments are
  always monthly-billed (per #709, #717, #718, #829), so the only
  provider that can legitimately produce a NULL recurring breakdown at
  the commitment layer is Azure all-upfront. Comment now mentions only
  Azure and explicitly notes that GCP always populates MonthlyCost.

- F2 (store_postgres_db_test.go): add a "MonthlyCost nil round-trip"
  subtest that inserts a PurchaseHistoryRecord with MonthlyCost: nil
  and asserts it reads back as nil. This is the contract migration
  000063 was meant to establish (NULL -> nil, distinct from 0.0 -> &0),
  and the frontend renders the dash glyph vs "$0.00" based on it. Also
  strengthens the existing 0.0 subtest to assert the pointer is
  non-nil with *MonthlyCost == 0.0, so both sides of the contract are
  pinned end-to-end.

No production code changes; comment + test only.
cristim added a commit that referenced this pull request Jun 5, 2026
… (#793)

Replace the hand-maintained purchasePaymentWhitelist var with
purchasePaymentSet(), which derives the accepted token set from
config.ValidPaymentOptionsByProvider -- the same source used by the
plan-validator in #709, eliminating drift between the two boundaries.

Apply config.NormalizePaymentOption before the whitelist check so legacy
AWS-style tokens on Azure/GCP are coerced to canonical spellings (e.g.
all-upfront -> upfront on Azure) before validation, preserving backward
compatibility for existing callers.

Tighten the rejection error to name the provider and list its valid
options: `invalid payment option for azure service: "foo"
(valid for azure: upfront, monthly)`.

Add 15 new test cases covering the per-provider canonical sets, the
cross-provider normalization paths, and the error message shape.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/many Affects most users priority/p1 Next up; this sprint severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant