Skip to content
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

Merged
merged 2 commits into from
May 8, 2024
Merged

Generate Frontend config and serve it #657

merged 2 commits into from
May 8, 2024

Conversation

matthew-white
Copy link
Member

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:

  • branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@@ -67,6 +67,10 @@ server {
include /usr/share/odk/nginx/common-headers.conf;
add_header Cache-Control no-cache;
}
location /client-config.json {
Copy link
Member Author

@matthew-white matthew-white May 2, 2024

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 \
Copy link
Member Author

@matthew-white matthew-white May 2, 2024

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
Copy link
Member Author

@matthew-white matthew-white May 2, 2024

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.

@matthew-white matthew-white requested review from lognaturel and ktuite and removed request for lognaturel May 2, 2024 03:42
@matthew-white matthew-white merged commit 1ef2c9c into next May 8, 2024
1 check passed
@matthew-white matthew-white deleted the request-config branch May 8, 2024 05:18
@matthew-white
Copy link
Member Author

Once it's on staging, I can double-check that /client-config.json is served correctly.

I can see that client-config.json is served and that its content matches my expectation: https://staging.getodk.cloud/client-config.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants