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

Adds Bucket Inventory. Default False. #47

Merged
merged 12 commits into from Jan 13, 2020
Merged

Adds Bucket Inventory. Default False. #47

merged 12 commits into from Jan 13, 2020

Conversation

@esacteksab
Copy link
Contributor

esacteksab commented Jan 7, 2020

Adds bucket inventory to the existing bucket.

@esacteksab esacteksab requested review from dynamike, jsclarridge and kilbergr Jan 7, 2020
This reverts commit 8e44e6d.
@esacteksab esacteksab mentioned this pull request Jan 8, 2020
main.tf Outdated Show resolved Hide resolved
@esacteksab esacteksab requested a review from jsclarridge Jan 9, 2020
main.tf Outdated
included_object_versions = "All"

schedule {
frequency = "Weekly"

This comment has been minimized.

Copy link
@dynamike

dynamike Jan 9, 2020

Contributor

I think it makes sense to support both daily and weekly updates to the inventory, but default to weekly.

main.tf Outdated

destination {
bucket {
format = "ORC"

This comment has been minimized.

Copy link
@dynamike

dynamike Jan 9, 2020

Contributor

I suspect it would be good to make this conditional as well. This format depends on what would be consuming this inventory file. My guess is Amazon Athena is the most likely answer. They recommends ORC or Parquet as for performance reasons. So maybe make one of those the default.

@@ -65,3 +76,26 @@ resource "aws_s3_bucket_public_access_block" "public_access_block" {
restrict_public_buckets = true
}

resource "aws_s3_bucket_inventory" "inventory" {

This comment has been minimized.

Copy link
@dynamike

dynamike Jan 9, 2020

Contributor

Does it make sense to default encryption with sse_s3 ?

This comment has been minimized.

Copy link
@esacteksab

esacteksab Jan 9, 2020

Author Contributor

The bucket is encrypted. Do we need to encrypt the file?

This comment has been minimized.

Copy link
@dynamike

dynamike Jan 10, 2020

Contributor

yeah, nevermind :)

esacteksab added 5 commits Jan 9, 2020
main.tf Outdated Show resolved Hide resolved
@@ -65,3 +76,26 @@ resource "aws_s3_bucket_public_access_block" "public_access_block" {
restrict_public_buckets = true
}

resource "aws_s3_bucket_inventory" "inventory" {

This comment has been minimized.

Copy link
@dynamike

dynamike Jan 10, 2020

Contributor

yeah, nevermind :)

main.tf Outdated Show resolved Hide resolved
@dynamike dynamike requested a review from chrisgilmerproj Jan 10, 2020
esacteksab and others added 2 commits Jan 10, 2020
Co-Authored-By: Michael Kania <dynamike@truss.works>
Co-Authored-By: Michael Kania <dynamike@truss.works>
@esacteksab esacteksab requested a review from dynamike Jan 10, 2020
esacteksab added 2 commits Jan 10, 2020
Copy link
Contributor

chrisgilmerproj left a comment

🚀

@esacteksab esacteksab merged commit e8f1875 into master Jan 13, 2020
2 checks passed
2 checks passed
ci/circleci: terratest Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
@esacteksab esacteksab deleted the barry-s3-inventory branch Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.