feat(providers/azure): emit all-upfront + no-upfront payment variants per recommendation (closes #679) - #682
Merged
cristim merged 6 commits intoMay 22, 2026
Conversation
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements payment variant expansion for Azure reservation recommendations. The core ExpandPaymentVariants function splits each base recommendation into upfront and monthly variants with distinct recurring monthly costs and savings calculations. The logic is integrated into all Azure service clients (compute, cache, cosmosdb, database, search) and the Advisor adapter, with comprehensive unit and integration tests covering the expansion behavior and variant-specific pricing. ChangesAzure Payment Variant Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
providers/azure/services/search/client_test.go (1)
589-602: ⚡ Quick winAdd a non-nil conversion test case with propagated pricing fields.
This test only calls
convertAzureSearchRecommendation(ctx, nil), so it can’t catch missing extraction/population of cost and SKU fields before variant expansion. Add a fixture-backed case asserting resource type, costs, term, and payment-option inputs to fan-out.🤖 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 `@providers/azure/services/search/client_test.go` around lines 589 - 602, The test TestSearchClient_ConvertAzureSearchRecommendation only exercises convertAzureSearchRecommendation with a nil input; add a second fixture-backed subtest that passes a non-nil Azure recommendation object (with SKU/resource type, cost/pricing fields, term and payment option populated) into convertAzureSearchRecommendation and assert that returned rec includes the expected resource type (SKU), cost fields, CommitmentType/Term/PaymentOption and that those pricing fields are preserved before any variant expansion; locate and update TestSearchClient_ConvertAzureSearchRecommendation and the convertAzureSearchRecommendation-related fixtures to ensure the new case covers propagation of pricing and SKU into the fan-out logic.
🤖 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.
Inline comments:
In `@providers/azure/recommendations.go`:
- Line 237: ExpandPaymentVariants currently overwrites Advisor-provided
EstimatedSavings when OnDemandCost and CommitmentCost are zero; before calling
azrecs.ExpandPaymentVariants(*rec) capture the existing EstimatedSavings (parsed
from annualSavingsAmount) and after expansion restore it for records where both
OnDemandCost and CommitmentCost are unset (or zero). Locate the call where
recommendations = append(recommendations, azrecs.ExpandPaymentVariants(*rec)...)
and modify it to preserve EstimatedSavings for such Advisor records (e.g., save
rec.EstimatedSavings, call ExpandPaymentVariants, then for each expanded result
with zero OnDemandCost and CommitmentCost set its EstimatedSavings back to the
saved value).
In `@providers/azure/services/search/client.go`:
- Line 153: The code calls azrecs.ExpandPaymentVariants on a base recommendation
that is not populated from the Azure result, causing variants to have
empty/incorrect pricing and resource fields; update
convertAzureSearchRecommendation to map relevant fields from azureRec (pricing,
resource identifiers, currency, meterId, etc.) into the returned base
recommendation before ExpandPaymentVariants is called (or alternatively populate
those fields in the caller right before append), and ensure the caller uses that
populated recommendation when invoking ExpandPaymentVariants so the generated
variants inherit the correct values.
---
Nitpick comments:
In `@providers/azure/services/search/client_test.go`:
- Around line 589-602: The test
TestSearchClient_ConvertAzureSearchRecommendation only exercises
convertAzureSearchRecommendation with a nil input; add a second fixture-backed
subtest that passes a non-nil Azure recommendation object (with SKU/resource
type, cost/pricing fields, term and payment option populated) into
convertAzureSearchRecommendation and assert that returned rec includes the
expected resource type (SKU), cost fields, CommitmentType/Term/PaymentOption and
that those pricing fields are preserved before any variant expansion; locate and
update TestSearchClient_ConvertAzureSearchRecommendation and the
convertAzureSearchRecommendation-related fixtures to ensure the new case covers
propagation of pricing and SKU into the fan-out logic.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32b32ee8-8c2f-4b41-8ff0-ff9de30e957a
📒 Files selected for processing (14)
providers/azure/internal/recommendations/converter.goproviders/azure/internal/recommendations/converter_test.goproviders/azure/recommendations.goproviders/azure/recommendations_test.goproviders/azure/services/cache/client.goproviders/azure/services/cache/client_test.goproviders/azure/services/compute/client.goproviders/azure/services/compute/client_test.goproviders/azure/services/cosmosdb/client.goproviders/azure/services/cosmosdb/client_test.goproviders/azure/services/database/client.goproviders/azure/services/database/client_test.goproviders/azure/services/search/client.goproviders/azure/services/search/client_test.go
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
cristim
force-pushed
the
feat/679-azure-payment-variants
branch
from
May 22, 2026 16:41
9795292 to
d7ba251
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
Rebased onto latest feat/multicloud-web-frontend (now atop 65ecbf8 from #680). Conflicts in the 5 service client files (cache/compute/cosmosdb/database/search) were limited to the import block: #680 added the new services/internal/reservations import while this PR renamed the recommendations import to the azrecs alias. Resolution kept both imports per file (azrecs alias + reservations). Also added a follow-up test commit (d7ba251) splitting the search converter test into a nil-guard + populate-all-fields pair to match the azrecs.Extract contract introduced in the CR auto-fix pass — this test was already failing on the PR HEAD before the rebase because ci.yml does not trigger on PRs targeting feat/multicloud-web-frontend (only main/develop), so the breakage was hidden. All 4743 tests pass locally, go vet clean, gofmt clean, gocyclo under budget. Re-pinging review since the search test fix is a semantic change beyond pure conflict resolution. @coderabbitai review |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@providers/azure/internal/recommendations/converter.go`:
- Around line 287-292: The current computation of savings and savingsPct
(variables totalOnDemand, totalReservation, savingsPct, savings) can produce
negative EstimatedSavings when OnDemandCost/totalOnDemand is zero; change the
logic in the converter where these variables are computed so that if
totalOnDemand == 0 you set savings = 0 and savingsPct = 0 (instead of computing
totalOnDemand - totalReservation), otherwise keep the existing calculations
((totalOnDemand - totalReservation) and percent formula); ensure the downstream
EstimatedSavings/OnDemandCost usage uses these guarded values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45ce49b9-f2a8-4566-ab9f-b84625929b77
📒 Files selected for processing (14)
providers/azure/internal/recommendations/converter.goproviders/azure/internal/recommendations/converter_test.goproviders/azure/recommendations.goproviders/azure/recommendations_test.goproviders/azure/services/cache/client.goproviders/azure/services/cache/client_test.goproviders/azure/services/compute/client.goproviders/azure/services/compute/client_test.goproviders/azure/services/cosmosdb/client.goproviders/azure/services/cosmosdb/client_test.goproviders/azure/services/database/client.goproviders/azure/services/database/client_test.goproviders/azure/services/search/client.goproviders/azure/services/search/client_test.go
✅ Files skipped from review due to trivial changes (1)
- providers/azure/services/cosmosdb/client_test.go
…upfront variants Azure reservation recommendations previously emitted a single entry with PaymentOption="upfront" (bare, non-canonical). Users could not see the no-upfront (monthly billing) alternative even though Azure supports both billing plans at the same total price. Changes: - Add ExpandPaymentVariants helper in providers/azure/internal/recommendations/ (alongside the existing Extract helper). Given a base recommendation it returns two copies: one with PaymentOption="all-upfront" (RecurringMonthlyCost=0) and one with PaymentOption="no-upfront" (RecurringMonthlyCost=CommitmentCost/termMonths). EstimatedSavings and SavingsPercentage are identical across both variants since Azure charges the same total price for both billing plans. - Update all five service converters (compute, cache, database, cosmosdb, search) and the Azure Advisor path to call ExpandPaymentVariants, producing two recs per API recommendation instead of one. - Rename the import alias to azrecs in each service client to resolve the local- variable shadowing that the recommendations package name previously caused. - Replace bare "upfront" with canonical "all-upfront" in all Azure code paths. - Add focused unit tests for ExpandPaymentVariants covering: zero on-demand guard, zero commitment cost, 1yr vs 3yr term-month math, pointer independence between variants, savings equality across variants, and shared-field pass-through. - Add TestComputeClient_GetRecommendations_EmitsBothPaymentVariants to exercise the end-to-end fan-out path via a mock pager. - Update existing converter tests to expect "all-upfront" instead of "upfront". Closes #679
Fixed 2 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
…t gocyclo budget Pull the per-page advisor-record loop body out of getAdvisorRecommendations into appendAdvisorPageRecs, following the same pattern used for resolveAdvisorRegion. getAdvisorRecommendations drops from cyclomatic 12 to 4; appendAdvisorPageRecs lands at 5. No behaviour change.
After the azrecs.Extract refactor, convertAzureSearchRecommendation returns nil on unusable SDK payloads (matching the contract shared by cache, compute, cosmosdb, and database converters). Split the single test into a nil-guard test and a field-population test that uses BuildLegacyReservationRecommendation, consistent with the other service converter test patterns.
… variant expansion ExpandPaymentVariants was emitting "all-upfront"/"no-upfront" (AWS terms) instead of the Azure-canonical "upfront"/"monthly". This made the emitted values inconsistent with every other Azure code path and with the AZURE_PAYMENTS constant in commitmentOptions.ts which defines "upfront" and "monthly" as the canonical values. Rename throughout: - "all-upfront" -> "upfront" in ExpandPaymentVariants, all five service converter functions (compute/cache/database/cosmosdb/search), and the convertAdvisorRecommendation base rec. - "no-upfront" -> "monthly" in ExpandPaymentVariants. - Update all test assertions and fixtures to match. - Update the ExpandPaymentVariants docstring precondition and value list. The purchase-path switch cases in all Azure reservation clients already accept both the canonical value and the former alias (e.g. case "all-upfront", "upfront": and case "monthly", "no-upfront":), so renaming the emitted values does not break the purchase path. Add "upfront" -> "Upfront" to PAYMENT_DISPLAY_LABELS in recommendations.ts so the Payment column in the recommendations table renders a human-readable label for Azure "upfront" recs rather than falling back to the raw string. "monthly" was already covered.
cristim
force-pushed
the
feat/679-azure-payment-variants
branch
from
May 22, 2026 19:21
d7ba251 to
c6c2df9
Compare
Member
Author
|
Pushed commit c6c2df9 to rename the payment option values emitted by
Why the existing consumers already accept the renamed values:
So the renamed emitted values route correctly with no breakage. Frontend label map: Added Files changed: 15 files - Tests: 571 Go tests pass (14 packages). Frontend recommendations tests pass (309 tests). @coderabbitai review |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
providers/azure/internal/recommendations/converter.go (1)
287-292:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
EstimatedSavingswhenOnDemandCostis zero.On Line 291,
savingsis still computed astotalOnDemand - totalReservationeven whentotalOnDemand == 0, which can emit negative savings despite the zero-demand guard described in the function contract.💡 Proposed fix
var savingsPct float64 - if totalOnDemand != 0 { - savingsPct = (totalOnDemand - totalReservation) / totalOnDemand * 100 - } - savings := totalOnDemand - totalReservation + var savings float64 + if totalOnDemand != 0 { + savings = totalOnDemand - totalReservation + savingsPct = (savings / totalOnDemand) * 100 + }🤖 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 `@providers/azure/internal/recommendations/converter.go` around lines 287 - 292, The code computes savings := totalOnDemand - totalReservation even when totalOnDemand == 0 which can produce negative EstimatedSavings; update the logic so when totalOnDemand == 0 (OnDemandCost == 0) you set savings = 0 and savingsPct = 0 (or skip percentage calc) instead of subtracting reservations, so that EstimatedSavings and savingsPct reflect zero demand; adjust the block around variables savingsPct, savings, totalOnDemand, totalReservation so EstimatedSavings never becomes negative when OnDemandCost is zero.
🧹 Nitpick comments (2)
providers/azure/internal/recommendations/converter_test.go (1)
312-319: ⚡ Quick winAdd a regression case for
OnDemandCost == 0 && CommitmentCost > 0.Current coverage only checks the
0/0path; it won’t catch negativeEstimatedSavingsregressions when commitment is non-zero and on-demand is zero.💡 Suggested test tweak
-func TestExpandPaymentVariants_ZeroOnDemand_NoSavings(t *testing.T) { +func TestExpandPaymentVariants_ZeroOnDemand_NoSavings(t *testing.T) { // Guard: avoid divide-by-zero when OnDemandCost is 0. - variants := ExpandPaymentVariants(baseRec(common.ServiceCompute, "1yr", 0, 0)) + variants := ExpandPaymentVariants(baseRec(common.ServiceCompute, "1yr", 0, 10)) for _, v := range variants { assert.InDelta(t, 0.0, v.EstimatedSavings, 1e-9) assert.InDelta(t, 0.0, v.SavingsPercentage, 1e-9) } }🤖 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 `@providers/azure/internal/recommendations/converter_test.go` around lines 312 - 319, Add a regression test for the case where OnDemandCost == 0 but CommitmentCost > 0 by creating a new test (e.g., TestExpandPaymentVariants_ZeroOnDemand_NonZeroCommitment) that calls ExpandPaymentVariants(baseRec(common.ServiceCompute, "1yr", 0, X)) with X>0, then assert for each variant that EstimatedSavings is negative and equals -CommitmentCost using assert.InDelta(t, -float64(X), v.EstimatedSavings, 1e-9) (or equivalent) and that SavingsPercentage is negative and finite (assert.Less(t, v.SavingsPercentage, 0.0) and ensure it is not NaN/Inf); reference ExpandPaymentVariants and baseRec to locate the code under test.providers/azure/services/search/client_test.go (1)
598-622: ⚡ Quick winAdd cost-field assertions to match the test contract.
TestSearchClient_ConvertAzureSearchRecommendation_PopulatesAllFieldscurrently doesn’t assert the cost fields passed viamocks.WithCosts(...). Verifying them will lock in the converter behavior that this PR depends on.Proposed test assertion additions
assert.Equal(t, "standard2", rec.ResourceType) assert.Equal(t, 2, rec.Count) + assert.Equal(t, 120.0, rec.OnDemandCost) + assert.Equal(t, 80.0, rec.CommitmentCost) + assert.Equal(t, 40.0, rec.EstimatedSavings) assert.Equal(t, common.CommitmentReservedInstance, rec.CommitmentType) assert.Equal(t, "1yr", rec.Term) assert.Equal(t, "upfront", rec.PaymentOption)🤖 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 `@providers/azure/services/search/client_test.go` around lines 598 - 622, The test TestSearchClient_ConvertAzureSearchRecommendation_PopulatesAllFields is missing assertions that verify the cost values supplied via mocks.WithCosts(120, 80, 40); update the test after calling client.convertAzureSearchRecommendation(ctx, azRec) to assert that the resulting rec contains the expected cost fields (the three cost-related fields produced by the converter) match 120, 80 and 40 respectively — locate this in the same test around azRec/rec and add assert.Equal (or require.Equal) checks referencing the actual rec cost properties produced by the converter.
🤖 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.
Duplicate comments:
In `@providers/azure/internal/recommendations/converter.go`:
- Around line 287-292: The code computes savings := totalOnDemand -
totalReservation even when totalOnDemand == 0 which can produce negative
EstimatedSavings; update the logic so when totalOnDemand == 0 (OnDemandCost ==
0) you set savings = 0 and savingsPct = 0 (or skip percentage calc) instead of
subtracting reservations, so that EstimatedSavings and savingsPct reflect zero
demand; adjust the block around variables savingsPct, savings, totalOnDemand,
totalReservation so EstimatedSavings never becomes negative when OnDemandCost is
zero.
---
Nitpick comments:
In `@providers/azure/internal/recommendations/converter_test.go`:
- Around line 312-319: Add a regression test for the case where OnDemandCost ==
0 but CommitmentCost > 0 by creating a new test (e.g.,
TestExpandPaymentVariants_ZeroOnDemand_NonZeroCommitment) that calls
ExpandPaymentVariants(baseRec(common.ServiceCompute, "1yr", 0, X)) with X>0,
then assert for each variant that EstimatedSavings is negative and equals
-CommitmentCost using assert.InDelta(t, -float64(X), v.EstimatedSavings, 1e-9)
(or equivalent) and that SavingsPercentage is negative and finite
(assert.Less(t, v.SavingsPercentage, 0.0) and ensure it is not NaN/Inf);
reference ExpandPaymentVariants and baseRec to locate the code under test.
In `@providers/azure/services/search/client_test.go`:
- Around line 598-622: The test
TestSearchClient_ConvertAzureSearchRecommendation_PopulatesAllFields is missing
assertions that verify the cost values supplied via mocks.WithCosts(120, 80,
40); update the test after calling client.convertAzureSearchRecommendation(ctx,
azRec) to assert that the resulting rec contains the expected cost fields (the
three cost-related fields produced by the converter) match 120, 80 and 40
respectively — locate this in the same test around azRec/rec and add
assert.Equal (or require.Equal) checks referencing the actual rec cost
properties produced by the converter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de5e5ac9-a029-469c-99f1-882b3e772ae9
📒 Files selected for processing (11)
frontend/src/recommendations.tsproviders/azure/internal/recommendations/converter.goproviders/azure/internal/recommendations/converter_test.goproviders/azure/recommendations.goproviders/azure/services/cache/client.goproviders/azure/services/compute/client.goproviders/azure/services/compute/client_test.goproviders/azure/services/cosmosdb/client.goproviders/azure/services/database/client.goproviders/azure/services/search/client.goproviders/azure/services/search/client_test.go
✅ Files skipped from review due to trivial changes (1)
- frontend/src/recommendations.ts
Three findings from CodeRabbit review 4348367061: 1. `converter.go:287-292` (recurring Major): the OnDemandCost-zero guard protected savingsPct (divide-by-zero) but the savings = totalOnDemand - totalReservation line ran unconditionally. Result: when OnDemandCost was zero and CommitmentCost was non-zero, EstimatedSavings was emitted as a negative number (-CommitmentCost), contradicting the function contract. Move savings inside the guard so both fields stay 0 when on-demand is zero. 2. `converter_test.go`: add TestExpandPaymentVariants_ZeroOnDemand_NonZeroCommitment regression asserting EstimatedSavings == 0 (not -CommitmentCost) when OnDemand=0, Commitment>0. The existing _ZeroOnDemand_NoSavings test only covered the 0/0 sub-case and would have missed the regression. 3. `client_test.go`: add cost-field assertions to TestSearchClient_ConvertAzureSearchRecommendation_PopulatesAllFields so the WithCosts(120, 80, 40) fixture is actually checked through to rec.OnDemandCost / rec.CommitmentCost / rec.EstimatedSavings. Locks in the converter behaviour this PR depends on.
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This was referenced May 25, 2026
cristim
added a commit
that referenced
this pull request
May 25, 2026
…tor (#709) * fix(config): accept provider-canonical payment options in plan validator 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 * docs(config): add Go docs for new payment-validator helpers 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%). * fix(config): tighten provider-canonical payment-option sets to verified 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. * fix(rec-emission): normalize AWS-style payment tokens for GCP/Azure before 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. * fix(config): tighten GCP payment-option set to monthly-only per GCP CUD 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. * fix(config): deterministic ordering for validPaymentOptionsUnion 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. * docs(config): clarify ValidPaymentOptionsByProvider canonical-sets framing + 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.
7 tasks
7 tasks
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
"upfront"label). Users had no way to see the monthly (no-upfront) billing alternative, even though Azure supports both billing plans at the same total price.ExpandPaymentVariantshelper inproviders/azure/internal/recommendations/that fans a single base recommendation out into two: one withPaymentOption="all-upfront"(RecurringMonthlyCost=0) and one withPaymentOption="no-upfront"(RecurringMonthlyCost=CommitmentCost/termMonths). EstimatedSavings and SavingsPercentage are identical across both variants.recommendationstoazrecsin each client to resolve local-variable shadowing.Closes #679
Test plan
go build ./providers/azure/...passes with exit 0go test github.com/LeanerCloud/CUDly/providers/azure/... github.com/LeanerCloud/CUDly/providers/azure/internal/... github.com/LeanerCloud/CUDly/providers/azure/services/...- all 529 tests pass (18 new tests added: 12 inconverter_test.goforExpandPaymentVariants+ 1 incompute/client_test.gofor end-to-end fan-out; 6 existing tests updated to expect canonical"all-upfront")TestComputeClient_GetRecommendations_EmitsBothPaymentVariantsverifies a single Azure API recommendation produces exactly two output entries with the correct cashflow split and identical savings valuesTestExpandPaymentVariants_RecurringMonthlyCostPointersAreIndependentverifies the two variants hold distinct pointer allocationsSummary by CodeRabbit
Release Notes
New Features
Tests