-
Notifications
You must be signed in to change notification settings - Fork 5
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
Template docker #87
base: main
Are you sure you want to change the base?
Template docker #87
Conversation
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.
Hey! Thank you for this great contribution! It is indeed something that we would like to have :)
I did a first review. For things a little more technical, I let Jmsche go over it
.env.exemple
Outdated
# IMPORTANT: You MUST configure your server version, either here or in config/packages/doctrine.yaml | ||
# | ||
#DATABASE_URL="sqlite:///%kernel.project_dir%/var/dbsaver.db" | ||
DB_HOST=${APP_SLUG}_database |
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.
Not sure if the '_database' suffix is needed here since it can only be that if I'm not mistaken. It would be redundant. @jmsche ?
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 suffix '_database' is necessary because it is the no of the container. you can find them in the docker-compose: https://github.com/TheoMeunier/dbsaver/blob/main/docker-compose.yaml#L37
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.
Can you rename this to .env.example
?
.gitignore
Outdated
.idea | ||
.vscode |
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.
This should be part of your global .gitignore, not put in all your projects. Take a look on https://gist.github.com/subfuzion/db7f57fff2fb6998a16c
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 can confirm these should not be listed in this file.
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.
Maybe add a "Docker installation guide" :)
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.
Perfect I will make a doc to see explain the installation of docker on debian and ubuntu.
What format did you want the guide in?
in the README.md
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.
Just to be sure, I'm talking about the installation via docker of the application and not the installation of Docker in general :) (I think you had understood)
You can put that under "Installation with Task" (or that of the maker).
A very simple trick will do, it's just to signal that they can use Docker. Besides, I'm not a Docker pro. Did you set up something in the docker script to initialize the application for the 1st time, or do you have to run "task install" or "make install" before?
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.
We can do the installation of the project via "make install" or "task install" or via docker commands. So I will add the installation process via docker simply. Do you want me to update also the installation with tack to use docker?
The implementation of docker was created using my template where we can find all the information for the installation, which I could implement in this project ;)
Afterwards I could create a docker image for the whole application if the docker stack will suit you.
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 you can add it on the Task file too ;)
Wow, a docker image would be awesome!
I let @jmsche for the end of the review :)
.env.exemple
Outdated
# IMPORTANT: You MUST configure your server version, either here or in config/packages/doctrine.yaml | ||
# | ||
#DATABASE_URL="sqlite:///%kernel.project_dir%/var/dbsaver.db" | ||
DB_HOST=${APP_SLUG}_database |
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.
Can you rename this to .env.example
?
.env.exemple
Outdated
DB_PASSWORD=root | ||
DB_ROOT_PASSWORD=root | ||
DATABASE_URL="mysql://dbsaver:root@${APP_SLUG}_database:3306/dbsaver" | ||
#DATABASE_URL="postgresql://db_user:db_password@127.0.0.1:5432/db_name?serverVersion=13&charset=utf8" |
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.
You can remove this line, Postgres is not officially supported by this app.
.env.exemple
Outdated
# Format described at https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/configuration.html#connecting-using-a-url | ||
# IMPORTANT: You MUST configure your server version, either here or in config/packages/doctrine.yaml | ||
# | ||
#DATABASE_URL="sqlite:///%kernel.project_dir%/var/dbsaver.db" |
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.
You can remove this line, sqlite is not officially supported by this app.
.gitignore
Outdated
.idea | ||
.vscode |
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 can confirm these should not be listed in this file.
.gitignore
Outdated
|
||
###> symfony/framework-bundle ### | ||
/.env |
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.
As this app is primarily a Symfony app, the .env file should be versioned.
docker/php/Dockerfile
Outdated
libpng-dev \ | ||
libjpeg62-turbo-dev \ | ||
libfreetype6-dev \ | ||
libxslt-dev \ | ||
locales \ | ||
zip \ | ||
jpegoptim optipng pngquant gifsicle \ | ||
vim \ | ||
unzip \ | ||
git \ | ||
curl \ | ||
nodejs |
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.
Some packages should be removed from this list.
RUN apt-get clean && rm -rf /var/lib/apt/lists/* | ||
|
||
# Install extensions | ||
RUN docker-php-ext-install pdo pdo_mysql xsl intl gd |
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.
Are xsl & gd really necessary?
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, dependencies is realy neccessay for application, else symfony ask install
docker/nginx/nginx.conf
Outdated
# add_header X-XSS-Protection "1; mode=block"; | ||
# add_header X-Content-Type-Options "nosniff"; |
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.
To remove
@@ -0,0 +1,32 @@ | |||
server { | |||
listen 80 default; | |||
client_max_body_size 308M; |
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.
Why?
docker-compose.yaml
Outdated
# PHP Service | ||
php: | ||
build: | ||
context: . | ||
dockerfile: docker/php/Dockerfile | ||
container_name: ${APP_SLUG}_php | ||
restart: unless-stopped | ||
environment: | ||
SERVICE_NAME: ${APP_SLUG}_php | ||
SERVICE_TAGS: ${APP_ENV} | ||
working_dir: /var/www | ||
volumes: | ||
- ./:/var/www | ||
- ./docker/php/local.ini:/usr/local/etc/php/conf.d/local.ini | ||
networks: | ||
- app_network | ||
|
||
# Nginx Service | ||
nginx: | ||
image: nginx:alpine | ||
container_name: ${APP_SLUG}_nginx | ||
restart: unless-stopped | ||
ports: | ||
- "8888:80" | ||
volumes: | ||
- ./:/var/www | ||
- ./docker/nginx/nginx.conf:/etc/nginx/conf.d/default.conf | ||
networks: | ||
- app_network |
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 not sure I want to include PHP & nginx in the general Docker compose configuration. Maybe this could be moved to a docker-compose.prod.yaml file instead?
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.
you prefer using symfony cli for dev ?I don't care about that
Hi, I'll get back to you to find out what you think of my proposal for a docker application. Isox |
Hi, there are still many points to fix according to latest review. Also, you should rebase the current main branch on yours to allow merging the PR. |
Hello, I have fixed the modifications as well as the things to be fixed. Would there be other things to bring or to modify? |
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.
Hello, j'ai mit à jours le projet de Bastien sur mon fork. Ainsi j'ai mis au propre mes modificatios.
docker/php/Dockerfile
Outdated
|
||
# Install extensions | ||
RUN docker-php-ext-install pdo pdo_mysql xsl intl gd | ||
RUN npm install -g -y yarn |
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.
we would need to install the dependencies in the docker container so we need to compose
docker/php/Dockerfile
Outdated
# Get nodejs | ||
RUN curl -fsSL https://deb.nodesource.com/setup_lts.x | bash - |
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.
Why removed composer ?
@@ -0,0 +1,10 @@ | |||
date.timezone=Europe/Berlin | |||
memory_limit=512M |
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.
it's the memory limit of the application, do you want me to decrease it?
hi,
I propose you a complete docker stack for this project that I have set up on a production server and it is very stable.
I propose you to update it regularly according to the improvement of the project
I hope you will like it and don't need to come to me if you want other functionalities
;)