-
Notifications
You must be signed in to change notification settings - Fork 0
PLT-1224 #322
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
base: main
Are you sure you want to change the base?
PLT-1224 #322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously, we don't use the hcl
suffix on the other backends, so this is non-standard. However, this would be a nice touch to enable native highlighting/formatting in IDEs, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our backends use the *.s3.tfbackend
naming convention as recommended at https://developer.hashicorp.com/terraform/language/backend#file in order to hopefully look familiar to any engineer coming from other terraform projects. I'm not wedded to anything in the HashiCorp docs, however, and I like giving the editor hints for file formatting. Also, *.s3.hcl
is shorter, which is always a plus.
module "bucket_key" { | ||
source = "../key" | ||
name = "${var.name}-bucket" | ||
description = "For ${var.name} S3 bucket and its access logs" | ||
user_roles = var.cross_account_read_roles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change probably shouldn't be accepted until other module declaration sources of the modules/bucket
child are pinned to existing references.
data "aws_ssm_parameters_by_path" "cdap" { | ||
path = "/cdap" | ||
recursive = true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the aws_ssm_parameters_by_path
data source allows for empty results. Paired with the call to the zipmap()
in local.cdap_ssm
and the lookup()
on the same for local.access_logs_bucket
, this helps us avoid failures on missing configuration and obviates the need for hard-coding bucket-access-logs buckets.
output "region_name" { | ||
description = "**Deprecated**. Use `primary_region.name`. The region name associated with the current caller identity" | ||
sensitive = false | ||
value = data.aws_region.this.name | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change.
As noted, this was already deprecated in favor of the primary_region
output. With updates to aws provider version 6, data.aws_region
has a deprecated name
and an attribute, in favor of region
, so... references like this would be most correct as data.aws_region.this.region
. ¯\_(ツ)_/¯
output "account_id" { | ||
description = "The AWS account ID associated with the current caller identity" | ||
description = "Deprecated. Use `aws_caller_identity.account_id`. The AWS account ID associated with the current caller identity" | ||
sensitive = true | ||
value = data.aws_caller_identity.this.account_id | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecating account_id
to match how the platform module surfaces this. It might be interesting to start using the opentofu-specific deprecation
fields once we're confident that all terraform is executed under opentofu....
🎫 Ticket
https://jira.cms.gov/browse/PLT-1224
🛠 Changes
ℹ️ Context
🧪 Validation