-
Notifications
You must be signed in to change notification settings - Fork 0
DT-523 add vpc s3 endpoint resource #28
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
Conversation
Deployed the changes. ❯ terraform state list | grep vpc_endpoint deployment-team-dev 12:37:25
module.comet_vpc[0].aws_vpc_endpoint.s3 |
Endpoint exists
Verify route table entries
|
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.
Pull Request Overview
Adds a gateway VPC endpoint for S3 and wires in a new region
variable, while updating default ElastiCache versions for Redis 7.
- Update default Redis engine version and parameter group to v7
- Introduce
region
variable in the VPC module - Provision an
aws_vpc_endpoint
for S3 and passregion
through the root module
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
variables.tf | Bump ElastiCache engine version and param group |
modules/comet_vpc/variables.tf | Add required region variable |
modules/comet_vpc/main.tf | Add aws_vpc_endpoint resource for S3 |
main.tf | Pass var.region into the VPC module |
Files not reviewed (1)
- .terraform.lock.hcl: Language not supported
Comments suppressed due to low confidence (2)
modules/comet_vpc/main.tf:44
- [nitpick] The resource name
s3
is ambiguous; consider renaming it tos3_endpoint
for clarity and consistency with other endpoint resources.
resource "aws_vpc_endpoint" "s3" {
variables.tf:261
- The default ElastiCache engine version was changed from "7.1.0" to "7.1"—please verify that AWS accepts this shorter format and update the variable description or docs if necessary.
default = "7.1"
|
||
variable "region" { | ||
description = "AWS region to provision resources in" | ||
type = string |
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.
[nitpick] Consider adding a validation block to the region
variable to enforce only supported AWS region identifiers (e.g., us-east-1
, eu-west-1
).
type = string | |
type = string | |
validation { | |
condition = can(regex("^([a-z]{2}-[a-z]+-[0-9]{1})$", var.region)) | |
error_message = "The region must be a valid AWS region identifier, e.g., 'us-east-1', 'eu-west-1'." | |
} |
Copilot uses AI. Check for mistakes.
|
||
resource "aws_vpc_endpoint" "s3" { | ||
vpc_id = module.vpc.vpc_id | ||
service_name = "com.amazonaws.${var.region}.s3" |
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.
Hardcoding the partition as com.amazonaws
may break in GovCloud or China partitions—use the data.aws_partition
source to build the service name dynamically.
Copilot uses AI. Check for mistakes.
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.
I approve. This looks fine. You may want to consider incorporating this submodule: https://registry.terraform.io/modules/terraform-aws-modules/vpc/aws/latest/submodules/vpc-endpoints
https://comet-ml.atlassian.net/browse/DT-523