-
Notifications
You must be signed in to change notification settings - Fork 133
Adds Carbon Report Export #827
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
Adds Carbon Report Export #827
Conversation
|
@iamkirkbater: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/label tide/merge-method-squash |
| } | ||
|
|
||
| // validateAccount validates that the account is a number with at least 12 digits | ||
| func (o *carbonReportOptions) validateAccount() 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.
I could go either way with this check, but seems not necessary.
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.
My main concern when adding this check is ensuring that we're passing a valid AWS Account ID - specifically if there are leading 0s in the case of copy/paste errors from excel/sheets or if someone running this tries to pass a cluster ID then it would fail.
Claude did it's thing and added all of the extra validation, but since it's AWS Account IDs I figured it would be fine.
| "payer_account_id": true, | ||
| "usage_account_id": true, |
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.
Without knowing the data, is it better to have an includedColumns map?
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 could go either way. There's a CSV in https://issues.redhat.com/browse/OHSS-34489 that is the output of this command if you want to take a look at what else is there. In this case I think I'd rather be permissive than be overly exclusive, but I could be convinced otherwise.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iamkirkbater, joshbranham The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This command exports Carbon Reporting data for a given Red Hat Owned AWS Account. We have customers who wish to obtain this data from their OSD clusters, however this is not published at an account level when an account is a child account, so we cannot expose this as self-service for these customers.
This command allows an SRE to export carbon reporting data for a given account across a given usage period to a CSV file via stdout.
Example Usage:
osdctl cost carbon-report --account [aws account id] --usage-period 2025 > /tmp/emissions.csvThis will go into the carbon emissions S3 bucket, loop through all 2025 usage period reports, and gather only the rows for a given account id, and then publish them to stdout which we can then pipe to a file.