docker compose command was incorrect, dependencies are also helpful#3934
docker compose command was incorrect, dependencies are also helpful#3934LuminousHustler wants to merge 0 commit intoelementary:masterfrom
Conversation
|
@danirabbit lmk if I should take another pass at this. My 'hello world' equivalent PR before I attempt to tackle #3742 |
danirabbit
left a comment
danirabbit
left a comment
There was a problem hiding this comment.
Probably better for someone in @elementary/web-developers to review than me! I couldn’t get the docker method working properly and I think neither could @RMcNeely 😅
.github/CONTRIBUTING.md
Outdated
| ``` | ||
| cd ./dev && docker compose up -d | ||
| cd ./dev | ||
| sudo docker-compose up |
There was a problem hiding this comment.
I think the recommendation when installing docker is to add your user to the docker group so that you're not running containers as root
There was a problem hiding this comment.
I’m also not sure what the -d flag was for so I’m hesitant to approve removing it unless someone else knows
There was a problem hiding this comment.
It's for detached mode. It makes the compose environment run in the background instead of requiring an active tty. I'd think we would still want it to detach.
There was a problem hiding this comment.
Yeah I would leave it without sudo, if you really want maybe add a note about needing to have docker set up.
There was a problem hiding this comment.
I didn't realize that adding the user to the docker group would omit the need for running the container as root. I'll definitely take another stab at this PR and remove sudo. I never questioned it until you mentioned it, and a quick google shows that apparently I have been doing docker wrong for years!
Would it be cool if I added a reference/footnote or something that linked to the docker docs https://docs.docker.com/engine/install/linux-postinstall/
...unless that seems like too much hand-holding ofc. Personally I'd have found that useful to know back in the day, but here we are :)
There was a problem hiding this comment.
I'm always pro footnotes 👍
RMcNeely
left a comment
RMcNeely
left a comment
There was a problem hiding this comment.
Can we edit line 7 to either include the entire stack in docker or remove PHP?
.github/CONTRIBUTING.md
Outdated
| ``` | ||
| cd ./dev && docker compose up -d | ||
| cd ./dev | ||
| sudo docker-compose up |
There was a problem hiding this comment.
Yeah I would leave it without sudo, if you really want maybe add a note about needing to have docker set up.
LuminousHustler
force-pushed
the
master
branch
from
910fc1f to
f544f56
Compare
4 participants
Fixes # N/A
Changes Summary
This pull request [ is ] ready for review.