-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Use the actual billing cycle dates in all usage-based Billing pages #14485
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
Conversation
started the job as gitpod-build-jx-actual-billing-cycle.1 because the annotations in the pull request description changed |
28a9896
to
e6b8596
Compare
Insert coin to try again 🎰 /werft run 👍 started the job as gitpod-build-jx-actual-billing-cycle.3 |
e6b8596
to
9bad6c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add an explicit BillingCycleStart property to cost center. (see comment above)
Need an explicit billing cycle start date in cost centers. Back to Draft 📝 |
9bad6c3
to
4893567
Compare
dc8d8a7
to
e8e124c
Compare
de11def
to
7822137
Compare
…Center API when it should be undefined 🙃
…e dates in usage-based Billing pages
7822137
to
e65dc3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Comments are mostly nits. The only thing we should look into soon is how to update the introduced column on the existing cost centers.
@@ -180,6 +184,7 @@ func (c *CostCenterManager) UpdateCostCenter(ctx context.Context, newCC CostCent | |||
return CostCenter{}, err | |||
} | |||
|
|||
newCC.BillingCycleStart = NewVarcharTime(now) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a little assymmetrical to have the start time but not the end time.
Do we know the end time for stripe as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svenefftinge Good catch. We know the (approximate) end time for Stripe -- it's the last millisecond of the calendar month.
However, I wasn't fully sure of the implications of setting it. Will Gitpod do anything when it reaches a nextBillingTime
for a Stripe CostCenter? (This would be problematic, because it's in fact Stripe's responsibility to issue and finalize an invoice, so I think Gitpod shouldn't do anything proactively on the Cost Center and instead wait for Stripe's callback.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we wouldn't do the invoice finalization, of course. But this is clearly communicated through the billingStrategy
. I just thought it seems strange that we have a starting time but no end time. One could argue that the start time is managed by stripe as well.
@@ -311,6 +318,9 @@ func (c *CostCenterManager) ResetUsage(ctx context.Context, cc CostCenter) (Cost | |||
|
|||
now := time.Now().UTC() | |||
|
|||
// Resetting the usage always resets the billing cycle start time | |||
billingCycleStart := now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: non-needed variable
@@ -325,11 +335,12 @@ func (c *CostCenterManager) ResetUsage(ctx context.Context, cc CostCenter) (Cost | |||
|
|||
// All fields on the new cost center remain the same, except for CreationTime and NextBillingTime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the comment is no longer 100% correct
BillingCycleStart := timestamppb.New(c.BillingCycleStart.Time()) | ||
if !c.BillingCycleStart.IsSet() { | ||
BillingCycleStart = nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would be good to introduce a method that does the conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@svenefftinge Good idea. Where should this method live? Is it possible to add it to go.Time
or timestamppb
somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put it into /workspace/gitpod/components/usage/pkg/db/types.go. @easyCZ wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DB package shouldn't have any dependency on API level packages. If we need a conversion, it should be in the API package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@easyCZ So in usage.go
is fine? 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd create apiv1/time.go
or apiv1/types.go
and put it there. There's nothing wrong with it being in usage.go
but it could make it harder to discover that the helper exists. As per go guidance, avoiding files such as util
or helper
or tools
is recommended to avoid kitchen-sink scenarios.
types.go
is fairly standard and often used for the underlying types and conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the guidance 👍
As per go guidance, avoiding files such as
util
orhelper
ortools
is recommended to avoid kitchen-sink scenarios.
💯 💯 💯 (love this about Go -- util
is one of my pet peeves)
Description
Use the actual billing cycle dates in all usage-based Billing pages
Related Issue(s)
Fixes #14363
How to test
a. this enables usage-based for your user
/billing
)a. notice the billing cycle dates (from ~now to ~a month from now)
a. notice the new billing cycle dates (from ~now to the last UTC millisecond of the month)
Hint: You can hover on the billing cycle dates to see their exact times.
Release Notes
Documentation
Werft options:
If enabled this will build
install/preview
Valid options are
all
,workspace
,webapp
,ide