feat: Added configuration options to replace security groups on destroy of Lambda function

terraform-aws-modules / terraform-aws-lambda

Terraform module, which takes care of a lot of AWS Lambda/serverless tasks (build dependencies, packages, updates, deployments) in countless combinations 🇺🇦

https://registry.terraform.io/modules/terraform-aws-modules/lambda/aws

Apache License 2.0

886 stars 657 forks source link

feat: Added configuration options to replace security groups on destroy of Lambda function #422

Closed ricoli closed 1 year ago

ricoli commented 1 year ago

Description

Add support for the newly introduced replace_security_groups_on_destroy and replacement_security_group_ids lambda function arguments in v4.54.0

Motivation and Context

Implements https://github.com/terraform-aws-modules/terraform-aws-lambda/issues/418

Breaking Changes

None

How Has This Been Tested?

[x] I have updated at least one of the examples/* to demonstrate and validate my change(s)
[x] I have tested and validated these changes using one or more of the provided examples/* projects
[x] I have executed pre-commit run -a on my pull request

antonbabenko commented 1 year ago

This PR is included in version 4.12.0 :tada:

nils-at-slashwhy commented 1 year ago

Hi Ricardo and Anton. Are you sure that you introduced this feature like intended?

The docs of the lambda module say that the attributes replace_security_groups_on_destroy and replacement_security_group_ids are optional.

But when I use the new version of the module, without changing my terraform code, my terraform deploy fails:

Error: Missing required argument ... │ "replacement_security_group_ids": all of │ replace_security_groups_on_destroy,replacement_security_group_ids must be │ specified

antonbabenko commented 1 year ago

@nils-at-slashwhy #433 is already about it. I will look at it during the next couple of hours and fix it.

nils-at-slashwhy commented 1 year ago

Great! Thank you very much :)

ricoli commented 1 year ago

Hi Ricardo and Anton. Are you sure that you introduced this feature like intended?

The docs of the lambda module say that the attributes replace_security_groups_on_destroy and replacement_security_group_ids are optional.

But when I use the new version of the module, without changing my terraform code, my terraform deploy fails:

Error: Missing required argument ... │ "replacement_security_group_ids": all of │ replace_security_groups_on_destroy,replacement_security_group_ids must be │ specified

Oh no, I wonder if replacement_security_group_ids should also be null as default then as to not trigger this validation?

antonbabenko commented 1 year ago

@ricoli Probably. Please try it if you have time.

ricoli commented 1 year ago

@ricoli Probably. Please try it if you have time.

I'm on it - sorry about the trouble! I should have tested it again after changing the default value from false to null on the other variable...

ricoli commented 1 year ago

@ricoli Probably. Please try it if you have time.

I'm on it - sorry about the trouble!

Indeed switching the default of replacement_security_group_ids to null prevents the argument validation from firing, PR is at https://github.com/terraform-aws-modules/terraform-aws-lambda/pull/434

github-actions[bot] commented 1 year ago

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.