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

Remove default ENV to allow user provide environment variables via env files #3925

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

Conversation

ileodo
Copy link

@ileodo ileodo commented Jan 25, 2024

NextJS app allow user define environment variables by providing a .env.local file in /app/.

Those related lines in the Dockerfile are setting values to the environment variables, which will be passed in as process.env, which ranked as the top one in the loading order

This PR is removing these default variables so that user can just mount a .env.local file to set environment variable, which can avoid include the sensitive information in the docker-compose file.

Copy link

vercel bot commented Jan 25, 2024

@ileodo is attempting to deploy a commit to the NextChat Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

Your build has completed!

Preview deployment

@@ -31,9 +31,6 @@ WORKDIR /app
RUN apk add proxychains-ng

ENV PROXY_URL=""
ENV OPENAI_API_KEY=""
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better commenting the env variables so that people would know what env to set when using this docker image

Copy link
Author

Choose a reason for hiding this comment

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

maybe those documentations can stay in the readme file?

@fredliang44
Copy link
Collaborator

@ileodo I'm assuming you are going to mount .env.local file inside the docker container so that NextJS would load the env by default?

@ileodo
Copy link
Author

ileodo commented Jan 26, 2024

yep.

@ileodo
Copy link
Author

ileodo commented Feb 4, 2024

any chance to get an approval on this? thx

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

2 participants