Skip to content

[usage] API for setting billingStrategy #12882

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

svenefftinge
Copy link
Member

@svenefftinge svenefftinge commented Sep 12, 2022

Description

Related Issue(s)

Fixes #

How to test

Release Notes

Documentation

Werft options:

  • /werft with-preview

@svenefftinge svenefftinge requested a review from a team September 12, 2022 14:26
@roboquat
Copy link
Contributor

@svenefftinge: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-sefftinge-ubp-model-free-plan-and-12741.7 because the annotations in the pull request description changed
(with .werft/ from main)

Comment on lines +120 to +123
enum BillingStrategy {
BILLING_STRATEGY_STRIPE = 0;
BILLING_STRATEGY_OTHER = 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

Because all fields in a protobuf message are optional, and protobuf defaults to the empty value, for Enums it will default to BILLING_STRATEGY_STRIPE in this struct, when not set. That's not always desirable, as if you forget to set it it will have adverse effects. For that reason, the UNKNOWN value is normally used in any enum (the RPC is then implemented to fail if the value is UNKNOWN.

Suggested change
enum BillingStrategy {
BILLING_STRATEGY_STRIPE = 0;
BILLING_STRATEGY_OTHER = 1;
}
enum BillingStrategy {
BILLING_STRATEGY_UNKNOWN = 0
BILLING_STRATEGY_STRIPE = 1;
BILLING_STRATEGY_OTHER = 2;
}

Comment on lines +201 to 214
summary, err := db.GetUsageSummary(ctx, s.conn, attributionID, time.Now(), time.Now(), false)
if err != nil {
return nil, err
}
if summary.CreditCentsBalanceAtEnd.ToCredits() > float64(in.SpendingLimit) {
return nil, status.Errorf(codes.InvalidArgument, "The spending limit must be higher than the current usage %u", summary.CreditCentsBalanceAtEnd.ToCredits())
}

//TODO (se) - Check if the strategy has changed and was 'other' before. If so run the invoice finalization.
_, err = db.SaveCostCenter(ctx, s.conn, &db.CostCenter{
ID: attributionID,
SpendingLimit: in.CostCenter.SpendingLimit,
SpendingLimit: in.SpendingLimit,
BillingStrategy: billingStrategy,
})
Copy link
Member

Choose a reason for hiding this comment

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

There's a possible race here due to write after read semantics, but I think in our case we don't actually need to handle it as we'll recoup it in the next billing cycle.

const currentInvoiceCredits = upcomingInvoice.getCredits();
if (currentInvoiceCredits >= costCenter.spendingLimit) {
const currentInvoiceCredits = upcomingInvoice.credits | 0;
if (currentInvoiceCredits >= (costCenter.spendingLimit || 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than || 0 should we use the same defaultSpendingLimit as when the Stripe subscription is configured?

See this comment.

@svenefftinge svenefftinge deleted the sefftinge/ubp-model-free-plan-and-12741 branch September 13, 2022 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants