-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
resource/aws_lambda_function: Recompute Lambda function version and qualified_arn on publish #3032
resource/aws_lambda_function: Recompute Lambda function version and qualified_arn on publish #3032
Conversation
1f2539e
to
6ef7474
Compare
@apparentlymart I'm not sure how the labels are used, but I wouldn't consider this an enhancement. The current behavior without this change means that resources that depend on the version attribute do not correct updates when a new Lambda function version is published. Both of the issues that this fixes are labeled as bugs. |
aws/resource_aws_lambda_function.go
Outdated
} | ||
} | ||
|
||
func updateVerionIfPublish(d *schema.ResourceDiff, meta interface{}) error { | ||
publish := d.Get("publish").(bool) | ||
if publish && needsFunctionCodeUpdate(d) { |
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.
There are two other attributes that need to be SetNewComputed:
- last_modified - This need to be recomputed any time there is a change to the Lambda code, even if publish =false
- qualified_arn - This needs to be updated only if publish = true
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.
Thank you for the feedback. I implemented the changes for last_modified and qualified_arn. In this version I only updated last_modified during the publish, but I see your point about it being needed more often than that.
6ef7474
to
3d666cf
Compare
3d666cf
to
db5bc3a
Compare
Won't this also fix #1124? (I've been hitting that bug). |
Yes, I think you're right that these changes will fix #1124. I missed that issue. |
aws/resource_aws_lambda_function.go
Outdated
} | ||
} | ||
|
||
func updateComputedAttributesOnPublish(d *schema.ResourceDiff, meta interface{}) error { | ||
publish := d.Get("publish").(bool) | ||
if publish && needsFunctionCodeUpdate(d) { |
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.
What if you did the following. It would make sure the last_modified was updated even if publish is set to false.
if needsFunctionCodeUpdate(d) {
d.SetNewComputed("last_modified")
if publish {
d.SetNewComputed("version")
d.SetNewComputed("qualified_arn")
}
}
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.
Check out the new commit. It should cover your scenario now too
I like that improvement. It should be pretty easy to make that change and
add a new testcase for the non-publish update case
…On Thu, Jan 18, 2018 at 11:57 AM ChristopherGAndrews < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In aws/resource_aws_lambda_function.go
<#3032 (comment)>
:
> }
}
+func updateComputedAttributesOnPublish(d *schema.ResourceDiff, meta interface{}) error {
+ publish := d.Get("publish").(bool)
+ if publish && needsFunctionCodeUpdate(d) {
What if you did the following. It would make sure the last_modified was
updated even if publish is set to false.
if needsFunctionCodeUpdate(d) {
d.SetNewComputed("last_modified")
if publish {
d.SetNewComputed("version")
d.SetNewComputed("qualified_arn")
}
}
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3032 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGhlypfueMVYeEjTCxAKmOB_KNyCuJ5ks5tL3fggaJpZM4Rh05t>
.
|
@radeksimko is there anything else I should do to help this PR along? |
If we could get this one out it would be great, killing me right now to update this by hand. Thanks @mdlavin and @radeksimko! |
Any updates? I'd love to help if this still needs something to get it over the line. |
Is there anything I can do to help get this merged? |
Hi folks, sorry for the maintainer delay here. I'm not sure when I'll personally have time to get to this yet as I have quite a bunch already queued up (and I can't speak for the other maintainers), but let me at least tag it with an upcoming release so its on the radar. At a very quick glance this PR is probably okay but we'll want to double check more closely into its implications. Is anyone running this code in a custom build to report 👍 or 👎 experiences in their environment? Let's reaction vote on this comment please -- thanks! |
I've been running this in a custom build since I posted my branch and I
haven't seen any trouble with it
…On Tue, Feb 13, 2018, 5:35 PM Brian Flad ***@***.***> wrote:
Hi folks, sorry for the maintainer delay here. I'm not sure when I'll
personally have time to get to this yet as I have quite a bunch already
queued up (and I can't speak for the other maintainers), but let me at
least tag it with an upcoming release so its on the radar. At a very quick
glance this PR is probably okay but we'll want to double check more closely
into its implications. Is anyone running this code in a custom build to
report 👍 or 👎 experiences in their environment? Let's reaction vote on
this comment please -- thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3032 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGhl56NuTGrVJDFMnayn0zJ-hTual-Mks5tUg4tgaJpZM4Rh05t>
.
|
Crossing my fingers for v1.10.0 |
Please can we merge this one for v1.10.0, manually editing my tf files to update the version for every change is not fun at 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.
LGTM! Thanks so much for this contribution! 🚀
(2 test failures unrelated)
=== RUN TestAccAWSLambdaFunction_importS3
--- PASS: TestAccAWSLambdaFunction_importS3 (29.76s)
=== RUN TestAccAWSLambdaFunction_s3
--- PASS: TestAccAWSLambdaFunction_s3 (31.42s)
=== RUN TestAccAWSLambdaFunction_localUpdate_nameOnly
--- PASS: TestAccAWSLambdaFunction_localUpdate_nameOnly (49.55s)
=== RUN TestAccAWSLambdaFunction_localUpdate
--- PASS: TestAccAWSLambdaFunction_localUpdate (76.78s)
=== RUN TestAccAWSLambdaFunction_expectFilenameAndS3Attributes
--- PASS: TestAccAWSLambdaFunction_expectFilenameAndS3Attributes (121.32s)
=== RUN TestAccAWSLambdaFunction_runtimeValidation_noRuntime
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_noRuntime (0.77s)
=== RUN TestAccAWSLambdaFunction_s3Update_basic
--- PASS: TestAccAWSLambdaFunction_s3Update_basic (48.82s)
=== RUN TestAccAWSLambdaFunction_VPC
--- FAIL: TestAccAWSLambdaFunction_VPC (130.85s)
=== RUN TestAccAWSLambdaFunction_DeadLetterConfig
--- PASS: TestAccAWSLambdaFunction_DeadLetterConfig (132.72s)
=== RUN TestAccAWSLambdaFunction_versionedUpdate
--- PASS: TestAccAWSLambdaFunction_versionedUpdate (184.01s)
=== RUN TestAccAWSLambdaFunction_DeadLetterConfigUpdated
--- PASS: TestAccAWSLambdaFunction_DeadLetterConfigUpdated (202.30s)
=== RUN TestAccAWSLambdaFunction_encryptedEnvVariables
--- PASS: TestAccAWSLambdaFunction_encryptedEnvVariables (202.98s)
=== RUN TestAccAWSLambdaFunction_s3Update_unversioned
--- PASS: TestAccAWSLambdaFunction_s3Update_unversioned (105.74s)
=== RUN TestAccAWSLambdaFunction_VPCUpdate
--- PASS: TestAccAWSLambdaFunction_VPCUpdate (220.60s)
=== RUN TestAccAWSLambdaFunction_importLocalFile
--- PASS: TestAccAWSLambdaFunction_importLocalFile (220.77s)
=== RUN TestAccAWSLambdaFunction_runtimeValidation_python27
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_python27 (92.32s)
=== RUN TestAccAWSLambdaFunction_VPC_withInvocation
--- FAIL: TestAccAWSLambdaFunction_VPC_withInvocation (236.08s)
=== RUN TestAccAWSLambdaFunction_runtimeValidation_java8
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_java8 (126.89s)
=== RUN TestAccAWSLambdaFunction_basic
--- PASS: TestAccAWSLambdaFunction_basic (258.03s)
=== RUN TestAccAWSLambdaFunction_versioned
--- PASS: TestAccAWSLambdaFunction_versioned (273.10s)
=== RUN TestAccAWSLambdaFunction_runtimeValidation_nodeJs43
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_nodeJs43 (159.42s)
=== RUN TestAccAWSLambdaFunction_updateRuntime
--- PASS: TestAccAWSLambdaFunction_updateRuntime (292.63s)
=== RUN TestAccAWSLambdaFunction_concurrencyCycle
--- PASS: TestAccAWSLambdaFunction_concurrencyCycle (322.39s)
=== RUN TestAccAWSLambdaFunction_tags
--- PASS: TestAccAWSLambdaFunction_tags (214.75s)
=== RUN TestAccAWSLambdaFunction_envVariables
--- PASS: TestAccAWSLambdaFunction_envVariables (349.38s)
=== RUN TestAccAWSLambdaFunction_runtimeValidation_python36
--- PASS: TestAccAWSLambdaFunction_runtimeValidation_python36 (174.45s)
=== RUN TestAccAWSLambdaFunction_concurrency
--- PASS: TestAccAWSLambdaFunction_concurrency (375.63s)
=== RUN TestAccAWSLambdaFunction_importLocalFile_VPC
--- PASS: TestAccAWSLambdaFunction_importLocalFile_VPC (442.52s)
=== RUN TestAccAWSLambdaFunction_tracingConfig
--- PASS: TestAccAWSLambdaFunction_tracingConfig (482.08s)
=== RUN TestAccAWSLambdaFunction_nilDeadLetterConfig
--- PASS: TestAccAWSLambdaFunction_nilDeadLetterConfig (562.45s)
This has been released in version 1.10.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
This change will cause the lambda function publishes to correctly trigger version updates that will affect downstream resources.
Fixes #626, #2915, #1124 and #3192