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

updated dockerfile for nest-forms-backend to new version #1400

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

skifahrer
Copy link
Contributor

@skifahrer skifahrer commented Sep 16, 2024

This pull request updates the old Dockerfile. The new Dockerfile is based on the nest-prisma-template with some specific requirements for nest-forms-backend.

@skifahrer skifahrer changed the title updated dockerfile to new version updated dockerfile for nest-forms-backend to new version Sep 17, 2024
@skifahrer skifahrer marked this pull request as ready for review September 17, 2024 07:15
@skifahrer skifahrer marked this pull request as draft September 17, 2024 07:49
@skifahrer skifahrer marked this pull request as ready for review September 17, 2024 14:27
@skifahrer skifahrer marked this pull request as draft September 17, 2024 14:38
@skifahrer
Copy link
Contributor Author

Tested on dev cluster and forms-backend successfully started

@skifahrer skifahrer marked this pull request as ready for review September 18, 2024 11:58
nest-forms-backend/Dockerfile Outdated Show resolved Hide resolved
# production
FROM base AS prod
# Install Playwright dependencies
RUN npx playwright install-deps chromium
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this needs to be present for the build step also or just for the run? If just for the run I would push it to line 28, just before prisma generate

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that I marked in the previous comment a wrong lines. But the question still stands is this needed also for build stage or can it be just part of the application and installed in app-base stage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When marek introduced this change he said that it needs to be present during build. But I will test if it is still ok in app-base

nest-forms-backend/Dockerfile Outdated Show resolved Hide resolved
nest-forms-backend/Dockerfile Outdated Show resolved Hide resolved
FROM base AS build-base
# Make forms-shared available
WORKDIR /forms-shared
COPY --from=shared /app/ ./
Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover, do we need entire /app directory with node_modules and everything? Wouldn't just /app/dist/ be enough?

dist has just 7.2Mb and node_modules has 1.2Gb. It would be a lot less coping and if we could just limit it files that we only need.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that you tested with removing from here and from the prod stage. So that means that it needs node_modules for actual run of the application. But as I'm looking at the https://github.com/bratislava/konto.bratislava.sk/actions/runs/10992601218/job/30517938642#step:19:1819 it seems that the docker was built successfully. So, that means that we can probably have here just

COPY --from=shared /app/dist /forms-shared/dist

and then have in the prod stage copy the app.


ENTRYPOINT npx prisma migrate deploy && npm run start:prod
COPY --chown=node:node --from=shared /app/ /home/node/forms-shared
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above comment here. Do we need entire /app directory with everything? Can't we just copy a part of it like just dist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it a few times, but it didn't work. The way the forms-shared module is included in the nest-forms-backend doesn't allow us to strip node_modules from forms-shared. For example, here's a run where /app/dist was used in prod build, and it failed when did run due to missing node_modules in forms-shared.
https://github.com/bratislava/konto.bratislava.sk/actions/runs/10992601218/job/30517938642#step:19:1819

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that is fine then. I would still like to just specifically copy over only the files we need. I guess in this case it would be dist and node_modules (?) rather then entire /app folder.

@github-actions github-actions bot added pr: needs work 🛠️ Changes requested before another review and removed pr: needs review 🙏 labels Sep 18, 2024
@skifahrer skifahrer removed the pr: needs work 🛠️ Changes requested before another review label Sep 23, 2024
Copy link

github-actions bot commented Sep 24, 2024

Test build pipeline info 🚀

Changes in the code and tag info:

➡️ Changes in forms-shared: false

➡️ Changes in next: false

➡️ Changes in strapi: false

➡️ Changes in nest-forms-backend: true

➡️ Changes in nest-clamav-scanner: false

We are going to build 🚢

🔜 nest-forms-backend

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

Successfully merging this pull request may close these issues.

3 participants