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

Added possibility to deploy sample example with terraform #37

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

k-a-il
Copy link
Contributor

@k-a-il k-a-il commented Oct 18, 2024

This PR provides possibility to deploy the app using Terraform, additionally to the bin/deploy.sh script.

Copy link
Contributor

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this cool contribution, @k-a-il!
And I have to apologize that the pipeline is not working correctly.
This is because this repo hasn't had outside collaborators yet, and the pipelines do need secrets (f.e. to start LocalStack Pro) which are not available when the pipelines are triggered from forks (due to security considerations).
We will definitely tackle this in a follow-up to make sure that community contributions can be tested properly as well!
For now, I just triggered the pipeline manually by pushing your commit to a temporary branch and executing it from there, and it's 💚:
https://github.com/localstack-samples/sample-serverless-image-resizer-s3-lambda/actions/runs/11416513029
In another iteration, we could also think about introducing a CI pipeline step to also test your new way of deployment, but I would say that this definitely is out of scope of this PR. 😄

I added a few comments, mostly nitpicks and some questions to discuss. Afterwards these things have been discussed / addressed, we should be good to go! 🚀

deployment/policies/website_bucket_policy.json.tpl Outdated Show resolved Hide resolved
deployment/policies/lambda.json Outdated Show resolved Hide resolved
bin/deploy.sh Outdated Show resolved Hide resolved
bin/deploy.sh Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
deployment/terraform/main.tf Outdated Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
Copy link

@sannya-singal sannya-singal left a comment

Choose a reason for hiding this comment

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

Nice work @k-a-il 🎉 Apologies for the pipeline issues, as mentioned above.

Thanks for adding the Terraform integration—it’s a valuable addition to the sample, enabling automated infrastructure provisioning with tflocal and enhancing the deployment options. Also, appreciate you addressing the nits from Alex before my review! 😄 The structure is very clean, making the code easy to follow and understand.

I have some comments and also same questions around the local_run variable and the current state for aws deployment of the terraform script as highlighted by Alex. Once these are resolved, we should be good to go! 🚀

deployment/terraform/main.tf Outdated Show resolved Hide resolved
deployment/policies/lambda.json Outdated Show resolved Hide resolved
bin/deploy.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link

@sannya-singal sannya-singal left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback. The PR LGTM! 🚀

Copy link
Contributor

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Thanks, @k-a-il for the contribution, and for addressing all the comments! 🏅
The new terraform deployment is looking great, and will definitely be useful for terraform users who want to get started with LocalStack! 💯

@alexrashed alexrashed merged commit b615e04 into localstack-samples:main Oct 24, 2024
1 of 2 checks passed
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.

3 participants