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

Update docker setup to include migrations #189

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mmerfort
Copy link
Contributor

No description provided.

@mmerfort mmerfort marked this pull request as draft March 31, 2023 13:02
zuzuvelas
zuzuvelas previously approved these changes Apr 3, 2023
@mmerfort mmerfort marked this pull request as ready for review April 7, 2023 12:36
Copy link
Contributor

@yusu-banana yusu-banana left a comment

Choose a reason for hiding this comment

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

I'm not sure the database migration stuff process is fully correct now, but I was supposed to be the one that fixed that like 2 months ago, so I think this can work for now.

@@ -1,11 +1,32 @@
FROM node:16.13.1-alpine3.15
# syntax=docker/dockerfile:1
FROM node:lts-alpine3.17 as builder
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to specify the exact version so if someone else pulls the dockerfile later on doesn't get a different minor version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what were the benefits of having a builder stage? Wouldn't it mean that the builder stage is not cached and every release would have to reinstall the node modules part even if it's just a change in a string in our code?

COPY --from=builder /app/package*.json ./
COPY --from=builder /app/build ./build
COPY --from=builder /app/prisma ./prisma
COPY --from=builder /app/prisma/database.empty ./prisma/db/main.db
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of doing this type of renames instead of having the same filename inside and outside, so I would suggest having the same name.

Copy link
Contributor

Choose a reason for hiding this comment

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

However since this is for a production Dockerfile only for now is not as bad.

COPY --from=builder /app/prisma ./prisma
COPY --from=builder /app/prisma/database.empty ./prisma/db/main.db

RUN chown -R twiggy:twiggy /app/prisma/db /app/assets/NFD/images

This comment was marked as resolved.

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 only want to have specific paths accessible for the twiggy user.

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