-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Initial incubator resources for container build #819
Conversation
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! Renaming docker-entrypoint.sh
will help avoid confusion. Thank you!
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.
gunicorn
is not a dependency anymore and I can't see where it's being installed. You might be able to get away with running the fastapi run app/main.py --port 8000
command instead.
Good call @paulespinosa - I made this change as you suggested. It's using a virtual env there now because there was an issue running either of the |
It looks good. Nothing off the top of my head can think of doing it any other way. Nice work! |
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. Do you want to add an environment variable to capture the right path to the nginx logs for this PR or leave it for another?
backend/app/health.py
Outdated
|
||
@health_router.get("/nginx-logs", status_code=status.HTTP_200_OK) | ||
def nginx_logs(): | ||
with open('/var/log/qa.homeunite.us/nginx-access.log') as f: |
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 is a good place to introduce a HUU_ENVIRONMENT
environment variable in config.Settings
.
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.
Ah good catch, will change this before merging
Completes some items from #627
What changes did you make?
base
andapp
container images to.incubator
directory.incubator
directoryRationale behind the changes?
Testing done for these changes
What did you learn or can share that is new?(optional)
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
N/A