Fix production build broken by missing env DATABASE_URL at build time #41
Conversation
| node_modules | ||
| npm-debug.log | ||
| .pnpm-store | ||
| out | ||
| dist | ||
| *.md | ||
| .git |
There was a problem hiding this comment.
We were inserting a lot of shit in the image.
| @@ -17,38 +18,37 @@ RUN --mount=type=cache,id=pnpm,target=/pnpm/store pnpm install \ | |||
| --frozen-lockfile \ | |||
| --filter "@latitude-data/web..." | |||
|
|
|||
| # PRODUCTION stage | |||
| # ------------------------------------------------------ | |||
| FROM installer AS builder | |||
There was a problem hiding this comment.
builder and installer can be merged I think.
andresgutgon
force-pushed
the
fix/security-issue-with-dockerfile-production
branch
from
f0552b4 to
ecfce74
Compare
| dockerfile: apps/web/docker/Dockerfile | ||
| # This enables to have this service for testing production build | ||
| # It will not run when doing docker compose up | ||
| profiles: [building] |
There was a problem hiding this comment.
Docker compose allow to have profiles. So no profile runs when docker compose up which is perfect for this case. I only want for the combinience of developing Dockerfile production and run docker compose build web
| @@ -24,6 +24,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@latitude-data/env": "workspace:^", | |||
| "@t3-oss/env-core": "^0.10.1", | |||
There was a problem hiding this comment.
I have the perfect excuse for this sorry. Explained in the next comment.
There was a problem hiding this comment.
plz... if (process.env.BUILDING_CONTAINER == 'true') return let's not fight more over this
There was a problem hiding this comment.
I don't understand why you don't want a tool built for this propose
There was a problem hiding this comment.
I actually like it for NextJS for the client-side safety measures that you mention ☝🏼 . I hate that we use it in api because it brings 0 value there, and adding a dependency is just overhead.
There was a problem hiding this comment.
Also this package is type save. When you do env.SOMETHING you're sure that's a number, string, enum,... Doing the old way you still use untyped process.env
|
|
||
| import '@latitude-data/env' | ||
|
|
||
| const envvars = z.object({ |
There was a problem hiding this comment.
Doing this breaks production build because it tries to find DATABASE_URL at build time. With t3-env this is easy fixable by using skipValidation with BUILDING_CONTAINER flag as we did in web.
There was a problem hiding this comment.
why? I don't get it why is this code evaluated at build time?
There was a problem hiding this comment.
Because it's imported by nextjs bundler. And because is imported zod parse function is run
There was a problem hiding this comment.
next bundler runs the code of its dependencies at compile time? :drunk:
andresgutgon
force-pushed
the
fix/security-issue-with-dockerfile-production
branch
from
c8999f0 to
d04f5c4
Compare
apps/api/tsconfig.json
Outdated
| @@ -1,6 +1,7 @@ | |||
| { | |||
| "extends": "@latitude-data/typescript-config/base.json", | |||
| "compilerOptions": { | |||
| "moduleResolution": "Bundler", | |||
There was a problem hiding this comment.
'bundler' for use with bundlers. Like node16 and nodenext, this mode supports package.json "imports" and "exports", but unlike the Node.js resolution modes, bundler never requires file extensions on relative paths in imports.
I think Bundler is ok
There was a problem hiding this comment.
why did u need this change?
There was a problem hiding this comment.
For t3-env/core. It use a module system that requires the more modern version. But the thing is we're already using Bundler in NextJS app.
There was a problem hiding this comment.
An I think bundler makes a lot of sense for us because we're using one only final bundler in nextjs or api and not having by package bundlers
| RUN --mount=type=cache,id=pnpm,target=/pnpm/store \ | ||
| BUILDING_CONTAINER=true \ | ||
| pnpm turbo build --filter='@latitude-data/web...' | ||
|
|
||
| FROM base AS runner | ||
| FROM base as prod-deps |
There was a problem hiding this comment.
I think it is worth it to have a stage only to install production deps. That then are copied in production stage
andresgutgon
changed the title
We moved DB initialization to core: #34 This breaks production build because at build time DATABASE_URL is not and should never be defined
andresgutgon
force-pushed
the
fix/security-issue-with-dockerfile-production
branch
from
d04f5c4 to
5dce8ba
Compare
What?
We moved DB initialization to core: #34 This breaks production build because at build time DATABASE_URL is not and should never be defined
TODO