[usage] Use stripe clients rather than a singleton in the usage controller #10854
Conversation
| type stripeKeys struct { | ||
| PublishableKey string `json:"publishableKey"` | ||
| SecretKey string `json:"secretKey"` | ||
| } | ||
|
|
||
| // Authenticate authenticates the Stripe client using a provided file containing a Stripe secret key. | ||
| func Authenticate(apiKeyFile string) error { | ||
| func Authenticate(apiKeyFile string) (*Client, error) { |
There was a problem hiding this comment.
I'd recommend passing in a struct with the credentials, instead of the Path. The path can be resolved during component initialization. For example, unit tests should need a path, just data.
There was a problem hiding this comment.
| func Authenticate(apiKeyFile string) (*Client, error) { | |
| func New(apiKeyFile string) (*Client, error) { |
So that you can do stripe.New(..)
| bytes, err := os.ReadFile(apiKeyFile) | ||
| if err != nil { | ||
| return err | ||
| return nil, err | ||
| } | ||
|
|
||
| var stripeKeys stripeKeys |
| } | ||
|
|
||
| stripe.Key = stripeKeys.SecretKey | ||
| return nil | ||
| sc := &client.API{} |
There was a problem hiding this comment.
Would client.New(key, backends) be more appropriate here?
|
started the job as gitpod-build-af-instantiate-stripe-client-instance.6 because the annotations in the pull request description changed |
I think we have a few options:
|
|
I think using Using a real account is not really an option for unit tests, they will run possibly in parallel (from multiple PRs) and we'd have a hard time ensuring they are not flaky. |
|
Thanks 👍 We'll add tests for the Stripe integration in a later PR. Could you approve if you are happy with these changes 🙏 . /hold |
|
I was waiting for the parent PR to land, otherwise it would've merged this one into the parent PR |
|
Thanks. I put |
| } | ||
|
|
||
| // UpdateUsage updates teams' Stripe subscriptions with usage data | ||
| // `usageForTeam` is a map from team name to total workspace seconds used within a billing period. | ||
| func UpdateUsage(usageForTeam map[string]int64) error { | ||
| func (c *Client) UpdateUsage(usageForTeam map[string]int64) error { |
There was a problem hiding this comment.
For follow-up, I'd recommend returnig a struct which contains a summary of the succesful and failed updates. That way, you can also better test that the right number of records got updated. For an example, see the Usage Reconciler
andrew-farries
force-pushed
the
af/instantiate-stripe-client-instance
branch
from
f71a674 to
a71fc50
Compare
|
/unhold |
`Authenticate` now returns a Client object rather than acting as a singleton. Change the `UpdateUsage` function to be a method on the client type. See: https://github.com/stripe/stripe-go#with-a-client
Give an instance of the stripe client to the stripe billing controller.
* Rename the function to `New`. * Lift config file unmarshaling out of the package and into the consumer.
andrew-farries
force-pushed
the
af/instantiate-stripe-client-instance
branch
from
a71fc50 to
5f9e5b6
Compare
roboquat
added
deployed: webapp
Description
Change the usage component's Stripe package to return client instances rather than using a Stripe singleton.
Related Issue(s)
#10754 (comment)
How to test
The usual test steps to set up usage based billing and generate a test invoice.
See eg #10801
Release Notes
Documentation
Werft options:
/hold