Skip to content
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

test: pkg/aws/s3 #31

Merged
merged 3 commits into from
Dec 6, 2023
Merged

test: pkg/aws/s3 #31

merged 3 commits into from
Dec 6, 2023

Conversation

skl
Copy link
Contributor

@skl skl commented Dec 5, 2023

  • Introduce new CostExplorer interface in front of the real AWS Cost Explorer
  • Auto-generated mocks via mockery
  • Change all signatures to use CostExplorer instead of AWS *costexplorer.Client struct
  • Coverage of pkg/aws/s3: 78%

Copy link
Contributor

@Pokom Pokom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 With one nit, but naming is hard and I think this is great.

pkg/aws/costexplorer/costexplorer.go Outdated Show resolved Hide resolved
Comment on lines +274 to +277
if group.Keys == nil {
log.Printf("skipping group without keys")
continue
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a potential, if unlikely, panic.

Comment on lines +271 to +273
// TODO: region lookup failure
// TODO: test should fail
Keys: []string{"AWS GovCloud (US-East)-Requests-Tier1"},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may actually be a bug?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we don't have any govcloud instances so this is likely a bug. Do you have an example output of it failing?

@skl skl marked this pull request as ready for review December 5, 2023 19:56
Copy link
Contributor

@Pokom Pokom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, really happy to see where this is going. Thanks @skl!

@skl skl merged commit 3122d20 into main Dec 6, 2023
1 check passed
@skl skl deleted the skl/test-aws-s3 branch December 6, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants