-
Notifications
You must be signed in to change notification settings - Fork 29
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
basic dockerfile with usage #2
base: main
Are you sure you want to change the base?
Conversation
Forgive me but I have no clue how to use Docker. But it looks like it's just including the main app, but we should also launch the embeddings server (https://github.com/openai/chatgpt-retrieval-plugin) as part of the container right? The DigitalOcean button deploys both things. |
The embeddings server is a separate container image. That Dockerfile is located at https://github.com/openai/chatgpt-retrieval-plugin. Something like docker compose could be used to deploy both locally, but so could other tools. |
WORKDIR /app | ||
|
||
# Copy the package.json and package-lock.json files to the working directory | ||
COPY package*.json ./ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repository contains a yarn.lock file, so it should grab that file and install with yarn
instead of npm install
to grab the frozen dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, lets make this change 👍
COPY . . | ||
|
||
# Expose port 3000 | ||
EXPOSE 3000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the same mistake in my Dockerfile, but the index.js
is using an environment variable PORT
to set the port, or defaulting to 1333. So this should be 1333 or a PORT environment variable should be set in the Dockerfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, lets set a PORT env var please
Can do a swarm doc or helm chart, helm chart will take a bit more time to piece together and test. I can build off your PR or mine, lmk |
If it makes sense to just have this one then lets roll with this for now and can build on it later |
@@ -0,0 +1,20 @@ | |||
# Use an official Node.js runtime as a parent image | |||
FROM node:14-alpine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node 14 is now EOL. The most recent LTS version is node:18.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also developing and testing on 18 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok min version is 16, I just added nvmrc file and engines
prop in the package.json
If @AntonioDeJesus doesn't get back to us, feel free to re-open your PR @stvcooke |
Dockerfile with build instructions in README to build and run locally.
Uses container.env file for easier input of third party keys