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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Mar 10, 2023
Merged

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

merged 2 commits into from
Mar 10, 2023

Conversation

ricoli
Copy link
Contributor

@ricoli ricoli commented Feb 13, 2023 •
edited
Loading

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 #418

Breaking Changes

None

How Has This Been Tested?

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

@ricoli ricoli changed the title add config to replace security groups on destroy feat: add config to replace security groups on destroy Feb 13, 2023
@ricoli ricoli changed the title feat: add config to replace security groups on destroy feat: add configuration options to replace security groups on destroy Feb 13, 2023
@ricoli ricoli changed the title feat: add configuration options to replace security groups on destroy feat: Configuration options to replace security groups on destroy Feb 13, 2023
@ricoli ricoli mentioned this pull request Feb 13, 2023
Closed
antonbabenko
antonbabenko requested changes Mar 10, 2023
View reviewed changes
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

LGTM. Minor improvement and docs update.

variables.tf Outdated Show resolved Hide resolved
@ricoli ricoli force-pushed the replace-security-groups-on-destroy branch from 5053a97 to e21c20c Compare March 10, 2023 12:15
@antonbabenko antonbabenko changed the title feat: Configuration options to replace security groups on destroy feat: Added configuration options to replace security groups on destroy of Lambda function Mar 10, 2023
@antonbabenko antonbabenko merged commit 2d92236 into terraform-aws-modules:master Mar 10, 2023
antonbabenko pushed a commit that referenced this pull request Mar 10, 2023
@semantic-release-bot
chore(release): version 4.12.0 [skip ci]
54ca3a3 
## [4.12.0](v4.11.0...v4.12.0) (2023-03-10)

### Features

* Added configuration options to replace security groups on destroy of Lambda function ([#422](#422)) ([2d92236](2d92236))
@antonbabenko
Copy link
Member

This PR is included in version 4.12.0 🎉

@ricoli ricoli deleted the replace-security-groups-on-destroy branch March 10, 2023 13:05
@nils-at-slashwhy
Copy link

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
Copy link
Member

@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
Copy link

Great! Thank you very much :)

@ricoli
Copy link
Contributor Author

ricoli commented Mar 10, 2023

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
Copy link
Member

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

@ricoli
Copy link
Contributor Author

ricoli commented Mar 10, 2023 •
edited
Loading

@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 ricoli mentioned this pull request Mar 10, 2023
Merged
3 tasks
@ricoli
Copy link
Contributor Author

ricoli commented Mar 10, 2023

@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 #434

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@antonbabenko antonbabenko antonbabenko requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ricoli @antonbabenko @nils-at-slashwhy