-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add up space billing get
with GCS support
#345
Conversation
10e1de8
to
b386aaa
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.
Couple of suggestions, but nothing major. I do think that using an interface for the cloud providers will make life marginally easier, but not in the critical path for delivery.
|
||
// UsageQuery() returns a query for usage data for an Upbound account across a | ||
// range of time. startTime is inclusive and endTime is exclusive to the hour. | ||
func UsageQuery(account string, startTime, endTime time.Time) (*storage.Query, error) { |
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.
WDTH about use a CloudProvider
interface instead? Since you're going to have to implement AWS and Azure as well the interface pattern could make things easier.
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.
Totally agreed, I have a related TODO here: https://github.com/upbound/up/pull/345/files#diff-827c313d3f294c17efea02ef7099b048dd5362f8de24a14cc9829fab195dd87cR46-R47
I'm not sure what that interface should look like yet, so I was planning on feeling it out when I add support for a second storage provider. I foresee this clientutil/gcs
package eventually being used by the GCS implementation of that interface.
513cf2a
to
fa1e316
Compare
Co-authored-by: Ben Howard <1087475+darkmuggle@users.noreply.github.com>
fa1e316
to
16c52af
Compare
It may be nice to put the date in the tarball. For someone running this multiple times (e.g. three monthly reports for the quarter), it would be nice not to have to rename the file. This is a small suggestion -- it could happen in a future revision. |
@@ -0,0 +1,29 @@ | |||
The storage location for the billing data used to create the report is supplied |
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 haven't seen help text done like this in the CLI -- this is great!
Branden -- I read through the code. Overall, it's super clear and well tested. Thank you! That said, I'm not entirely clear on what the output is. From the sample output above, it looks like we're getting a "max usage" for the time period specified by the user. Is that right? I thought we were going for total usage. So if someone has an EKS cluster for 30 days, they'd have 720 loop-hours of usage. Is that what you're producing, or are you producing "one EKS cluster"? |
@AlainRoy The output is a series of events covering the billing period, with each event recording the maximum observed count of a GVK on an MCP within a given hour. This was what product requested, I'll follow up offline. |
So in my example, we'd have 720 events, each saying "1 EKS cluster"? |
For the record, Branden and I talked and we're happy. We have an idea to add a summary of the time period in a separate file, but we can do that in the future -- no need to do it now. |
Description of your changes
This adds an
up
subcommandspace billing get
for getting a billing report from an object storage API, with initial support for GCS. Support for AWS and Azure will be added in followup PRs.I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR, as appropriate.How has this code been tested