feat!: Added support for date-based partitioning. Bump AWS provider version to 5.x and Terraform to 1.0

terraform-aws-modules / terraform-aws-s3-bucket

Terraform module to create AWS S3 resources 🇺🇦

https://registry.terraform.io/modules/terraform-aws-modules/s3-bucket/aws

Apache License 2.0

526 stars 3.62k forks source link

feat!: Added support for date-based partitioning. Bump AWS provider version to 5.x and Terraform to 1.0 #268

Closed EreminAnton closed 10 months ago

EreminAnton commented 11 months ago

Description

Add optional target_object_key_format configuration block to support automatic date-based partitioning. This functionality was released in 5.27.0 version of AWS provider.

Motivation and Context

To leverage the new functionality of access logging.

Breaking Changes

As I understand it, this pins the provider's version to 5.27.0 but offers a significant update in access logs partitioning.

How Has This Been Tested?

[x] I have updated at least one of the examples/* to demonstrate and validate my change(s)
[x] I have tested and validated these changes using one or more of the provided examples/* projects I deployed every possible combination of the target_object_key_format configuration, placed objects, and waited until access logs arrived in the required format.
[x] I have executed pre-commit run -a on my pull request

dickc-sg commented 10 months ago

I was just about to submit a PR for this myself and then realized I had overlooked yours, @EreminAnton . I approached this a little differently:

partition_date_source is required if partitioned_prefix is declared and so I removed the try():

partition_date_source = target_object_key_format.value.partitioned_prefix["partition_date_source"]

I also chose different logic for the for_each in the simple_prefix dynamic that was more similar to the logic used other places in this module:

for_each = try([target_object_key_format.value.simple_prefix], [])

Those are the only differences that I'm aware of.

Testing my fork by generating plans seemed to work well. Errors, as expected, were thrown:

if both partitioned_prefix and simple_prefix were declared in logging input
if target_object_key_format was declared and empty

Otherwise, the plans came back successful. I am not sure which approach is best but thought I'd comment here for review.

undeadops commented 10 months ago

I was about to submit a patch for this as well, but thought I should look at open PRs first. Good thing I did, saved me the effort.

EreminAnton commented 10 months ago

Hello! I have updated versions.tf. However, the pre-commit Terraform wrapper with for_each in the module has resulted in numerous changes in the /wrappers directory. I'm unsure if this is the expected outcome.

antonbabenko commented 10 months ago

This PR is included in version 4.0.0 :tada:

github-actions[bot] commented 9 months ago

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.