-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
Drop support for deprecated features, revamp VPC Endpoint support #112
Conversation
/test all |
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.
Looks good and well documented. One small comment, no fix really, just how I read it
|
||
You will need to add `route_table_ids` to `gateway_vpc_endpoints`, but it can be an empty list. | ||
|
||
Terraform plan may show changes, but they should not have any effect. |
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 line is somewhat concerning Should not have any effect
@@ -19,11 +19,11 @@ output "vpc_id" { | |||
} | |||
|
|||
output "gateway_vpc_endpoints" { | |||
value = module.vpc_endpoints.gateway_vpc_endpoints | |||
value = module.vpc_endpoints.gateway_vpc_endpoints_map | |||
description = "List of Gateway VPC Endpoints deployed to the VPC." | |||
} |
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'd update the description to
description = "Gateway VPC Endpoints deployed to the VPC."
but we can do it in a follow up OR since we have to update this anyway
source = "cloudposse/security-group/aws"
version = "2.0.0-rc1"
description = "List of Gateway VPC Endpoints deployed to the VPC." | ||
} | ||
|
||
output "interface_vpc_endpoints" { | ||
value = module.vpc_endpoints.interface_vpc_endpoints | ||
value = module.vpc_endpoints.interface_vpc_endpoints_map | ||
description = "List of Interface VPC Endpoints deployed to the VPC." |
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.
description = "Interface VPC Endpoints deployed to the VPC."
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.
lgtm, a few nitpicks which can be addressed in follow up PRs
Note
This will be released as version 2.0.0-rc1 and possibly as 2.0.0 without changes.
See migration notes for details.
what
modules/vpc-endpoints
)why
references