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

add terraform lambda layers sample + migrate serverless sample to new dir #238

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

HarshCasper
Copy link
Member

@HarshCasper HarshCasper commented May 17, 2024

This PR:

TODO

  • Add @joe4dev as the commit author before merge.

@HarshCasper HarshCasper requested a review from joe4dev May 17, 2024 15:38
@HarshCasper HarshCasper force-pushed the lambda-layers-sample branch from de67b78 to 5b24623 Compare May 17, 2024 15:47
Copy link
Contributor

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

LGTM after updaing the Terraform installation and validating the Lambda invocation for CI.

Thank you for pushing this out @HarshCasper 👍

@which awslocal || pip install awscli-local
@which terraform || (\
echo 'Terraform was not found, installing locally' && \
wget https://releases.hashicorp.com/terraform/1.0.8/terraform_1.0.8_linux_amd64.zip && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Outdated Terraform version and hardcoding amd64 fails on ARM machines.
https://developer.hashicorp.com/terraform/install

output.json
cat output.json

run: start install package init deploy invoke
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding some (minimal) validation step. We should at least check that the Lambda invoked successfully and did not fail for the CI to catch regressions.

rm -rf .terraform
rf -f output.json

.PHONY: start install init deploy invoke clean
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: incomplete missing (e.g., missing run, start, ...)

Make sure that LocalStack is started:

```
LOCALSTACK_AUTH_TOKEN=... DEBUG=1 localstack start
Copy link
Contributor

Choose a reason for hiding this comment

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

General Idea for docs: Would it make sense to use a variable such as $LOCALSTACK_AUTH_TOKEN to facilitate copy/pasting the command rather using some bash-incompatible placeholder (e.g., ... or <>)?

@test -e .venv || ($(PYTHON_BIN) -m venv .venv; source .venv/bin/activate; pip3 install terraform-local;)

package:
source .venv/bin/activate; pip install \
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment pointing to the AWS packaging docs for Python would be helpful here: https://docs.aws.amazon.com/lambda/latest/dg/python-package.html
In particular, mention the limitation that dependencies with native dependencies require further configuration and should ideally be build using Docker.


## Prerequisites

* LocalStack
Copy link
Contributor

Choose a reason for hiding this comment

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

We should clarify that Layers require the Pro image here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants