-
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 AWS support to up spaces billing get
#347
Conversation
6ded32b
to
a9b5468
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.
Thanks @nlinx! I left some comments, I think there might be bugs regarding storage queries and download concurrency. But they look quite fixable, this looks close.
cmd/up/space/billing/get.go
Outdated
@@ -168,7 +174,7 @@ func (c *getCmd) cleanupOnError() { | |||
} | |||
} | |||
|
|||
func (c *getCmd) collectReport() error { | |||
func (c *getCmd) collectReport() error { //nolint:gocyclo |
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.
Could this method be refactored so that it isn't hitting the cyclomatic complexity limit? The switch
or its cases look like a juicy target for factoring out into a separate method.
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.
Bump, let's refactor this method instead of silencing the gocyclo linter on it.
ef3cf6e
to
fdc3b15
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.
This LGTM, there's just one lint error that I think we should address rather than silencing it.
cmd/up/space/billing/get.go
Outdated
@@ -168,7 +174,7 @@ func (c *getCmd) cleanupOnError() { | |||
} | |||
} | |||
|
|||
func (c *getCmd) collectReport() error { | |||
func (c *getCmd) collectReport() error { //nolint:gocyclo |
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.
Bump, let's refactor this method instead of silencing the gocyclo linter on it.
internal/usage/report/gcs/gcs.go
Outdated
@@ -39,10 +41,27 @@ const ( | |||
errWriteEvents = "error writing events" | |||
) | |||
|
|||
// MaxResourceCountPerGVKPerMCP reads usage data for an account and time range | |||
// GenerateReport |
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: Incomplete docstring
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.
lol that's what i get for stepping away to eat while writing doc string
@@ -40,10 +41,29 @@ const ( | |||
errWriteEvents = "error writing events" | |||
) | |||
|
|||
// MaxResourceCountPerGVKPerMCP reads usage data for an account and time range | |||
func GenerateReport(ctx context.Context, account, endpoint, bucket string, billingPeriod usage.TimeRange, w report.MCPGVKEventWriter) 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.
nit: No docstring
internal/usage/report/gcs/gcs.go
Outdated
"cloud.google.com/go/storage" | ||
|
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: cloud.google.com/go/storage
should be grouped with the other third-party imports below.
Description of your changes
up spaces billing get
Fixes #
https://github.com/upbound/mxe/issues/143
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