-
Notifications
You must be signed in to change notification settings - Fork 50
Allow /version
to be defined by environment variables
#1015
Conversation
We can remove the _api-up dependency from collectstatic since it's already called in the dj recipe
These were an artifact of having to create the nginx config within the init.tpl for the API, which required escaping the $. These are no longer needed and actually ended up producing erroneous output.
@sarayourfriend I've added you as a reviewer since you were the original author of #990 and are most familiar with the code! |
API Developer Docs Preview: Ready https://wordpress.github.io/openverse-api/_preview/1015 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
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, thank you for catching this! I totally forgot about this requirement when working on the original issue. Thanks for fixing the logging as well, I had not noticed that.
ENV NGINX_ENVSUBST_FILTER="DJANGO_UPSTREAM_URL" | ||
# Only environment variables with this prefix will be available in the template | ||
ENV NGINX_ENVSUBST_FILTER="DJANGO_NGINX_" | ||
ENV DJANGO_NGINX_ENVIRONMENT="local" |
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.
We'll need to be sure to pass this environment variable in the ECS configuration as well, once we get there. That at least is constant for the environment, so not an issue.
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.
Yes, same with the DJANGO_NGINX_GIT_REVISION
!
Regarding |
That's exactly what I did as well! And what I ended up having to link to. I even had to play around with |
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.
Thank you for the testing instructions! LGTM 😄
Fixes
This is necessary for continued work on https://github.com/WordPress/openverse-infrastructure/issues/175
Description
This PR makes a few changes to the nginx target image for the API in order to prevent the need for mounting any files into the docker container. Previously, the
/version
was configured to return the contents of aversion.json
file. This file was created during deployment and mounted into the nginx image:https://github.com/WordPress/openverse-infrastructure/blob/369d7858636797de276dbfc299b90b822a5ac527/modules/services/openverse-api/init.tpl#L79-L85
The issue with this is that we are trying to avoid the need for mounting anything into our API-related containers. This was the last necessary item that was being mounted. You can see that this endpoint is not served by the API by running
just up
and visiting http://localhost:50280/version.I've altered the image to construct the response from
/version
using environment variables rather than reading from a file on-disk. This involved a few changes to the environment substitution setup, which I was unfamiliar with (there are seemingly no references toNGINX_ENVSUBST_FILTER
on GitHub where it's being defined by anyone other than us!). I added a few more comments to try and provide reference to what was being used, and where.I also addressed an issue wherein we were erroneously referencing variables inside the nginx template with
\$
rather than just$
. This was an artifact of constructing the config inside of theinit.tpl
file during deployment (which required that we escape the$
character since it was used for its own interpolation). This was producing logs that looked like the following (note the extra\
characters before field values):Lastly, I added a
just
recipe for running the nginx image locally with some suggestions on how to test that it works.Testing Instructions
just up
just nginx
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin