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

Use docker-compose extensively and put nodejs available so all commands works ok #1057

Merged
merged 3 commits into from
Jan 29, 2020

Conversation

ktecho
Copy link
Contributor

@ktecho ktecho commented Dec 25, 2019

Install nodejs alongside php so bakery bake commands can be executed.

Use docker-compose extensively so you know in which container you're running your commands.

@ktecho ktecho mentioned this pull request Dec 25, 2019
@ktecho
Copy link
Contributor Author

ktecho commented Dec 25, 2019

@lcharette Please, could you give a try with this commands on MacOS? It works flawlessly in Ubuntu Linux 19.10. If it works and you're ok with the changes, I'll create a new version of the README.md and the docs:

git clone https://github.com/userfrosting/UserFrosting.git userfrosting

chmod 777 app/{logs,cache,sessions}
cp app/sprinkles.example.json app/sprinkles.json
cp app/.env.example app/.env

docker-compose build
docker-compose up -d
 
docker-compose exec app sh -c "composer update"
docker-compose exec app sh -c "cd /app/build ; npm install ; npm run uf-assets-install"
docker-compose exec app sh -c "php bakery migrate"
docker-compose exec app sh -c "php bakery create-admin"

And you can try this command that doesn't work in the 984 PR, because now we have nodejs installed in the "app" machine also:

docker-compose exec app sh -c "php bakery bake"

The reason I changed the name from "php" to "app" is because it's no longer a php container anymore, but the container where all the commands related to the app are run (php or nodejs).

Comments?

@ktecho
Copy link
Contributor Author

ktecho commented Dec 25, 2019

@Silic0nS0ldier Would be nice to know your opinion on this also. Thanks!

@codecov
Copy link

codecov bot commented Dec 25, 2019

Codecov Report

Merging #1057 into hotfix will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             hotfix    #1057   +/-   ##
=========================================
  Coverage     66.79%   66.79%           
  Complexity     1933     1933           
=========================================
  Files           162      162           
  Lines          6748     6748           
=========================================
  Hits           4507     4507           
  Misses         2241     2241

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81a9d84...23012b6. Read the comment docs.

@ktecho ktecho changed the title Use_docker-compose_extensively Use docker-compose extensively and put nodejs available so all commands works ok Dec 26, 2019
@lcharette lcharette self-assigned this Dec 26, 2019
@lcharette lcharette added this to the 4.3.x milestone Dec 26, 2019
@lcharette lcharette changed the base branch from master to hotfix December 26, 2019 23:40
@Silic0nS0ldier
Copy link
Member

Functionality and change wise this all looks fine.

I still plan on eventually having a dedicated "tooling" image in the future (hope is to have development and production configurations that are similar enough to reduce chance of production specific issues). #1004 and #1004 are where these are being tracked.

Haven't tested Docker for Windows under this configuration (I've got the edge Docker for Windows build with WSL2 on all my machines, so stable would still need to be tested).

@ktecho
Copy link
Contributor Author

ktecho commented Dec 30, 2019

@Silic0nS0ldier Have you tried MacOS?

Can somebody test this in Docker for Windows? Thanks!

@ktecho
Copy link
Contributor Author

ktecho commented Jan 11, 2020

Any comment on this one? If you want to merge it, I can do the changes to the documentation.

@Silic0nS0ldier
Copy link
Member

@ktecho There appears to be an error in the nginx config for this due to a reference to the service name.

nginx_1    | 2020/01/23 22:18:25 [emerg] 1#1: host not found in upstream "php" in /etc/nginx/conf.d/default.conf:14
nginx_1    | nginx: [emerg] host not found in upstream "php" in /etc/nginx/conf.d/default.conf:14
userfrosting_nginx_1 exited with code 1

Fix should be to replace php with app in docker\nginx\default.conf:14.

Copy link
Member

@Silic0nS0ldier Silic0nS0ldier left a comment

Choose a reason for hiding this comment

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

There is a bug in the nginx config, as said in other comment

@Silic0nS0ldier
Copy link
Member

NGINX fix did the trick, tested locally (Docker WSL2). Rather slow, but that is an existing problem that even barebone Slim sites have under my setup.

@ktecho
Copy link
Contributor Author

ktecho commented Jan 24, 2020

@Silic0nS0ldier Thanks for the change. I didn't have the time to do it today.

The documentation can be simplified, as bakery bake takes care of a lot of stuff. I hope I have the time later today to make the changes. The only problem I've had is one I think you already had: sometimes I have to run composer update twice. I still have to investigate it, but maybe having the vendor folder created in advance can improve this.

@ktecho
Copy link
Contributor Author

ktecho commented Jan 25, 2020

README.md updated here.

@lcharette lcharette changed the base branch from hotfix to develop January 28, 2020 15:03
@lcharette
Copy link
Member

As discussed with @Silic0nS0ldier on the chat, I'll merge this into develop for release in UF 4.4, just in case. Same goes for #1060

@ktecho
Copy link
Contributor Author

ktecho commented Jan 28, 2020

@lcharette When are you going to merge it? I need to make changes to learn too with the instructions.

@lcharette lcharette merged commit cee8333 into userfrosting:develop Jan 29, 2020
@lcharette
Copy link
Member

@ktecho Done. You can submit the PR for learn on the develop branch. I'll merge everything into master in a couple of days when 4.4 is fully ready.

@ssnukala
Copy link
Contributor

ssnukala commented Feb 2, 2020

Sorry for jumping in late. here is a way I was handling the missing node modules in the docker build.

Here are the changes I made on my side to get the docker build work consistently

  1. docker/node/Dockerfile
FROM node:alpine
RUN apk --update add --no-cache git
RUN echo '{ "allow_root": true }' > /root/.bowerrc
  1. Then add the following section to docker-compose.json
  node:
    image: node:alpine
    build:
      context: ./docker/node
    volumes:
      - .:/app
    working_dir: /app/build
    command: npm run uf-assets-install
  1. These are the install scripts for installing uf on docker
chmod 777 app/{logs,cache,sessions}
docker-compose build --force-rm --no-cache

docker-compose run composer install --ignore-platform-reqs --no-scripts
docker-compose run node npm install
docker-compose run composer update --ignore-platform-reqs --no-scripts
docker-compose up -d

echo -n "Enter Docker Container Name --> "
read docker_container
docker exec -it -u www-data $docker_container sh -c 'php bakery migrate'
docker exec -it -u www-data $docker_container sh -c 'php bakery create-admin'

@lcharette lcharette removed this from the 4.5.x milestone Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants