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

Enable docker, #18 #19

Merged
merged 4 commits into from
Sep 8, 2017
Merged

Enable docker, #18 #19

merged 4 commits into from
Sep 8, 2017

Conversation

Awkan
Copy link
Contributor

@Awkan Awkan commented Sep 6, 2017

No description provided.

@Awkan Awkan changed the title Enable docker, #18 WIP: Enable docker, #18 Sep 6, 2017
@@ -0,0 +1,2 @@
HTTPD_PORT=80
PMA_PORT=8090
Copy link
Member

Choose a reason for hiding this comment

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

Missing new line at EOF

Copy link
Member

Choose a reason for hiding this comment

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

Fixed

build: docker/php
volumes:
- .:/var/www/
- ./docker/php/php.ini:/usr/local/etc/php/conf.d/custom.ini
Copy link
Member

Choose a reason for hiding this comment

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

you already copy this file in the dockerfile ;)

web/app_dev.php Outdated
@@ -19,7 +19,7 @@
header('HTTP/1.0 403 Forbidden');
exit('You are not allowed to access this file. Check '.basename(__FILE__).' for more information.');
}
//*/
*/
Copy link
Member

Choose a reason for hiding this comment

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

Protip: this modification is useless. When you remove the first / it automatically comments the whole block and when you add it, it uncomments the block. :)

web/app_dev.php Outdated
@@ -10,7 +10,7 @@
// This check prevents access to debug front controllers that are deployed by accident to production servers.
// Feel free to remove this, extend it, or make something more sophisticated.

//*
/*
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 👎 with this change as it may generate a serious security issue if you forget to drop the app_dev.php file. Otherwise adding the environment in the .env file looks like a good idea to me.

You'll need to change the app.php file to support an environment var.

"less": "1.7.*",
"bower": "*",
"uglify-js": "*",
"uglifycss": "*"
Copy link
Member

Choose a reason for hiding this comment

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

👍


COPY php.ini /usr/local/etc/php/conf.d/custom.ini

# End docker config
Copy link
Member

Choose a reason for hiding this comment

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

hu ? actually nope 2 last line are still docker config haha.

}

# DEV
location ~ ^/(app_dev|config)\.php(/|$) {
Copy link
Member

Choose a reason for hiding this comment

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

Dropping app_dev.php (as asked in a previous comment) impact this. (this section should probably simply dropped)

@@ -1,10 +1,10 @@
parameters:
database_driver: pdo_mysql
database_host: 127.0.0.1
database_host: db
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment with "You may want to change to 127.0.0.1 if you don't use docker)

}

error_log /var/log/nginx/neko_wiki.log;
access_log /var/log/nginx/neko_wiki.log;
Copy link
Member

Choose a reason for hiding this comment

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

That's great. What is bad is that it's not reported outside of the docker. 2 solutions:

  • configure (or check if it's not already the case?) to report on stderr (so it can be log by docker-compose logs)
  • share the log directory with var/logs (and don't forget to add it in the .gitignore)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it to var/logs folder

@@ -0,0 +1 @@
date.timezone= "UTC"
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. That's probably the best configuration. Servers configured in France will probably have Europe/Paris. We need to adapt the code to support many timezone... And this is a critical issue.

@Nek- Nek- mentioned this pull request Sep 6, 2017
@Awkan Awkan force-pushed the feature/18-docker branch from e479b67 to 072fe92 Compare September 6, 2017 18:15
@Awkan Awkan changed the title WIP: Enable docker, #18 Enable docker, #18 Sep 6, 2017
@Awkan Awkan force-pushed the feature/18-docker branch from 072fe92 to 55c488a Compare September 6, 2017 18:26
@Awkan
Copy link
Contributor Author

Awkan commented Sep 6, 2017

@Nek- I fixed your comments + update README.md file in order to add Docker steps

web/app.php Outdated
// Change 'sf2' to a unique prefix in order to prevent cache key conflicts
// with other applications also using APC.
/*
$apcLoader = new ApcClassLoader('sf2', $loader);
Copy link
Member

Choose a reason for hiding this comment

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

would be great to have NekoWiki here? :)

@Nek-
Copy link
Member

Nek- commented Sep 6, 2017

@Awkan thanks this work is awesome. :) I just need to test it with standard installation (without docker) then it's good to me.

@Awkan Awkan force-pushed the feature/18-docker branch from d20ba04 to f4dedef Compare September 7, 2017 07:02
COPY php.ini /usr/local/etc/php/conf.d/custom.ini

# Set default user + dir
USER www-data
Copy link
Member

Choose a reason for hiding this comment

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

This looks like an issue. IDK why but when I run the docker-compose (ubuntu/16.04) the docker cannot write in cache folder.

I replaced with this line (found on the web): RUN usermod -u 1000 www-data

That worked for me, can you try if it's ok to you?

Also adding RUN echo 'alias sf="php app/console"' >> ~/.bashrc is a great idea to me, if you can id be glad to use it. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only use USER www-data to default connect to container with www-data user. Otherwise we just have to specify user : docker-compose exec --user www-data php bash. I'll remove the line since www-datais created by default.

It give:

# cat /etc/passwd | grep www-data
www-data:x:33:33:www-data:/var/www:/usr/sbin/nologin

I'll add alias ;-)

Copy link
Member

Choose a reason for hiding this comment

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

I tried also without this clause... I was surprise to see that even without this (which means root?! right??). I still had rights issue. Maybe root is not the 1000 user? It's something weird with docker I don't explain for now.

Btw it's probably fixed on dockermac because they run it inside a VM that have no special rights on.

Copy link
Member

Choose a reason for hiding this comment

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

It may be because of ACL I used on my cache directory (well it's probably because of that). As my user (nek) is 1000 on my computer, forcing www-data to 1000 inside the docker fix the issue ("www-data" use the same id as "nek" on my computer and all is good).

This fix may be an issue if somebody comes out with a user with id 1001 :')... I don't have a better solution for now.

@Awkan Awkan force-pushed the feature/18-docker branch from f412951 to 0b8c35b Compare September 8, 2017 07:10
@Nek- Nek- merged commit b88d324 into Nekland:master Sep 8, 2017
@Awkan Awkan deleted the feature/18-docker branch September 8, 2017 08:12
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.

2 participants