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

Dockerize app #79

Merged
merged 8 commits into from
May 26, 2023
Merged

Dockerize app #79

merged 8 commits into from
May 26, 2023

Conversation

kareemmahlees
Copy link
Contributor

What type of Pull Request is this?

Delete all options except for the one that applies

  • Feature

What is the current behavior?

Issue Number: #50

What is the new behavior?

  • An image is built for the app
  • The image was provided to the docker-compose
  • the app is dependent on the db

Other information

I would like to hear your feedback if everything is okay before moving on to add this section to the CONTRIBUTING file

Dockerfile Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@w3cj
Copy link
Member

w3cj commented Feb 27, 2023

Let's focus on a production build version of the svelte app and put it in a separate docker-compose file docker-compose.prod.yml. This file should only have the app service and not the db service.

I've added a node adapter for the svelte build. I'll add some comments on your PR on how you can target the production build in the Dockerfile

I'll note that the setup for using docker in development will be a bit different, but lets do that in a separate PR after we have the production one setup.

Dockerfile Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
- 5173:5173

environment:
AUTH_SECRET: ${AUTH_SECRET}
Copy link
Member

Choose a reason for hiding this comment

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

All of the environment variables will need to be exposed. See .env.example for a list of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the environment variables will need to be exposed. See .env.example for a list of them.

I am not really sure I understand what you mean by exposed ?!

package.json Outdated Show resolved Hide resolved
.dockerignore Outdated Show resolved Hide resolved
.dockerignore Outdated
.node_modules
.husky
.eslintignore
.eslintrc.cjs
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how .dockerignore is used exactly, but for a development container (which we'll setup later), these configuration files will be needed. They will likely be mapped to a volume that mirrors this directory.

Copy link
Contributor Author

@kareemmahlees kareemmahlees Mar 2, 2023

Choose a reason for hiding this comment

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

I'm not sure how .dockerignore is used exactly, but for a development container (which we'll setup later), these configuration files will be needed. They will likely be mapped to a volume that mirrors this directory.

Do you mean you want to create a dev container where the users can open their vscode inside the container and work from there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Two containers (docker-compose) files, dev and prod versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two containers (docker-compose) files, dev and prod versions.

I did that in the latest commit, but I kept the configuration files ignored in .dockerignore and every thing seems to run ok!

Copy link
Collaborator

@omar2205 omar2205 left a comment

Choose a reason for hiding this comment

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

adds a second stage to run the production app

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@kareemmahlees kareemmahlees requested review from omar2205 and w3cj and removed request for omar2205 March 2, 2023 15:54
@w3cj
Copy link
Member

w3cj commented Mar 3, 2023

There are several changes I need you to make, so lets just focus on the production Dockerfile and docker-compose.prod.yml for now.

Remove docker-compose.dev.yml and .dockerignore, we will add these in a future PR where we can discuss the development container specifically.

Dockerfile:

  1. Should run the command npm run build
  2. Should copy files from the generated folder called build instead of the whole app
  3. Should expose port 3000
  4. Should run the command node build

docker-compose.prod.yml:

  1. Remove depends_on: - db - the production container will connect to a remote DB
  2. Every environment variable listed in the .env.example file should be listed under environment

Remove current changes to the docker-compose.yml file

Copy link
Member

@w3cj w3cj left a comment

Choose a reason for hiding this comment

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

See comment above.

@kareemmahlees
Copy link
Contributor Author

@w3cj sorry for being late, was busy lately.
i did all the changes mentioned here:

There are several changes I need you to make, so lets just focus on the production Dockerfile and docker-compose.prod.yml for now.

but regarding this part:

Every environment variable listed in the .env.example file should be listed under environment

couldn't we just do this:

 env_file:
    - .env

@w3cj
Copy link
Member

w3cj commented Apr 7, 2023

Thanks for this. Looks good, but the original docker-compose.yml file got deleted. Be sure to leave the original as well as the new one you created.

@w3cj
Copy link
Member

w3cj commented Apr 7, 2023

I think we want to avoid doing this, because the production container likely won't have a .env file. The variables will already be loaded in the system or specified when launching the container.

@kareemmahlees
Copy link
Contributor Author

@w3cj Done, could you check?


CMD node build

FROM node:18-alpine as production
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to split the build and run into separate containers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To limit the image size?

Copy link
Member

Choose a reason for hiding this comment

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

Ok seems fine then, but does there need to be an EXPOSE and CMD for the first stage? Isn't it needed for only the second container?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, @kareemmahlees just remove the EXPOSE and CMD node build from the build stage.

@w3cj w3cj merged commit 89dee7e into CodingGarden:main May 26, 2023
w3cj pushed a commit that referenced this pull request May 26, 2023
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