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

add files for docker deploy #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add files for docker deploy #1

wants to merge 1 commit into from

Conversation

makovecl
Copy link

please review and say what I did wrong

@@ -0,0 +1,30 @@
FROM node:20-alpine AS deps
Copy link
Member

Choose a reason for hiding this comment

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

Pls koristi zadnji LTS (node 22). Node 20 ide u maintainance mode za koji tjedan.

Preporučam napraviti nešto kao:

FROM node:22-alpine AS base


FROM base AS deps
...

RUN yarn --frozen-lockfile

FROM node:20-alpine AS builder
ARG APP_FILE_STORAGE_DIR
Copy link
Member

Choose a reason for hiding this comment

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

Ovi argovi su nepotrebni, pls ih makni

WORKDIR /app
COPY --from=deps /app/node_modules ./node_modules
COPY . .

Copy link
Member

Choose a reason for hiding this comment

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

Preporučam da dodaš

ENV NEXT_TELEMETRY_DISABLED 1
ENV SKIP_ENV_VALIDATION 1

da ne šaljemo telemetriju pri CI buildovima baš


RUN SKIP_ENV_VALIDATION=1 yarn build

FROM gcr.io/distroless/nodejs20-debian12 AS runner
Copy link
Member

Choose a reason for hiding this comment

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

Zašto koristiš drugačiju verziju nodejs za build i release?
Pls standardiziraj na gore naveden base koji dodaš


ENV NODE_ENV production

COPY --from=builder /app/next.config.mjs ./
Copy link
Member

Choose a reason for hiding this comment

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

Jedino što je potrebno kopirati su public, .next/standalone i .next/static, pls makni ostale

args:
APP_FILE_STORAGE_DIR: ${APP_FILE_STORAGE_DIR}
APP_PUBLIC_URL: ${APP_PUBLIC_URL}
working_dir: /app
Copy link
Member

Choose a reason for hiding this comment

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

Također nepotrebno, može potrgati stvar ako se promijeni dockerfile

APP_FILE_STORAGE_DIR: ${APP_FILE_STORAGE_DIR}
APP_PUBLIC_URL: ${APP_PUBLIC_URL}
working_dir: /app
ports:
Copy link
Member

Choose a reason for hiding this comment

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

Ports ne treaba biti u compose file-u. To se dodaje u override (jer npr. za naše potrebe ne želimo exposati niti jedan port)

working_dir: /app
ports:
- "3000:3000"
image: comp_ctf_2023
Copy link
Member

Choose a reason for hiding this comment

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

Ovo isto nije dobro da je tu, pls makni

Copy link
Member

Choose a reason for hiding this comment

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

Što je ova datoteka i zašto je tu?

Copy link
Member

Choose a reason for hiding this comment

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

Dodaj još .env.example, yarn-error.log, .vscode i compose.* da maknemo još te nepotrebne stvari

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.

2 participants