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

added dockerfile #160

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

added dockerfile #160

wants to merge 1 commit into from

Conversation

smellman
Copy link
Contributor

Description

unvt/nanban can't build in arm64 arch, so I make Dockerfile for charites only.

Type of Pull Request

  • Adding a feature
  • Fixing a bug
  • Maintaining documents
  • Others ()

Verify the followings

  • Code is up-to-date with the main branch
  • No build errors after npm run build
  • No lint errors after npm run lint
  • No errors on using charites help globally
  • Make sure all the existing features working well
  • Have you added at least one unit test if you are going to add new feature?
  • Have you updated documentation?

Refer to CONTRIBUTING.MD for more details.

@smellman smellman requested review from miya0001 and naogify March 17, 2023 04:16
@yuiseki yuiseki self-requested a review December 12, 2023 03:47
Copy link
Member

@yuiseki yuiseki left a comment

Choose a reason for hiding this comment

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

I agree with adding a Dockerfile specifically for charites, thanks for your Pull Request!

I think there should be a blank line at the end of the file.
Ref: https://stackoverflow.com/a/729795

And I thought the Dockerfile could be simpler as well.
Ref: https://github.com/yuiseki/charites-ai/blob/main/Dockerfile

Comment on lines +3 to +9
WORKDIR /usr/src/app
COPY package*.json .
RUN npm install
COPY . .
RUN npm run build
RUN npm install -g .
RUN chmod +x entrypoint.sh
Copy link
Member

Choose a reason for hiding this comment

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

I think a simple Dockerfile like the following would work fine.

Suggested change
WORKDIR /usr/src/app
COPY package*.json .
RUN npm install
COPY . .
RUN npm run build
RUN npm install -g .
RUN chmod +x entrypoint.sh
COPY . /app
WORKDIR /app
RUN npm ci
RUN npm run build
RUN npm install -g .
RUN chmod +x entrypoint.sh

RUN npm install -g .
RUN chmod +x entrypoint.sh

ENTRYPOINT [ "./entrypoint.sh" ]
Copy link
Member

Choose a reason for hiding this comment

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

I think there should be a blank line at the end of the file

Suggested change
ENTRYPOINT [ "./entrypoint.sh" ]
ENTRYPOINT [ "./entrypoint.sh" ]

@@ -0,0 +1,3 @@
#!/bin/bash -e

exec charites "$@"
Copy link
Member

Choose a reason for hiding this comment

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

Also I suggest a blank line at the end of the file

Suggested change
exec charites "$@"
exec charites "$@"

@yuiseki yuiseki requested a review from hfu December 21, 2023 00:56
Copy link
Member

@hfu hfu left a comment

Choose a reason for hiding this comment

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

Thank you all!

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