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.

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

provider/aws: Add support for updating Lambda function #5239

Merged

Conversation

radeksimko
Copy link
Member

This supersedes #3825

To do:

@radeksimko
Copy link
Member Author

Waiting for #5305 to be reviewed and merged. That should make writing acceptance test for S3 deployment much easier.

@radeksimko radeksimko force-pushed the f-aws-lambda-func-updates branch 9 times, most recently from 76c09be to 9409086 Compare February 25, 2016 19:44
@radeksimko radeksimko changed the title [WIP] provider/aws: Add support for updating Lambda function provider/aws: Add support for updating Lambda function Feb 25, 2016
@radeksimko
Copy link
Member Author

This is now ready for final review.

if d.HasChange("s3_bucket") || d.HasChange("s3_key") || d.HasChange("s3_object_version") {
d.SetPartial("s3_bucket")
d.SetPartial("s3_key")
d.SetPartial("s3_object_version")
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we're building up inputs for a single API call rather than making a bunch of smaller API calls, I believe the partial mode code is unnecessary. The update will either fully succeed or fully fail, there's no partial success here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was first thinking the same thing when I first saw this piece of code (it's not exactly my addition), but then I scrolled down to line 351. We actually send two API calls: https://github.com/hashicorp/terraform/pull/5239/files#diff-2dcdbaf0cd0066fa2112ca76a0c2135eR351

So I think the partial state handling here is actually useful, isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry yes, missed that. So I think the pattern should then be to setPartial on all the attributes that would be successfully updated by UpdateFunctionCode once it completes successfully. In pseudocode:

enable partial mode
build up UpdateFunctionCode request
  if error, return and nothing is updated
  if successful, mark all FunctionCode related attributes successful
build up UpdateFunctionConfig request
  if error, return and only marked FunctionCode attrs will be picked up
  if successful, disable partial mode, since everything worked (perhaps mark specific attrs here anyways to prepare for a potential 3rd update API call down the road?)

Let me know if this makes sense!

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I think it does - basically it means I'll need to move all of the d.SetPartial( calls below conn.UpdateFunctionCode so these are only called when conn.UpdateFunctionCode is actually successful.

The same will apply to UpdateFunctionConfiguration and its related fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep exactly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@phinze
Copy link
Contributor

phinze commented Mar 9, 2016

Few nits inline, but this is looking good!

@radeksimko radeksimko force-pushed the f-aws-lambda-func-updates branch from 01d5fdd to 4a3ee46 Compare March 9, 2016 11:33
@radeksimko radeksimko force-pushed the f-aws-lambda-func-updates branch from 4a3ee46 to 9f6b487 Compare March 10, 2016 12:31
@radeksimko
Copy link
Member Author

@phinze are u happy with all the changes I made, especially around the partial state mgmt?

@phinze
Copy link
Contributor

phinze commented Mar 11, 2016

Yep this LGTM!

radeksimko added a commit that referenced this pull request Mar 11, 2016
provider/aws: Add support for updating Lambda function
@radeksimko radeksimko merged commit d8b3653 into hashicorp:master Mar 11, 2016
@radeksimko radeksimko deleted the f-aws-lambda-func-updates branch March 11, 2016 16:59
seajoshc added a commit to awslabs/lambda-chef-node-cleanup that referenced this pull request Mar 16, 2016
seajoshc added a commit to awslabs/lambda-chef-node-cleanup that referenced this pull request Mar 17, 2016
@ghost
Copy link

ghost commented Apr 27, 2020

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants