Bootstrap new frontend#666
Conversation
4c5a99e to
92e8838
Compare
92e8838 to
b929d6f
Compare
b929d6f to
c51f06f
Compare
c51f06f to
3884482
Compare
rachelekm
left a comment
There was a problem hiding this comment.
This is working great! I have a couple small suggestions/questions, the most significant being the static directory naming convention which would be worth testing again if we change, but other than that this is close to merging!
| - ./src/frontend/build:/usr/local/src/backend/static/ | ||
| # Mount Vite dist to a new, distinct directory | ||
| # This directory must match the path in STATICFILES_DIRS in settings.py | ||
| - ./src/frontend/dist:/usr/local/src/backend/vite_dist_output |
There was a problem hiding this comment.
I think it would be easier for other developers to spin up/troubleshoot if we match the project template pattern here . Instead of having a uniquely named vite_dist_output and collected_static_files directories we just directly target and mount /dist/assets. What do you think?
|
|
||
| echo "Running frontend app tests..." | ||
| ./scripts/yarn run test | ||
|
|
There was a problem hiding this comment.
I can't add a comment below, but noting here that the project template shifted from using GH superlinter to megalinter (this was captured in an issue I prematurely closed), if we have time in the scope of this card we might want to switch to megalinter to better match the project template. If it feels out of scope I can re-open that issue and we can tackle as part of the setup epic.
There was a problem hiding this comment.
Ah, got it. How about we re-open that issue? The frontend part is fresh and should be straightforward to switch to a new linter. I suspect backend might be a bit more involved- just a guess.
There was a problem hiding this comment.
Good call on the backend I feel like I remember that being messy - I'll reopen and drop it into the setup epic lineup!
|
|
||
| # Build app container image first | ||
| # Build app container image | ||
| docker compose build app |
There was a problem hiding this comment.
I don't know why the app and db/django builds were separate, but I wonder if this can now just be docker compose build?
There was a problem hiding this comment.
I don't recall the reason exactly, but I do vague remember something about the frontend docker container needs to build first (frontend used to have a dockerfile for itself before this PR) and then build the static app, and then the backend (django, postgres) containers build, so that later the static asset collection could work etc.
We shouldn't have that issue anymore. Addressed in 71f43fe
| ENV DEBIAN_FRONTEND=noninteractive | ||
| # Ensures Python output is unbuffered | ||
| ENV PYTHONUNBUFFERED 1 | ||
| ENV DJANGO_SETTINGS_MODULE echo.settings |
There was a problem hiding this comment.
What does PYTHONUNBUFFERED do? Also DJANGO_SETTINGS_MODULE seems already set in ASGI and WSGI configs, do we need it here too?
There was a problem hiding this comment.
I removed DJANGO_SETTINGS_MODULE in 0d9d762
What does PYTHONUNBUFFERED do?
This was a result of giving Gemini the dockerfile and it suggested this improvement. I think the reasoning justifies it, and decided to add this in. Please let me know what you think!
The line ENV PYTHONUNBUFFERED 1 in a Dockerfile is a very common and important best practice when running Python applications inside Docker containers.
Here's what it means and why it's used:
What is Buffering?
By default, Python's standard output (stdout) and standard error (stderr) streams are buffered. This means that when your Python application prints something (e.g., using print(), or logging messages), that output isn't immediately sent to the underlying operating system or terminal. Instead, it's held in a temporary memory buffer. The buffer is only flushed (i.e., its contents are actually sent out) when:
The buffer becomes full.
The program explicitly flushes it (e.g., sys.stdout.flush()).
The program exits.
A newline character is encountered, but only if output is to a TTY (like a terminal you're directly interacting with).
Why is it a Problem in Docker?In a typical Docker environment, your application's stdout and stderr are usually redirected to Docker's logging driver (e.g., json-file, syslog, journald). Docker collects these streams as logs.
If Python's output is buffered:
Delayed Logs: Your application might print something, but it won't show up in docker logs <container_id> immediately. There could be a significant delay (seconds or even minutes) until the buffer fills up or the application eventually exits.
Missing Logs on Crash: If your application crashes unexpectedly before the buffer is flushed, you might lose valuable log messages that occurred just before the crash, making debugging very difficult.
Debugging Frustration: When you're trying to debug an issue, you want to see logs in real-time as they happen, not after a delay.
How ENV PYTHONUNBUFFERED 1 Solves It:Setting the PYTHONUNBUFFERED environment variable to 1 (or any non-empty string) forces Python's stdout and stderr streams to be unbuffered.
This means:
Real-time Logs: Every print() statement or log message is immediately written out to the underlying stream.
No Lost Logs: If the application crashes, all output up to that point should have already been sent to Docker's logging system.
Easier Debugging: You see exactly what your application is doing as it happens in the docker logs output.
There was a problem hiding this comment.
Oh neat, yeah let's keep PYTHONUNBUFFERED in!
| COPY ./src/frontend/build /usr/local/src/backend/static | ||
|
|
||
| # Expose the port Gunicorn server will listen on | ||
| EXPOSE 8085 |
There was a problem hiding this comment.
Do we need this too if we have gunicorn.conf.py?
| host: true, | ||
| strictPort: true, | ||
| port: 9966, | ||
| }, |
There was a problem hiding this comment.
We had to add a django proxy config as part of project template, do you think we need to add something similar here too? Nevermind, I tried out https://stg.echosearch.org/api/neighborhoods/ with the new deployment which is successful so I don't think the project template proxy config is necessary 🎉
| "type": "git", | ||
| "url": "https://github.com/azavea/echo-locator.git" | ||
| }, | ||
| "author": "Azavea", |
There was a problem hiding this comment.
| "author": "Azavea", | |
| "author": "Element 84", |
|
@rachelekm thanks for the review and the feedback. I addressed them and replied to your comments. Please let me know what you think. I also kicked off a staging deployment with the |
rachelekm
left a comment
There was a problem hiding this comment.
Thank you for implementing those updates and kicking off a staging deployment for testing, this looks great and good to go!
0d9d762 to
66dd634
Compare
|
Thanks for the re-review. I squashed the fixup commits and pushed up. Going to merge after CI. |
Overview
This PR:
Checklist
fixup!commits have been squashedCHANGELOG.mdupdated with summary of features or fixes, followingKeep a Changelog guidelines
README.mdupdated if necessary to reflect the changes./scripts/formatto lint, format, and fix the application source code.Testing Instructions
./scripts/updateand./scripts/servertestbranch to trigger a staging deployment. After it's deployed, go to https://stg.echosearch.org/ and then also visit https://stg.echosearch.org/components. Also make sure https://stg.echosearch.org/admin works fine as well.Resolves #639