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.

Sign up for GitHub

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

fix: escape env variables #62

Merged
merged 1 commit into from
Nov 17, 2022
Merged

fix: escape env variables #62

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions tutormfe/patches/local-docker-compose-dev-services
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,4 @@
tty: true
volumes:
- ../plugins/mfe/apps/mfe/webpack.dev-tutor.config.js:/openedx/app/webpack.dev-tutor.config.js:ro
env_file:
- ../plugins/mfe/build/mfe/env/production
- ../plugins/mfe/build/mfe/env/development
environment:
- "PORT={{ app['port'] }}"
{%- for key, value in app.get("env", {}).get("production", {}).items() %}
- "{{ key }}={{ value }}"
{% endfor %}
{%- for key, value in app.get("env", {}).get("development", {}).items() %}
- "{{ key }}={{ value }}"
{%- endfor %}
{% endfor %}
52 changes: 35 additions & 17 deletions tutormfe/templates/mfe/build/mfe/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@ RUN echo "copying i18n data" \
&& mkdir -p /openedx/i18n/{{ app["name"] }} \
{%- endfor %}
echo "done."

{% for app in iter_values_named(suffix="MFE_APP") %}

################ {{ app["name"] }} MFE
######## {{ app["name"] }} (src)
FROM base AS {{ app["name"] }}-src
RUN git clone {{ app["repository"] }} --branch {{ app.get("version", MFE_COMMON_VERSION) }} --depth 1 .
Expand All @@ -37,15 +36,15 @@ COPY --from=i18n /openedx/i18n/{{ app["name"] }} /openedx/i18n/{{ app["name"] }}
COPY --from=i18n /openedx/i18n/i18n-merge.js /openedx/i18n/i18n-merge.js
RUN /openedx/i18n/i18n-merge.js /openedx/app/src/i18n/messages /openedx/i18n/{{ app["name"] }} /openedx/app/src/i18n/messages

######## {{ app["name"] }} (dev)
FROM base AS {{ app["name"] }}-dev
######## {{ app["name"] }} (common)
FROM base AS {{ app["name"] }}-common
COPY --from={{ app["name"] }}-src /openedx/app/package.json /openedx/app/package.json
COPY --from={{ app["name"] }}-src /openedx/app/package-lock.json /openedx/app/package-lock.json
ARG NPM_REGISTRY={{ NPM_REGISTRY }}
{{ patch("mfe-dockerfile-pre-npm-install") }}
{# Required for building optipng on M1 #}
{#- Required for building optipng on M1 #}
ENV CPPFLAGS=-DPNG_ARM_NEON_OPT=0
{# We define this environment variable to bypass an issue with the installation of pact https://github.com/pact-foundation/pact-js-core/issues/264 #}
{#- We define this environment variable to bypass an issue with the installation of pact https://github.com/pact-foundation/pact-js-core/issues/264 #}
ENV PACT_SKIP_BINARY_INSTALL=true
RUN npm clean-install --no-audit --no-fund --registry=$NPM_REGISTRY \
&& rm -rf ~/.npm
Expand All @@ -54,22 +53,41 @@ COPY --from={{ app["name"] }}-src /openedx/app /openedx/app
COPY --from={{ app["name"] }}-i18n /openedx/app/src/i18n/messages /openedx/app/src/i18n/messages
ENV PUBLIC_PATH='/{{ app["name"] }}/'
EXPOSE {{ app['port'] }}
CMD ["npm", "run", "start", "---", "--config", "./webpack.dev-tutor.config.js"]

{% endfor %}

{% for app in iter_values_named(suffix="MFE_APP") %}

######## {{ app["name"] }} (production)
FROM {{ app["name"] }}-dev AS {{ app["name"] }}
COPY ./env/production /openedx/env/production
RUN touch /openedx/env/production.override \
{%- set overrides = app.get("env", {}).get("production", {}) %}
{%- if overrides %}
RUN echo "setting production overrides..." \
{%- for key, value in app.get("env", {}).get("production", {}).items() %}
&& echo "{{ key }}='{{ value }}'" >> /openedx/env/production.override \
&& echo "{{ key }}='{{ value }}'" >> /openedx/env/production \
{%- endfor %}
&& echo "done setting production overrides"
RUN bash -c "set -a && source /openedx/env/production && source /openedx/env/production.override && npm run build"
{%- endif %}

######## {{ app["name"] }} (dev)
FROM {{ app["name"] }}-common AS {{ app["name"] }}-dev
COPY ./env/development /openedx/env/development
{%- set overrides = app.get("env", {}).get("development", {}) %}
{%- if overrides %}
RUN echo "setting development overrides..." \
{%- for key, value in overrides.items() %}
&& echo "{{ key }}='{{ value }}'" >> /openedx/env/development \
{%- endfor %}
&& echo "done setting development overrides"
{%- endif %}
CMD ["/bin/bash", "-c", "set -a && \
source /openedx/env/production && \
source /openedx/env/development && \
npm run start --- --config ./webpack.dev-tutor.config.js"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the three dashes here? I can't for the life of me find official documentation on why that's necessary in CMD in the first place, but I thought it was just because it was a separate item in the array. (I could totally be wrong, though.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 this change was introduced a couple days ago by @ARMBouhali: https://github.com/overhangio/tutor-mfe/pull/74/files#r1015250142 In my understanding, it's necessary because it's the only way to use the webpack dev config.

Is there an alternative solution?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least, @ARMBouhali, can you point us to how/where you figured this one out?

{% endfor %}

# Production images are last to accelerate dev image building
{%- for app in iter_values_named(suffix="MFE_APP") %}
######## {{ app["name"] }} (production)
FROM {{ app["name"] }}-common AS {{ app["name"] }}-prod
RUN bash -c "set -a && \
source /openedx/env/production && \
npm run build"
{% endfor %}

####### final production image with all static assets
Expand All @@ -79,7 +97,7 @@ RUN mkdir -p /openedx/dist

# Copy static assets
{% for app in iter_values_named(suffix="MFE_APP") %}
COPY --from={{ app["name"] }} /openedx/app/dist /openedx/dist/{{ app["name"] }}
COPY --from={{ app["name"] }}-prod /openedx/app/dist /openedx/dist/{{ app["name"] }}
{% endfor %}

# Copy caddy config file
Expand Down
2 changes: 1 addition & 1 deletion tutormfe/templates/mfe/build/mfe/env/production
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ NODE_ENV=production
PUBLISHER_BASE_URL=
REFRESH_ACCESS_TOKEN_ENDPOINT={% if ENABLE_HTTPS %}https{% else %}http{% endif %}://{{ LMS_HOST }}/login_refresh
SEGMENT_KEY=
SITE_NAME={{ PLATFORM_NAME|replace("'", "'\\''")|replace(" ", "\ ") }}
SITE_NAME="{{ PLATFORM_NAME }}"
STUDIO_BASE_URL={{ "https" if ENABLE_HTTPS else "http" }}://{{ CMS_HOST }}
USER_INFO_COOKIE_NAME=user-info

Expand Down