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

Refactor to use s3-bucket module, update in general #66

Merged
merged 5 commits into from
Feb 23, 2022
Merged

Conversation

Nuru
Copy link
Contributor

@Nuru Nuru commented Feb 22, 2022

what

why

  • Simplify maintenance and standardize on single S3 bucket module, in preparation for upgrade to Terraform AWS provider v4
  • With Terraform AWS provider v4, having mfa_delete enabled requires entering an MFA token for every Terraform operation, which is incompatible with automation. Users requiring mfa_delete should either not use Terraform or create their own fork.
  • Current module does not work with AWS v4, but Renovate would try to update it anyway
  • Stay current with boilerplate and management tools

notes

This is the first of 2 upgrade releases to get this module to support Terraform AWS Provider v4. We are breaking it into 2 releases so that users have the option of upgrading step-by-step rather than all at once. Upgrade instructions are here.

@Nuru Nuru requested review from a team as code owners February 22, 2022 22:49
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

⚠️   Due to fdfbfef - Auto Format - 1 new error was added

Change details

Error ID Change Path Resource
BC_VUL_2 Added /test/src/go.sum undefined

@Nuru
Copy link
Contributor Author

Nuru commented Feb 22, 2022

/test all

@Nuru
Copy link
Contributor Author

Nuru commented Feb 22, 2022

/test all

@Nuru
Copy link
Contributor Author

Nuru commented Feb 23, 2022

/test all

@Nuru Nuru added the enhancement New feature or request label Feb 23, 2022
@Nuru Nuru requested review from aknysh, korenyoni, nitrocode and osterman and removed request for joe-niland and dylanbannon February 23, 2022 00:44

required_providers {
aws = {
source = "hashicorp/aws"
version = ">= 3.0"
version = ">= 3.0, < 4.0"
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand. I thought we weren't going to pin provider versions to major releases.

Also, this is more concise, no ?

Suggested change
version = ">= 3.0, < 4.0"
version = "~> 3.0"

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok, so 2 separate releases interesting.

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 is a temporary change to provide a stepping stone to the next version, where the version will be pinned at >= 4.2.

}

data "aws_caller_identity" "current" {}
data "aws_caller_identity" "current" { count = local.enabled ? 1 : 0 }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
data "aws_caller_identity" "current" { count = local.enabled ? 1 : 0 }
data "aws_caller_identity" "current" {
count = local.enabled ? 1 : 0
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like 1-liners for trivial stuff like this, and terraform fmt approves.

@Nuru Nuru requested a review from nitrocode February 23, 2022 02:22
Copy link
Member

@korenyoni korenyoni left a comment

Choose a reason for hiding this comment

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

LGTM but some nitpicks

default = 30
description = "Specifies when noncurrent object versions transitions"
default = 90
description = "Specifies when non-current object versions expire"
Copy link
Member

Choose a reason for hiding this comment

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

Nit

Suggested change
description = "Specifies when non-current object versions expire"
description = "Specifies when non-current object versions expire (in days)"

variable "noncurrent_version_transition_days" {
type = number
default = 30
description = "Specifies when noncurrent object versions transition to Glacier Flexible Retrieval"
Copy link
Member

Choose a reason for hiding this comment

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

Nit

Suggested change
description = "Specifies when noncurrent object versions transition to Glacier Flexible Retrieval"
description = "Specifies when noncurrent object versions transition to Glacier Flexible Retrieval (in days)"

@Nuru Nuru self-assigned this Feb 23, 2022
@Nuru Nuru merged commit 048ae6a into master Feb 23, 2022
@Nuru Nuru deleted the s3-refactor branch February 23, 2022 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants