-
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
Add lambda example #3168
Add lambda example #3168
Conversation
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.
Hi @oscr
Thanks for the suggestion, this is indeed a very common use-case :)
Just left a few things to address and discuss before merging.
Thanks! 👍 🚀
.gitignore
Outdated
@@ -1,3 +1,4 @@ | |||
*.zip |
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.
We actually have a few examples where we need those zips, as in: https://github.com/terraform-providers/terraform-provider-aws/tree/master/examples/cognito-user-pool so we should remov this one
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.
I will fix that mistake.
examples/lambda/README.md
Outdated
@@ -0,0 +1,9 @@ | |||
# Lambda Example | |||
|
|||
This examples shows how to deploy an AWS Lambda function. |
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.
I would like to enhance this message with: This examples shows how to deploy an AWS Lambda function using Terraform only.
Just to expose my point of view: another way to deploy a given lambda is by building the ZIP in your CI, put it on S3, and use a s3 object data source to get the file.
With this logic, you can version your lambdas and benefit from a real build - deploy phase.
Thoughts? :)
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.
I think that's an improvement. I'll add it.
examples/lambda/hello_lambda.py
Outdated
import os | ||
|
||
def lambda_handler(event, context): | ||
# This will show up in CloudWatch |
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.
I think we should either update this message to This will log to a CloudWatch Logs group if allowed by the Lambda role
or add the correct permission to the IAM role, as we are not defining the CloudWatch:Logs action to actually log.
In both cases, we should rewrite it to This will log to a CloudWatch Logs group
to better explain what's going on.
What do you think?
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.
I will remove logging because it's not that clear cut due to the AWS console. If you run the lambda you'll see the following under Log output:
START RequestId: dd6b2bf4-05e8-11e8-8679-a50298ecb88b Version: $LATEST
Value of 'foo': bar
END RequestId: dd6b2bf4-05e8-11e8-8679-a50298ecb88b
REPORT RequestId: dd6b2bf4-05e8-11e8-8679-a50298ecb88b Duration: 1.13 ms Billed Duration: 100 ms Memory Size: 128 MB Max Memory Used: 21 MB
If we were to print This will log to a CloudWatch Logs group if allowed by the Lambda role
and it appears in the console then it wouldn't be unreasonable to believe that the correct permissions are set. So I'll simplify the example.
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.
@oscr My question was meant for the comment line only, not the log line that would appear in CloudWatch Logs 😄, sorry for the confusion!
Hi @Ninir Thank you for your feedback. I have updated the example based on your comments. |
@Ninir Are there any other changes that you would like to see? |
@oscr will check it this day! |
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 @oscr :)
* commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9': (224 commits) v1.9.0 Update CHANGELOG for hashicorp#1101 and hashicorp#3283 docs/resource/aws_sns_platform_application: Add note about platform_credential and platform_principal hashing resource/aws_sns_platform_application: Refactor ID parsing to its own function, use testing.T in testAccAwsSnsPlatformApplicationPlatformFromEnv Add lambda example (hashicorp#3168) Update CHANGELOG for hashicorp#3157 docs/data-source/aws_region: Remove now deprecated current argument data-source/aws_region: Refactor logic into findRegionByEc2Endpoint and findRegionByName functions Update CHANGELOG for hashicorp#3301 Update CHANGELOG for hashicorp#2559 and hashicorp#3240 Update CHANGELOG.md resource/aws_kinesis_stream: Retry deletion on LimitExceededException (hashicorp#3108) Update CHANGELOG.md resource/aws_dynamodb_table_item: Cleanup + add missing bits Added dynamodb_table_item resource hashicorp#517 Update CHANGELOG.md New Resource: aws_cloud9_environment_ec2 Update CHANGELOG.md Fixed markdown typo in docs resource/aws_kinesis_firehose_delivery_stream: Prevent crashes on empty CloudWatchLoggingOptions and fix extended_s3_configuration kms_key_arn ... # Conflicts: # aws/validators.go
…parameters-features * commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9': (752 commits) v1.9.0 Update CHANGELOG for hashicorp#1101 and hashicorp#3283 docs/resource/aws_sns_platform_application: Add note about platform_credential and platform_principal hashing resource/aws_sns_platform_application: Refactor ID parsing to its own function, use testing.T in testAccAwsSnsPlatformApplicationPlatformFromEnv Add lambda example (hashicorp#3168) Update CHANGELOG for hashicorp#3157 docs/data-source/aws_region: Remove now deprecated current argument data-source/aws_region: Refactor logic into findRegionByEc2Endpoint and findRegionByName functions Update CHANGELOG for hashicorp#3301 Update CHANGELOG for hashicorp#2559 and hashicorp#3240 Update CHANGELOG.md resource/aws_kinesis_stream: Retry deletion on LimitExceededException (hashicorp#3108) Update CHANGELOG.md resource/aws_dynamodb_table_item: Cleanup + add missing bits Added dynamodb_table_item resource hashicorp#517 Update CHANGELOG.md New Resource: aws_cloud9_environment_ec2 Update CHANGELOG.md Fixed markdown typo in docs resource/aws_kinesis_firehose_delivery_stream: Prevent crashes on empty CloudWatchLoggingOptions and fix extended_s3_configuration kms_key_arn ... # Conflicts: # aws/resource_aws_ssm_parameter_test.go
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! |
I think that this example will be useful for a lot of people. At least when I was starting out with Terraform it took me quite a while to achieve this functionality.
If there is anything I can do to improve it please don't hesitate to ask.