[usage] Cost centers get `nextBillingTime` by svenefftinge · Pull Request #12913 · gitpod-io/gitpod

svenefftinge · 2022-09-13T12:16:26Z

Description

This PR contains small steps towards having cost centers for all users and teams, even if they haven't signed up for stripe pay-as-you go, yet, but using the billing strategy other.

In this PR I

introduce a property nextBillingTime on cost centers that will be used in an upcoming PR where a control loop uses the date to find cost centers that need to run finalization (i.e. create a synthetic invoice to balance out the ledger table).
make GetCostCenter always return a cost center

Related Issue(s)

Fixes #

How to test

Release Notes

NONE

Documentation

Werft options:

/werft with-preview
/werft with-payment

werft-gitpod-dev-com · 2022-09-13T14:56:28Z

started the job as gitpod-build-se-cost-center-next-billing.4 because the annotations in the pull request description changed
(with .werft/ from main)

easyCZ · 2022-09-14T07:46:15Z

    }
    BillingStrategy billing_strategy = 3;
+
+    // next_billing_time specifies when the next billing cycle happens. Only set when billing strategy is 'other'. This property is readonly.


Nice use of proto comments to describe semantics!

easyCZ · 2022-09-14T07:46:25Z

+  "defaultSpendingLimitForUsers": 5000,
+  "defaultSpendingLimitForTeams": 0,


easyCZ · 2022-09-14T07:47:30Z

+	DefaultSpendingLimitForUsers int32 `json:"defaultSpendingLimitForUsers,omitempty"`
+
+	DefaultSpendingLimitForTeams int32 `json:"defaultSpendingLimitForTeams,omitempty"`


Suggested change

DefaultSpendingLimitForUsers int32 `json:"defaultSpendingLimitForUsers,omitempty"`

DefaultSpendingLimitForTeams int32 `json:"defaultSpendingLimitForTeams,omitempty"`

DefaultSpendingLimitForUsers int `json:"defaultSpendingLimitForUsers,omitempty"`

DefaultSpendingLimitForTeams int `json:"defaultSpendingLimitForTeams,omitempty"`

This should be sufficient. int is actually compiler target dependant but often it's easier to use int and let the compiler deal with it.

easyCZ · 2022-09-14T07:50:52Z


 	Server *baseserver.Configuration `json:"server,omitempty"`
+
+	apiv1.UsageConfig


I'd name this explicitly. It also allows you to be explicit about the json field name which guards against issues when you rename the apiv1.UsageConfig in the future.

easyCZ

Looking good, left a couple of questions.
/hold

We can tackle these in a follow-up if we want.

I'd also recommend isolating DB migrations away from application changes which make use of those values. When we introduced #12873 and rolled it out, we actually saw requests fail as the migration was not compatible with the application changes - while this can happen with migrations being independent as well, it makes it easier to reason about it in the given PR.

easyCZ · 2022-09-15T06:40:08Z

 	require.Equal(t, costCenter.SpendingLimit, read.SpendingLimit)
+}
+
+func TestGetCostCenter(t *testing.T) {


Suggested change

func TestGetCostCenter(t *testing.T) {

func TestCostCenterManager_GetOrCreateCostCenter(t *testing.T) {

easyCZ · 2022-09-15T06:40:28Z

 }

-func TestGetCostCenter(t *testing.T) {
+func TestSaveCostCenter(t *testing.T) {


Suggested change

func TestSaveCostCenter(t *testing.T) {

func TestCostCenterManager_SaveCostCenter(t *testing.T) {

easyCZ · 2022-09-15T06:43:02Z

-	db := conn.WithContext(ctx)
-	costCenter.CreationTime = NewVarcharTime(time.Now())
-	db = db.Save(costCenter)
+func (c *CostCenterManager) SaveCostCenter(ctx context.Context, costCenter CostCenter) (CostCenter, error) {


From a consumer of the CostCenterManager API perspective, it's quite unclear what the difference is between GetOrCreate and SaveCostCenter. Why would I use one over the other?

I think here, we can rename it to

Suggested change

func (c *CostCenterManager) SaveCostCenter(ctx context.Context, costCenter CostCenter) (CostCenter, error) {

func (c *CostCenterManager) UpdateCostCenter(ctx context.Context, costCenter CostCenter) (CostCenter, error) {

To better communicate that this is intended to work on an existing record(even if it needs to be created under the hood)

I aligned with the naming of gorm but have no problem changing this to 'Update..'.

easyCZ · 2022-09-15T06:45:30Z

 	return costCenter, nil
 }
+
+func (c *CostCenterManager) ComputeFinalization(ctx context.Context, costCenter CostCenter) (Usage, error) {


Suggested change

func (c *CostCenterManager) ComputeFinalization(ctx context.Context, costCenter CostCenter) (Usage, error) {

func (c *CostCenterManager) ComputeFinalization(ctx context.Context, costCenter CostCenter) (Usage, error) {

From this signature, I'd have expected that it will also store the Usage record in the DB. Perhaps renaming to InvoiceUsageRecordForCostCenter would convey this better.

Additionally, why this is method accept the costCenter CostCenter when it only uses the costCenter.ID? Could we supply only the ID here?

I generally use 'Compute...' to indicate that a function doesn't have aside-effects but just computes something. But I'm happy to change the name to make it clearer for you.
I'm passing in the costCenter because it contains information about spendingLimit which is likely to be used pretty soon, but we can fetch it within the function if needed.

easyCZ · 2022-09-15T06:46:44Z

+func cleanUp(t *testing.T, conn *gorm.DB, attributionIds ...db.AttributionID) {
 	t.Cleanup(func() {


Suggested change

func cleanUp(t *testing.T, conn *gorm.DB, attributionIds ...db.AttributionID) {

t.Cleanup(func() {

func cleanUp(t *testing.T, conn *gorm.DB, attributionIds ...db.AttributionID) {

t.Helper()

t.Cleanup(func() {

just for general practice

easyCZ · 2022-09-15T06:47:18Z

 	EffectiveTime       VarcharTime    `gorm:"column:effectiveTime;type:varchar;size:255;" json:"effectiveTime"`
 	Kind                UsageKind      `gorm:"column:kind;type:char;size:10;" json:"kind"`
-	WorkspaceInstanceID uuid.UUID      `gorm:"column:workspaceInstanceId;type:char;size:36;" json:"workspaceInstanceId"`
+	WorkspaceInstanceID *uuid.UUID     `gorm:"column:workspaceInstanceId;type:char;size:36;" json:"workspaceInstanceId"`


FWIW, I'd have split this into a separate pre-PR to isolate it from the rest of the main logic in this PR.

easyCZ · 2022-09-15T06:49:07Z

+func (a AttributionID) IsUser() bool {
+	entity, _ := a.Values()
+	return entity == "user"
+}


[personal] I generally prefer not to introduce Binary methods on an object which can have more than a binary number of outcomes. This forces the caller to use

switch a.Kind { case User: ... case Team: ... default: ... }

This tends to be more resilient over time as you don't make binary choices anymore, but are forced to acknowledge existence of other values.

werft-gitpod-dev-com · 2022-09-15T07:37:08Z

started the job as gitpod-build-se-cost-center-next-billing.11 because the annotations in the pull request description changed
(with .werft/ from main)

svenefftinge · 2022-09-15T11:11:53Z

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-se-cost-center-next-billing.14
(with .werft/ from main)

svenefftinge · 2022-09-15T12:19:32Z

/werft run

👍 started the job as gitpod-build-se-cost-center-next-billing.17
(with .werft/ from main)

svenefftinge · 2022-09-15T14:21:37Z

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-se-cost-center-next-billing.20
(with .werft/ from main)

svenefftinge · 2022-09-15T14:48:58Z

/unhold

mrsimonemms

/hold

See comment for reasons for the hold. Happy to discuss further if necessary

mrsimonemms · 2022-09-15T16:10:16Z

 	Enabled                          bool               `json:"enabled"`
 	Schedule                         string             `json:"schedule"`
 	BillInstancesAfter               *time.Time         `json:"billInstancesAfter"`
+	DefaultSpendingLimit             map[string]int32   `json:"defaultSpendingLimit"`


Instead of using a map, my preference would be for this to use the DefaultSpendingLimit struct.

As this is in the experimental section, I'll approve this on the basis that it's not for public usage, but it would keep with the consistency of using structs in the configuration

svenefftinge · 2022-09-15T17:17:44Z

/unhold

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed size/XXL labels Sep 13, 2022

svenefftinge force-pushed the se/cost-center-next-billing branch from b5bf52b to 6e9c65e Compare September 13, 2022 12:41

roboquat added size/L and removed size/XXL labels Sep 13, 2022

svenefftinge closed this Sep 13, 2022

svenefftinge force-pushed the se/cost-center-next-billing branch from 6e9c65e to e3ed2c6 Compare September 13, 2022 14:51

roboquat added size/XS and removed size/L labels Sep 13, 2022

svenefftinge reopened this Sep 13, 2022

roboquat added size/L and removed size/XS labels Sep 13, 2022

svenefftinge changed the title ~~Se/cost-center-next-billing~~ [usage] Cost centers get nextBillingTime Sep 13, 2022

roboquat added release-note-none and removed do-not-merge/release-note-label-needed labels Sep 13, 2022

svenefftinge force-pushed the se/cost-center-next-billing branch 2 times, most recently from ab2cfcb to 23a3f31 Compare September 13, 2022 15:10

svenefftinge marked this pull request as ready for review September 13, 2022 15:10

svenefftinge requested a review from a team September 13, 2022 15:10

roboquat removed the do-not-merge/work-in-progress label Sep 13, 2022

github-actions Bot added the team: webapp Issue belongs to the WebApp team label Sep 13, 2022

svenefftinge force-pushed the se/cost-center-next-billing branch from 23a3f31 to dedd727 Compare September 13, 2022 15:55

svenefftinge requested a review from a team September 13, 2022 15:55

github-actions Bot added the team: delivery Issue belongs to the self-hosted team label Sep 13, 2022

easyCZ reviewed Sep 14, 2022

View reviewed changes

svenefftinge force-pushed the se/cost-center-next-billing branch from dedd727 to cda8e7a Compare September 14, 2022 18:47

roboquat added size/XL and removed size/L labels Sep 14, 2022

svenefftinge force-pushed the se/cost-center-next-billing branch from cda8e7a to f78a64d Compare September 15, 2022 06:39

easyCZ approved these changes Sep 15, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Sep 15, 2022

svenefftinge force-pushed the se/cost-center-next-billing branch from f78a64d to 58dd497 Compare September 15, 2022 07:31

svenefftinge force-pushed the se/cost-center-next-billing branch from 58dd497 to 39790df Compare September 15, 2022 09:16

roboquat added size/XXL and removed size/XL labels Sep 15, 2022

svenefftinge force-pushed the se/cost-center-next-billing branch from 39790df to 89bb051 Compare September 15, 2022 11:00

svenefftinge force-pushed the se/cost-center-next-billing branch 2 times, most recently from 7642da9 to aa21d77 Compare September 15, 2022 11:57

svenefftinge force-pushed the se/cost-center-next-billing branch 2 times, most recently from f3b9967 to 05c183e Compare September 15, 2022 14:14

roboquat removed the do-not-merge/hold label Sep 15, 2022

mrsimonemms approved these changes Sep 15, 2022

View reviewed changes

roboquat added the do-not-merge/hold label Sep 15, 2022

[usage] added costcenter nextBillingTime

51b0fd5

svenefftinge force-pushed the se/cost-center-next-billing branch from 05c183e to 51b0fd5 Compare September 15, 2022 16:23

roboquat removed the do-not-merge/hold label Sep 15, 2022

roboquat merged commit 2701ec7 into main Sep 15, 2022

roboquat deleted the se/cost-center-next-billing branch September 15, 2022 17:18

roboquat added the deployed: webapp Meta team change is running in production label Sep 16, 2022

		"defaultSpendingLimitForUsers": 5000,
		"defaultSpendingLimitForTeams": 0,

		DefaultSpendingLimitForUsers int32 `json:"defaultSpendingLimitForUsers,omitempty"`

		DefaultSpendingLimitForTeams int32 `json:"defaultSpendingLimitForTeams,omitempty"`


		Server *baseserver.Configuration `json:"server,omitempty"`

		apiv1.UsageConfig

	func TestGetCostCenter(t *testing.T) {
	func TestCostCenterManager_GetOrCreateCostCenter(t *testing.T) {

	func TestSaveCostCenter(t *testing.T) {
	func TestCostCenterManager_SaveCostCenter(t *testing.T) {

	func (c *CostCenterManager) SaveCostCenter(ctx context.Context, costCenter CostCenter) (CostCenter, error) {
	func (c *CostCenterManager) UpdateCostCenter(ctx context.Context, costCenter CostCenter) (CostCenter, error) {

	func (c *CostCenterManager) ComputeFinalization(ctx context.Context, costCenter CostCenter) (Usage, error) {
	func (c *CostCenterManager) ComputeFinalization(ctx context.Context, costCenter CostCenter) (Usage, error) {

		func cleanUp(t testing.T, conn gorm.DB, attributionIds ...db.AttributionID) {
		t.Cleanup(func() {

Uh oh!

Conversation

svenefftinge commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

Uh oh!

werft-gitpod-dev-com Bot commented Sep 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

easyCZ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

werft-gitpod-dev-com Bot commented Sep 15, 2022

Uh oh!

svenefftinge commented Sep 15, 2022 • edited by werft-gitpod-dev-com Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svenefftinge commented Sep 15, 2022 • edited by werft-gitpod-dev-com Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svenefftinge commented Sep 15, 2022 • edited by werft-gitpod-dev-com Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svenefftinge commented Sep 15, 2022

Uh oh!

mrsimonemms left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svenefftinge commented Sep 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

svenefftinge commented Sep 13, 2022 •

edited

Loading

svenefftinge commented Sep 15, 2022 •

edited by werft-gitpod-dev-com Bot

Loading

svenefftinge commented Sep 15, 2022 •

edited by werft-gitpod-dev-com Bot

Loading

svenefftinge commented Sep 15, 2022 •

edited by werft-gitpod-dev-com Bot

Loading