-
Notifications
You must be signed in to change notification settings - Fork 345
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
Docker #1170
base: master
Are you sure you want to change the base?
Docker #1170
Conversation
@jtojnar What do you think? Can it be merged? Or anything else 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.
Thank you for the PR and sorry I did not get to this yet.
From a glance it looks mostly okay except few things – there are too many extra files in the root of the repository. I am already trying to remove things (#913) and plan to continue in the future. Are you absolutely sure none of these files can be moved to utils
subdirectory?
Maybe at least the .dev.yml could be moved since it already needs to be referenced explicitly.
Also the dockerfiles themselves look incredibly complex. I wonder if it would be okay to just use PHP’s built in server instead of Apache.
.env.dist
Outdated
@@ -0,0 +1,2 @@ | |||
UID=1000 |
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.
Is there not a better way to mount the dev directory without needing uid/gid? Not only this adds to the clutter but it also feels a bit wonky.
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.
Unfortunately, this is a years-long issue with docker and volumes - moby/moby#2259. And in this case we need the executor of npm run dev
to match the owner of the mounted directory, which can be anything. This solutions is one of the least complicated and working.
assets/package-lock.json
Outdated
@@ -1888,10 +1888,13 @@ | |||
"dev": true |
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 not be part of this PR. Will cherry-pick to master.
@@ -26,7 +26,7 @@ | |||
"fix:server": "composer run-script fix", | |||
"install-dependencies": "npm run install-dependencies:client && npm run install-dependencies:server", | |||
"install-dependencies:client": "npm install --production=false --prefix assets/", | |||
"install-dependencies:server": "composer install --dev", |
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.
Same here.
OK, I tested how it works with docker-compose files in utils/docker and it seems to be fine. It's not standard but works. About Apache - I'd rather keep it, because there are a bunch of .htaccess files which will all stop working under php builtin server. Also php builtin server should not be used for production. What do you find complex about the Dockerfiles? They seem quite small to me, I've seen much worse :-D The production one is split into three parts using multi-staged build because we don't need npm nor composer for the production runtime environment. And the dev dockerfile basically only installs node and composer according to their official installation instructions. One minor simplification will become possible once the config.ini location is configurable or moved into data directory as suggested by #913 . |
Thanks a lot @squatica for this. |
@squatica
|
Isn't it yet ready for merge? |
The main blocker is that we do not have anyone knowledgable in Docker to properly review this. |
:(
…On 3/29/20, Jan Tojnar ***@***.***> wrote:
The main blocker is that we do not have anyone knowledgable in Docker to
properly review this.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1170 (comment)
|
Looks quite good, I have a few ideas:
|
|
It seems that now there are some package conflicts in package.lock.json due to the dependency updates. :-) |
Hi guys, I'm still alive. I rebased this PR on top of the current master & solved the conflicts. I'm looking into @kolaente's remarks, it should be possible so I'll do it. Unfortunately I lost access to my email and github started asking for email verification, so I cannot login to github web, but I can still commit. Hopefully the support will let me recover the account. If not then I'll have to continue under different username.
Hello, |
Sorry, I do not really use docker so it has been difficult for me to allocate time to it. I will try to do something with #1271 this week. |
If you need some help with it, I'm running https://github.com/armandabric/docker-selfoss since few year now. I hope it could help |
Hi,
building on top of the PR #1162 updated the branches with current master and joined mine and @akash07k 's changes together into this PR. I suggest to squash all the commits together (maybe except deasync and '--dev'), because it's pretty confusing as it is. I left them separate for now just in case you prefer to look at all the single commits.