feat: Add support for configuring expected_bucket_owner

rogeruiz commented 1 year ago

Description

The s3_settings example and main.tf in this module are missing the expected_bucket_owner optional attribute.

[x] Adding the expected_bucket_owner to s3_settings;
[x] Adding expected_bucket_owner to complete example

Motivation and Context

From the documentation found in the Terraform registry, I've noticed that the expected_bucket_owner optional argument is missing. My team and I have found that we are using this attribute to ensure that we're able to configure S3 Target DMS Endpoints to expect a specific bucket owner. Until this gets merged into the default branch, we will have to use a fork.

=> Documentation found in the Terraform registry around _Resource: aws_dms_s3endpoint

Breaking Changes

No breaking changes as leaving the optional property out will not cause any side effects.

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
- My team is doing this in a private repository that I cannot linked to. But as mentioned above, the logic of adding this property is documented in the official AWS Provider in the Terraform Registry.
[x] I have executed pre-commit run -a on my pull request

rogeruiz commented 1 year ago

I have executed the pre-commit run -a on my branch but I'm seeing unexpected output. Any help on this would be appreciated. Thank you! I'm not expecting this output as I have updated the examples/complete/main.tf and the main.tf in the repository.

>_ pre-commit run -a

Terraform fmt............................................................Passed
Terraform validate.......................................................Failed
- hook id: terraform_validate
- exit code: 1

Validation failed: .
╷
│ Error: Unsupported argument
│ 
│   on main.tf line 268, in resource "aws_dms_endpoint" "this":
│  268:       expected_bucket_owner             = lookup(s3_settings.value, "expected_bucket_owner", null)
│ 
│ An argument named "expected_bucket_owner" is not expected here.
╵

Validation failed: examples/complete
╷
│ Warning: Deprecated attribute
│ 
│   on .terraform/modules/msk_cluster/main.tf line 101, in resource "aws_msk_cluster" "this":
│  101:     ignore_changes = [broker_node_group_info[0].ebs_volume_size]
│ 
│ The attribute "ebs_volume_size" is deprecated. Refer to the provider
│ documentation for details.
╵
╷
│ Error: Unsupported argument
│ 
│   on ../../main.tf line 268, in resource "aws_dms_endpoint" "this":
│  268:       expected_bucket_owner             = lookup(s3_settings.value, "expected_bucket_owner", null)
│ 
│ An argument named "expected_bucket_owner" is not expected here.
╵
╷
│ Error: Unsupported argument
│ 
│   on ../../main.tf line 268, in resource "aws_dms_endpoint" "this":
│  268:       expected_bucket_owner             = lookup(s3_settings.value, "expected_bucket_owner", null)
│ 
│ An argument named "expected_bucket_owner" is not expected here.
╵
╷
│ Error: Unsupported argument
│ 
│   on ../../main.tf line 268, in resource "aws_dms_endpoint" "this":
│  268:       expected_bucket_owner             = lookup(s3_settings.value, "expected_bucket_owner", null)
│ 
│ An argument named "expected_bucket_owner" is not expected here.
╵

Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
check for merge conflicts................................................Passed
fix end of files.........................................................Passed

bryantbiggs commented 1 year ago

that attribute is only valid on the new resource https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/dms_s3_endpoint which is not yet added here

rogeruiz commented 1 year ago

Ah I see! Thank you @bryantbiggs! Does this mean this repository would need more work to update to that version of the provider?

This sounds like a much bigger PR knowing this, but I have time to implement that level of upgrade. I need this for work.

rogeruiz commented 1 year ago

Closing this as it is a much larger piece of work. @bryantbiggs let me know if there's a desire to support the new resource and I can find some time to add it to this module. Thanks!

bryantbiggs commented 1 year ago

yes - we def should look at adding it. I haven't had time to look at what it would take and if its a breaking change - but at minimum we should open an issue to track to get it added

rogeruiz commented 1 year ago

Awesome thanks @bryantbiggs I'll craft up an issue.

github-actions[bot] commented 1 year 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.

terraform-aws-modules / terraform-aws-dms