-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feature/infra setup #17
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # app/next.config.js
…vements (caching training data locallly, removing document orientation rotation)
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 and mergeable as-is, all things considered. I might recommend merging this via squashing it into a single commit so that we don't get a much larger git repo from the addition and removal of all those .next
and node_modules
files.
app/Dockerfile
Outdated
ARG RUN_UID | ||
ARG RUN_USER | ||
ARG RUN_UID=${UID:-4000} | ||
ARG RUN_USER=${USER:-'nodummy'} |
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 couldn't find any documentation that this syntax is supported here. Does it work?
"outputs": {}, | ||
"resources": [], | ||
"check_results": null | ||
} |
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.
This file shouldn't exist since the terraform state is stored in S3, right? We certainly don't want to commit the state here by accident.
# size = 100, # Size in MiB | ||
# mountOptions = ["defaults"] | ||
# } | ||
# ] |
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 general, we should remove code rather than commenting it out.
node_modules/.bin/tsc
Outdated
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.
It would be nice to add node_modules
and .next
to .gitignore
so they aren't accidentally included again.
@@ -20,7 +20,7 @@ locals { | |||
# If enabled, the networks associated with this application's environments | |||
# will have NAT gateways, which allows the service in the private subnet to | |||
# make calls to the internet. | |||
has_external_non_aws_service = false | |||
has_external_non_aws_service = true |
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.
This might cost us $30/month per NAT gateway. I will log into AWS to see what they're projecting this thing to cost.
Ticket
Changes