-
-
Notifications
You must be signed in to change notification settings - Fork 686
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
feat: Add SAM Metadata resources to enable the integration with SAM CLI tool #325
feat: Add SAM Metadata resources to enable the integration with SAM CLI tool #325
Conversation
@bryantbiggs .. Could you please check this PR. |
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.
Looks pretty good. I am excited to see an additional integration with SAM, thank you! It would be great to have at least a paragraph saying about this feature and commands to use this with SAM in readme or in example. (If there is no notion about something in terraform-aws-modules - feature doesn't exist)
main.tf
Outdated
resource_type = "ZIP_LAMBDA_FUNCTION", | ||
|
||
# The Lambda function source code. | ||
original_source_code = jsonencode(var.source_path), |
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.
source_path can be rather complicated type (see examples or readme), I am not sure this will be well supported by SAM. Could you please verify this?
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.
In Sam Cli, we expect the value of this property to be either String or JSON object. In case if it is a JSON object as we have here in serverless TF module, SAM CLI will expect another property source_code_property
to let SAM CLI know, where it can find the Lambda resource code path. In Sam Cli , we do not support multiple paths, so in case if the value of this attribute is a list (of strings, or objects), SAM CLI will process only the first item in the list.
Thanks @antonbabenko for your quick response. Regarding the Readme update, is it ok to add a new section regarding SAM CLI integration before the FAQ section in this Readme file |
@moelasmar I would put it under the |
I founded that it is better to have a separate section for SAM CLI integration, as we plan to support testing only for now, and no plan to support package and deploy in the current project. @antonbabenko, @bryantbiggs could you please have a look, and please let me know if you have any comments. |
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 looked at it in details after I had read the specs. In general, this PR looks pretty good, but I was not able to test it for real without building SAM with your changes.
There is one hard part I want to highlight:
- Instead of doing the whole cycle for AWS resource extraction (
terraform init
, plan into a file, show as jіon, jq, python, etc), I recommend that this module optionally creates a local file in the known location (if the user specifies likecreate_sam_metadata = true
), SAM just reads the captured JSON object and Prepare Hook will generate CFN Template (or even Terraform). This will be much faster, more flexible, and transparent for the user.
Maybe I don't know the whole picture, so please correct me (you have my email, too). I will be glad to cooperate with you on this (I am back from vacation, finally)!
README.md
Outdated
applications. Currently, SAM CLI tool only supports CFN applications, but SAM CLI team is working on a feature to extend the testing capabilities to support terraform applications (check this [Github issue](https://github.com/aws/aws-sam-cli/issues/3154) | ||
to be updated about the incoming releases, and features included in each release for the Terraform support feature). | ||
|
||
SAM CLI provides two ways of testing, local testing, and testing on-cloud (Accelerate). |
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.
SAM CLI provides two ways of testing, local testing, and testing on-cloud (Accelerate). | |
SAM CLI provides two ways of testing: local testing and testing on-cloud (Accelerate). |
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.
done
main.tf
Outdated
triggers = { | ||
# This is a way to let SAM CLI correlates between the Lambda function resource, and this metadata | ||
# resource | ||
resource_name = "aws_lambda_function.this[0]", |
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.
Please remove comas after each value on the map.
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.
done
Thanks @antonbabenko for your comment. Let me think more about it, also we can discuss it more in the meeting we have next week. |
Hi @moelasmar ! Thanks for your time and yesterday's discussion about this PR and SAM/TF integration. If you don't mind, I'd like to keep this PR open until I can run SAM CLI locally myself and verify that it works as expected. It also doesn't look good for the users if we include SAM CLI commands in the docs, but users can't run them because SAM CLI doesn't have them available (not even in |
…integration_null_resource_solution' into master_sam_cli_integration_null_resource_solution
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
modules/docker-build/main.tf
Outdated
# This resource contains the extra information required by SAM CLI to provide the testing capabilities | ||
# to the TF application. This resource will maintain the metadata information about the image type lambda | ||
# functions. It will contain the information required to build the docker image locally. | ||
resource "null_resource" "sam_metadata_docker_registry_image_this" { |
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.
Can we drop _this
from the name in resources?
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.
Looks good!
Great work, @moelasmar ! |
## [4.6.0](v4.5.0...v4.6.0) (2022-11-03) ### Features * Add SAM Metadata resources to enable the integration with SAM CLI tool ([#325](#325)) ([bfcd34c](bfcd34c))
This PR is included in version 4.6.0 🎉 |
Thanks @antonbabenko for your support |
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. |
Description
This change is to add new resources (sam_metadata_*) of type (null_resource) that will maintain some information about the lambda resources exist in the terraform application to enable the integration with SAM CLI. SAM CLI tool is looking for the lambda resources source code, and the resources that contain the building logic of these resources. SAM CLI can use this information to extend the SAM CLI testing capabilities to the terraform applications.
Motivation and Context
This change will enable the customers who use Serverless TF module to define Lambda resources to integrate with SAM CLI tool, and so the customers can test their applications either locally or on cloud.
Breaking Changes
no breaking changes.
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectsAs the new resource is just to collect metadata for SAM CLI tool, so it does not require any changes from the modules users side, and so no change in the examples. So I tested that I can run
terraform apply
for the available examples without any issues.pre-commit run -a
on my pull request