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

fix: Docker image migrations #866

Merged
merged 11 commits into from
Mar 25, 2024
Merged

fix: Docker image migrations #866

merged 11 commits into from
Mar 25, 2024

Conversation

anatolinicolae
Copy link
Contributor

Issue

Base startup script doesn't include DB migrations, therefore app may be unable to start or will error when database is not synchronized.

Related issue: #845

In this PR

  • App database is now packaged in the image for proper migrations
  • Docker entrypoint is decoupled from normal start.sh
  • Docker entrypoint now generates the schema and runs migrations before app startup
  • Dockerfile underwent a little cleanup with the unification of WORKDIR directive in the base step

Update steps

Recreating your previous container with a freshly-pulled image should suffice (make sure to pull first).

New startup behaviour

+ npm run setup:db

> setup:db
> prisma generate && prisma migrate deploy

Prisma schema loaded from app/database/schema.prisma

✔ Generated Prisma Client (v5.11.0) to ./node_modules/@prisma/client in 141ms

Start using Prisma Client in Node.js (See: https://pris.ly/d/client)

import { PrismaClient } from '@prisma/client'
const prisma = new PrismaClient()

or start using Prisma Client at the edge (See: https://pris.ly/d/accelerate)

import { PrismaClient } from '@prisma/client/edge'
const prisma = new PrismaClient()


See other ways of importing Prisma Client: http://pris.ly/d/importing-client

┌─────────────────────────────────────────────────────────────┐
│  Deploying your app to serverless or edge functions?        │
│  Try Prisma Accelerate for connection pooling and caching.  │
│  https://pris.ly/cli/accelerate                             │
└─────────────────────────────────────────────────────────────┘

Prisma schema loaded from app/database/schema.prisma
Datasource "db": PostgreSQL database "postgres", schema "public" at "aws-0-eu-central-1.pooler.supabase.com:5432"

91 migrations found in prisma/migrations

Applying migration `20240124105759_add_aed_currency`

The following migration(s) have been applied:

migrations/
  └─ 20240124105759_add_aed_currency/
    └─ migration.sql
      
All migrations have been successfully applied.
+ npm run start

> start
> NODE_ENV=production node build/index.js

🚀 Server started on port 8080
Scheduler and workers registration completed

Copy link
Contributor

@DonKoko DonKoko left a comment

Choose a reason for hiding this comment

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

Okey so I pulled this and manually merged it with our dev branch, so I can test our CI/CD on fly.io.
Unfortunately this change, doesn't seem to work for fly. If I deploy with this new dockerfile our app completely breaks.
When you try to login you get the following error:
Screenshot 2024-03-21 at 13 24 47

I am not sure but I think its something with that new docker file overriding some types or something, because the type OrganizationRoles exists in the DB. I double checked that.
Also running the app locally(no docker) and connecting to the staging DB, also works without issue. So this tells me its not an actual issue with the DB.

Maybe fly.io has some limitations related to what we are trying to do? Is it possible to have different docker files to be used when deploying to fly and when using for self-hosting?

@anatolinicolae
Copy link
Contributor Author

@DonKoko great catch! Possibly all fixed now...

Went ahead and further split Dockerfiles for fly.io and image builds so they're independent now. Reworked the Dockerfiles themselves and the steps, which I included for fly.io as well—give that a try and if it still breaks, I'll roll that back.

Upgraded to --include and --omit as --production seems to be deprecated.

Tried deploying the container on my side and everything seems to be working as expected. Can you give another try on staging?

Copy link
Contributor

@DonKoko DonKoko left a comment

Choose a reason for hiding this comment

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

Okey so I deployed this to our staging app and it works fine. No issues faced.

  1. The file named Dockerfile.image. The part after the dot, is that following some requirement from docker or it can be anything? If it can be anything i rather do it more descriptive. Something like Dockerfile.selfhost

  2. I think we need to add a bit of explanation of this to the docs, so people understand whats the purpose of each docker file and know where to touch if they are making adjustments/changes.

package.json Show resolved Hide resolved
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>

Fix broken links

Signed-off-by: Anatoli Nicolae <an@thundersquared.com>

Fix note and code block

Signed-off-by: Anatoli Nicolae <an@thundersquared.com>

Link referenced section

Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>

Fix workflow property

Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
Signed-off-by: Anatoli Nicolae <an@thundersquared.com>

Make notes more visible

Signed-off-by: Anatoli Nicolae <an@thundersquared.com>
@anatolinicolae
Copy link
Contributor Author

Awesome! As per the points you raised:

  1. The suffix .imagein Dockerfile.image follows a generic convention that aims to separate Dockerfiles per environment, e.g. Dockerfile.prod denoting a production environment. In this case makes sense to use Dockerfile.image as that's the file we'll be using for our ghcr.io image builds.
  2. I went ahead and added some notes in both Get Started and Docker sections, adding a build command example for self-hosted images.

@DonKoko DonKoko merged commit be8d1b7 into Shelf-nu:main Mar 25, 2024
11 checks passed
@null-dev
Copy link
Contributor

null-dev commented Mar 30, 2024

Hey @anatolinicolae, thanks for packaging Shelf.nu a Docker container!

BTW, pruning the non-dev dependencies breaks seeding the DB (docker exec -it shelf npm run setup:seed), prisma relies on tsx to seed the DB, and it's a dev dependency.

@anatolinicolae
Copy link
Contributor Author

Good catch @null-dev! Mind opening a separate issue for that? I'll prepare a PR later this week to get it sorted 👍

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.

None yet

3 participants