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

Fixed Deprecated aws_s3_bucket Options #266

Closed
wants to merge 6 commits into from

Conversation

afrazkhan
Copy link

The following options were deprecated, and have either been updated,
or replaced with the new necessary resources:

  • override_json replaced with override_policy_documents option
  • server_side_encryption_configuration replaced with aws_s3_bucket_server_side_encryption_configuration resource
  • versioning replaced with aws_s3_bucket_versioning resource
  • cors_rule replaced with aws_s3_bucket_cors_configuration resource
  • acl replaced with aws_s3_bucket_acl resource

  The following options were deprecated, and have either been updated,
  or replaced with the new necessary resources:

  * `override_json` replaced with `override_policy_documents` option
  * `server_side_encryption_configuration` replaced with `aws_s3_bucket_server_side_encryption_configuration` resource
  * `versioning` replaced with `aws_s3_bucket_versioning` resource
  * `cors_rule` replaced with `aws_s3_bucket_cors_configuration` resource
  * `acl` replaced with `aws_s3_bucket_acl` resource
@afrazkhan afrazkhan requested review from a team as code owners April 26, 2023 17:38
@afrazkhan afrazkhan requested review from dotCipher and joe-niland and removed request for a team April 26, 2023 17:38
@bshabae
Copy link

bshabae commented Sep 27, 2023

The following options were deprecated, and have either been updated, or replaced with the new necessary resources:

  • override_json replaced with override_policy_documents option
  • server_side_encryption_configuration replaced with aws_s3_bucket_server_side_encryption_configuration resource
  • versioning replaced with aws_s3_bucket_versioning resource
  • cors_rule replaced with aws_s3_bucket_cors_configuration resource
  • acl replaced with aws_s3_bucket_acl resource

Hi guys @dotCipher and @joe-niland!

Any updates on this? We are still seeing the following deprecation warning:

│ Warning: Argument is deprecated
│   on .terraform/modules/cloudfront_s3_site/main.tf line 248, in resource "aws_s3_bucket" "origin":
│  248: resource "aws_s3_bucket" "origin" {
│ 
│ Use the aws_s3_bucket_cors_configuration resource instead

@davidjeddy
Copy link

Bump, checking in on the progress of this patch.

@bshabae
Copy link

bshabae commented Jan 2, 2024

Bump, checking in on the progress of this patch.

Thank you! We are looking forward to this update.

@joe-niland
Copy link
Member

Thanks @afrazkhan and sorry for the delay in reviewing.

There are some formatting issues. Could you please run the following to see the output?

make precommit/terraform

I believe they can be fixed by running terraform fmt -recursive

@@ -480,6 +480,17 @@ variable "versioning_enabled" {
description = "When set to 'true' the s3 origin bucket will have versioning enabled"
}

variable "bucket_versioning" {
Copy link
Member

Choose a reason for hiding this comment

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

This effectively deprecates versioning_enabled. For backwards compatibility it should be mapped to the new variable with value "Enabled".

It would also be good to remove its use from the examples.

Copy link
Author

Choose a reason for hiding this comment

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

I might be misunderstanding, but since var.versioning_enabled is a bool, wouldn't I break backwards compatibility if I changed it to a string and set it's default to Enabled? Or do you mean to map it via something like doing some logic in locals?

Something like

locals {
  bucket_versioning = var.versioning_enabled || var.bucket_versioning == "Enabled" ? "Enabled" : "Disabled"
}

and then use local.bucket_versioning everywhere instead?

Copy link

mergify bot commented Mar 9, 2024

Thanks @afrazkhan for creating this pull request!

A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

While you wait, make sure to review our contributor guidelines.

Tip

Need help or want to ask for a PR review to be expedited?

Join us on Slack in the #pr-reviews channel.

@mergify mergify bot added the triage Needs triage label Mar 9, 2024
Copy link

mergify bot commented Mar 9, 2024

This pull request now has conflicts. Could you fix it @afrazkhan? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Mar 9, 2024
Copy link

mergify bot commented Mar 9, 2024

This PR has been closed due to inactivity and merge conflicts.
Please resolve the conflicts and reopen if necessary.

@mergify mergify bot closed this Mar 9, 2024
@mergify mergify bot removed conflict This PR has conflicts triage Needs triage labels Mar 9, 2024
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.

5 participants