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: Update lambda module and bump Terraform/AWS provider versions #151

Merged
merged 2 commits into from
Dec 9, 2021
Merged

feat: Update lambda module and bump Terraform/AWS provider versions #151

merged 2 commits into from
Dec 9, 2021

Conversation

bryantbiggs
Copy link
Member

Description

  • Update underlying lambda module to latest v2.x which removes this_ prefix
  • Bump min supported Terraform and AWS provider versions to match that of the lambda module (least common denominator)
  • Add new workflow file stub for unit tests. This is so that on the next PR the workflow file will be run (they won't run on the first PR introducing them due to security concerns protected by GitHub). This will be used to add in some unit tests for the actual lambda function python code
  • Remove the zip file and add a pattern match to the gitignore file - no longer required
  • Update pre-commit to latest

Motivation and Context

  • Keep module up to date with other modules
  • Prepping project for unit tests for better test integration on pull requests

Breaking Changes

  • No

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects
    • Tested with the examples/notify-slack-simple

@bryantbiggs
feat: update lambda module to latest and bump versions required to match
180e32a 
- Update underlying lambda module to latest v2.x which removes `this_` prefix
- Bump min supported Terraform and AWS provider versions to match that of the lambda module (least common denominator)
- Add new workflow file stub for unit tests. This is so that on the next PR the workflow file will be run (they won't run on the first PR introducing them due to security concerns protected by GitHub). This will be used to add in some unit tests for the actual lambda function python code
- Remove the zip file and add a pattern match to the gitignore file - no longer required
- Update pre-commit to latest
antonbabenko
antonbabenko approved these changes Dec 9, 2021
View reviewed changes
main.tf
@@ -109,7 +108,7 @@ module "lambda" {
# the value of presense of KMS. Famous "computed values in count" bug...
attach_cloudwatch_logs_policy = false
attach_policy_json = true
policy_json = element(concat(data.aws_iam_policy_document.lambda[*].json, [""]), 0)
policy_json = try(data.aws_iam_policy_document.lambda[0].json, "")
Copy link
Member

Choose a reason for hiding this comment

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

try() 🤣

Copy link
Member Author

Choose a reason for hiding this comment

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

my best friend ... try() 🤣

@antonbabenko antonbabenko changed the title feat: update lambda module to latest and bump Terraform/AWS provider versions required to match feat: Update lambda module and bump Terraform/AWS provider versions Dec 9, 2021
@antonbabenko antonbabenko merged commit 0a1fae8 into terraform-aws-modules:master Dec 9, 2021
antonbabenko pushed a commit that referenced this pull request Dec 9, 2021
@semantic-release-bot
chore(release): version 4.20.0 [skip ci]
e647784 
# [4.20.0](v4.19.0...v4.20.0) (2021-12-09)

### Features

* Update lambda module and bump Terraform/AWS provider versions ([#151](#151)) ([0a1fae8](0a1fae8))
@antonbabenko
Copy link
Member

This PR is included in version 4.20.0 🎉

@bryantbiggs bryantbiggs deleted the feature/lambda-module-update branch December 9, 2021 15:28
llamahunter
llamahunter reviewed Dec 10, 2021
View reviewed changes
Copy link

@llamahunter llamahunter left a comment

Choose a reason for hiding this comment

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

breaking changes should have major version number increments.

outputs.tf
@@ -1,4 +1,4 @@
output "this_slack_topic_arn" {
output "slack_topic_arn" {
Copy link

Choose a reason for hiding this comment

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

These are all breaking changes. You should increase the major version number.

@bryantbiggs
Copy link
Member Author

@llamahunter what breaking change are you seeing?

@llamahunter
Copy link

@llamahunter what breaking change are you seeing?

All the names of the outputs changed. Any terraform using this module that referenced those outputs broke with this change.

@bryantbiggs
Copy link
Member Author

there was one output changed this_slack_topic_arn => slack_topic_arn and in terms of Terraform, I wouldn't call it breaking because there are no destructive state changes. is there a small change required on the users end; yes. in hindsight, we probably could have left it in the codebase with a TODO for the next pending breaking change - but in the end it was a small change that is not destructive to provisioned resources

@bryantbiggs bryantbiggs mentioned this pull request Dec 11, 2021
Closed
@llamahunter
Copy link

It's destructive to existing builds. They all break when using ~>4.0 version matching, meaning they were pinning their module usage to backwards compatible API changes only, and only electing to move to a new incompatible version that requires code changes due to API deprecation in a controlled manner.

@laupow
Copy link

laupow commented Dec 13, 2021

I wouldn't call it breaking because there are no destructive state changes

@bryantbiggs Unfortunately, that isn't the whole story. The breaking change was the module output name change.

For example, these errors showed up after running with 4.20.0 +
Screen Shot 2021-12-13 at 3 19 43 PM

Can you at least add a note about renamed variables to the changelog? There's nothing in the changelog that indicates outputs were renamed.

@llamahunter
Copy link

Technically, per SemVer, they should publish a new 4.x change that rolls back the non-backwards compatible output name change, and label all intervening 4.x builds from 4.20.0 until the fix version as 'dangerous' to use due to the backwards incompatibility.

@ssp5zone
Copy link

This broke our builds! Had to give a patch fix.
Please avoid making such changes in the minor release.

image

@antonbabenko
Copy link
Member

Fixed in #159 , released v4.24.0.

We will remove this_slack_topic_arn in the upcoming major release v5.

Once more, apologies that it broke some builds. It was never our intention to do so.

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

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 Nov 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@llamahunter llamahunter llamahunter left review comments

@antonbabenko antonbabenko antonbabenko approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@bryantbiggs @antonbabenko @llamahunter @laupow @ssp5zone