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

Ayr 446/flask app in lambda #71

Merged
merged 11 commits into from
Dec 5, 2023
Merged

Conversation

anthonyhashemi
Copy link
Contributor

@anthonyhashemi anthonyhashemi commented Nov 22, 2023

Changes in this PR

  • Added zappa as poetry dependency
  • Added zappa_settings.json.template and aws_attach_policy.json
  • Add Deployment section in README.md so devs can run manual deployments to a Lambda-APIGateway set up on AWS, given a AWS profile of their choice, from their local machines.

JIRA ticket

https://national-archives.atlassian.net/jira/software/projects/AYR/boards/66?selectedIssue=AYR-446&text=poetry

@colinbowen
Copy link
Collaborator

PR looks clear to me with a great update to the README.

@ben-cerium
Copy link

This ok to get you started. The IAM policy we can slim down in the following days.
Note that the IP address restrictions have been handcranked on this currently live resource so we need make sure that stays in place. We have that policy in code but don't want to put that in a public repo so we need to chat about that also

@ben-cerium
Copy link

ben-cerium commented Nov 23, 2023

Might want to consider adding a lambda timeout. Think it's 30 seconds by default which is probably too high e.g.
"timeout_seconds": 7, happy if this this is put in later

Copy link
Contributor

@zaiziclebby zaiziclebby left a comment

Choose a reason for hiding this comment

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

Seems ok to me

Copy link
Contributor

@pgandhizaizi pgandhizaizi left a comment

Choose a reason for hiding this comment

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

very detailed explanation in readme.md -looks good
Please check for default lambda timeout and may be reduce to a level where it has to set minimum value

@anthonyhashemi
Copy link
Contributor Author

anthonyhashemi commented Nov 23, 2023

This ok to get you started. The IAM policy we can slim down in the following days. Note that the IP address restrictions have been handcranked on this currently live resource so we need make sure that stays in place. We have that policy in code but don't want to put that in a public repo so we need to chat about that also

So from when I have run this, it doesnt seem to overwright any of the ip stuff you have set manually. I believe that is because I am not attaching a policy for the API Gateway but just for the lambda. If we were to do that we would need to make sure the ips wereset appropriately and were filled in by env vars in github actions so they werent commited to the repo, but I think for now we can get away without it. Worth tidying it up in a follow up PR dedicated to the API Gateway policy if you agree?

And yes you are right, most of that policy is uneeded, we currently just need the Paramter store access so I will remove everything apart from that. And then once we sort the rds stuff I will add that in as needed.

@anthonyhashemi
Copy link
Contributor Author

Might want to consider adding a lambda timeout. Think it's 30 seconds by default which is probably too high e.g. "timeout_seconds": 7, happy if this this is put in later

good shout, can do this here

Copy link

codecov bot commented Nov 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c54e96f) 78.94% compared to head (78f2861) 78.94%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #71   +/-   ##
=======================================
  Coverage   78.94%   78.94%           
=======================================
  Files           7        7           
  Lines         266      266           
=======================================
  Hits          210      210           
  Misses         56       56           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anthonyhashemi
Copy link
Contributor Author

This ok to get you started. The IAM policy we can slim down in the following days. Note that the IP address restrictions have been handcranked on this currently live resource so we need make sure that stays in place. We have that policy in code but don't want to put that in a public repo so we need to chat about that also

So from when I have run this, it doesnt seem to overwright any of the ip stuff you have set manually. I believe that is because I am not attaching a policy for the API Gateway but just for the lambda. If we were to do that we would need to make sure the ips wereset appropriately and were filled in by env vars in github actions so they werent commited to the repo, but I think for now we can get away without it. Worth tidying it up in a follow up PR dedicated to the API Gateway policy if you agree?

And yes you are right, most of that policy is uneeded, we currently just need the Paramter store access so I will remove everything apart from that. And then once we sort the rds stuff I will add that in as needed.

have added apigateway policy with ip whitelisting being set by env vars

@anthonyhashemi anthonyhashemi force-pushed the AYR-446/flask-app-in-lambda branch 3 times, most recently from 4f2985c to bc0ab1f Compare November 30, 2023 15:22
@anthonyhashemi anthonyhashemi force-pushed the AYR-446/flask-app-in-lambda branch from acb959f to 78f2861 Compare December 5, 2023 13:56
@anthonyhashemi anthonyhashemi merged commit e7dae94 into main Dec 5, 2023
8 checks passed
@anthonyhashemi anthonyhashemi deleted the AYR-446/flask-app-in-lambda branch December 5, 2023 13:58
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.

5 participants