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

Ant frontend container #4531

Open
wants to merge 36 commits into
base: dev
Choose a base branch
from
Open

Ant frontend container #4531

wants to merge 36 commits into from

Conversation

amenasse
Copy link
Contributor

@amenasse amenasse commented May 4, 2023

This is based off the branch from #4528 , that should be reviewed and merged first.

Introduces a frontend.Dockerfile which uses the existing nginx configuration to serve the frontend services. The current Dockerfile has been updated to not build the frontend packages.

In hindsight this could probably be combined into a single Dockerfile with different stages for frontend and backend services.
https://docs.docker.com/build/building/multi-stage/#stop-at-a-specific-build-stage

A script is run at boot to copy the .env files mounted at /env into the web root of each package.. this will be fetched by the app on startup (not yet implemented)

@amenasse amenasse requested a review from passcod May 10, 2023 22:58
@passcod
Copy link
Member

passcod commented May 12, 2023

Probably needs a review from tupaia people

@@ -50,14 +50,17 @@ COPY ./tsconfig* babel.config.json tsconfig-js.json jest.config-ts.json .eslintr
# Build and install internal dependencies
Copy link
Contributor

@IgorNadj IgorNadj May 16, 2023

Choose a reason for hiding this comment

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

Need to consider index.html as well, it has some templating that looks at env vars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, the following seems to work, but probably needs to be looked at by someone familair with tupaia.. suggest this be addressed in a seperate PR

diff --git a/packages/web-frontend/public/index.html b/packages/web-frontend/public/index.html
index 78279d1f8..7e0804fec 100644
--- a/packages/web-frontend/public/index.html
+++ b/packages/web-frontend/public/index.html
@@ -13,6 +13,7 @@
     />
     <meta property="og:url" content="https://tupaia.org" />
     <meta property="og:title" content="Tupaia" />
+    <script src="%PUBLIC_URL%/env-config.js"></script>

     <% if (process.env.REACT_APP_DEPLOYMENT_NAME === 'master' ||
     process.env.REACT_APP_DEPLOYMENT_NAME === 'main' || process.env.REACT_APP_DEPLOYMENT_NAME ===

echo "creating $p/.env-config.js"
env-js "$tupaia_env_dir/$package/.env" "$p/.env-config.js"
fi
done
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed
const baseUrl = process.env.REACT_APP_CONFIG_SERVER_BASE_URL || window.env.REACT_APP_CONFIG_SERVER_BASE_URL || '...'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be good if this could be looked at by someone familiar with tupaia as part of a separate PR

@amenasse amenasse force-pushed the ant-frontend-container branch from 8d73dd8 to c0e4235 Compare May 18, 2023 01:57
amenasse and others added 23 commits May 18, 2023 12:13
Seems to be the blessed yarn way now.
Dockerfile is no longer specific to a particular service
Avoids invalidating layer cache and reduces image bloat. Approach
courtesy of @passcod.
Fetching env from LastPass to be handled by a separate container.
Remove the devops package  from .dockerignore. Scripts from this package are
required by CodeShip services containers.

This means the devops package is included in the image, which is
a little redundant but most likely harmless.

Move the Dockerfile out of devops so changes to the Dockerfile don't
invalidate the layer cache unnecessarily.
Co-authored-by: Félix Saparelli <felix@passcod.name>
@amenasse amenasse force-pushed the ant-frontend-container branch from c0e4235 to 79c8a08 Compare May 18, 2023 02:22
@amenasse amenasse marked this pull request as ready for review May 18, 2023 04:59
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.

3 participants