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

chore: add docker-compose.yml with postgres #3099

Merged
merged 1 commit into from
May 6, 2024
Merged

Conversation

RogerHYang
Copy link
Contributor

No description provided.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label May 6, 2024
@@ -32,7 +32,11 @@ RUN pnpm run build
# The second stage builds the backend.
FROM python:3.11-bullseye as backend-builder
WORKDIR /phoenix
COPY ./ /phoenix/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

copying too much on local build (e.g. node_modules)

Copy link
Contributor

Choose a reason for hiding this comment

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

Node modules shouldn't be copied, they are in .dockerignore. I think it keeps the Dockerfile simpler to avoid copying individual files. I'd also mention that none of the contents of this stage are included in the final build except for what is explicitly copied in the final step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ok. i guess it's copying the tox folder

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Can we add to .dockerignore instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are also other things like packages and docs. It's probably easier to copy just the specific files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we going to have more than one compose.yml for different configurations, e.g., also a separate for one SQLite? If so, should we rename this file to be more specific?

Comment on lines +37 to +39
COPY ./LICENSE /phoenix/
COPY ./IP_NOTICE /phoenix/
COPY ./README.md /phoenix/
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't make sense to copy these here since they are not included in the final image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are needed by the pip install

Copy link
Contributor

Choose a reason for hiding this comment

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

got it.

@RogerHYang RogerHYang merged commit a7d0514 into sql May 6, 2024
5 checks passed
@RogerHYang RogerHYang deleted the docker-compose branch May 6, 2024 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants