-
Notifications
You must be signed in to change notification settings - Fork 160
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
Generate Frontend config and serve it #657
Conversation
@@ -67,6 +67,10 @@ server { | |||
include /usr/share/odk/nginx/common-headers.conf; | |||
add_header Cache-Control no-cache; | |||
} | |||
location /client-config.json { |
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 location
block and the ones above and below it specify identical directives. However, I don't see a way to specify a list to location
. We could use a regular expression, but I'm not sure that that's preferable.
@@ -3,12 +3,14 @@ FROM node:20.12.2-slim as intermediate | |||
RUN apt-get update \ | |||
&& apt-get install -y --no-install-recommends \ | |||
git \ | |||
gettext-base \ |
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.
Below I run envsubst
. Originally I did so without installing gettext-base
. However, the build resulted in an error:
/bin/sh: 1: envsubst: not found
I'm not sure the best way to get envsubst
. This answer on StackOverflow suggested installing gettext
as a way to get it. I don't know much about gettext
, but I saw that enketo.dockerfile installs gettext-base
. Given that enketo.dockerfile installs it, I figured it'd be OK to do so here as well. service.dockerfile installs gettext
.
An alternative would have been to wait to run envsubst
further below. setup-odk.sh runs envsubst
, so it must get installed somehow. However, I thought it was clearer overall to run envsubst
here in intermediate
. intermediate
is where version.txt and the rest of the Frontend assets are generated, so it feels like the right place to generate client-config.json.
nginx.dockerfile
Outdated
&& rm -rf /var/lib/apt/lists/* | ||
|
||
COPY ./ ./ | ||
RUN files/prebuild/write-version.sh | ||
ARG OIDC_ENABLED | ||
RUN OIDC_ENABLED="$OIDC_ENABLED" files/prebuild/build-frontend.sh | ||
RUN envsubst < files/nginx/client-config.json.template > /tmp/client-config.json |
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.
Elsewhere, we specify a list of variables to envsubst
. However, with #473 in mind, I didn't do so here.
23d82a0
to
132f993
Compare
132f993
to
e24b28f
Compare
I can see that client-config.json is served and that its content matches my expectation: https://staging.getodk.cloud/client-config.json |
This PR makes progress on #656. It generates the Frontend config and serves it so that Frontend can then fetch it. The corresponding Frontend PR is getodk/central-frontend#988.
What has been done to verify that this works as intended?
So far just that CI passes. Once it's on staging, I can double-check that /client-config.json is served correctly.
Why is this the best possible solution? Were any other approaches considered?
I'm going to leave comments about a few lines.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
getodk/central-frontend#988 mentions the cost of an additional request. I don't think it will be much of an issue.
Before submitting this PR, please make sure you have:
next
branch OR only changed documentation/infrastructure (master
is stable and used in production)