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

Add docker development environment #1504

Merged

Conversation

@timoschwarzer
Copy link
Contributor

commented Jun 23, 2019

I wanted to make a small contribution but I didn't want to setup a database server on my local machine so I ended up making this PR.

My goal was to make getting started with development as easy and straightforward as possible.
This PR adds a complete Docker Compose development environment.

Improvements:

  • You don't need any dependencies installed on your computer (composer, php, mysql, node etc.), only Docker and Docker Compose
  • Added an admin user to DummyContentSeeder
  • Straightforward setup in 6 steps. (See changes in readme.md)
  • After the first setup, one only needs to run docker-compose up to spin up the entire development environment (php server, mysql server, npm run watch)

@timoschwarzer timoschwarzer force-pushed the timoschwarzer:docker-development-environment branch 3 times, most recently from 0796a53 to 49195f1 Jun 23, 2019

@jameswmcnab

This comment has been minimized.

Copy link

commented Jul 27, 2019

I would love to see something like this.

Currently trying to spin the project up to create a PR and test it has been a lot more complex than it really should be.

@timoschwarzer

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

For now, you can just check out my branch, make changes and then check out master and commit.

It would be great if @ssddanbrown might take a look at this.

@jameswmcnab

This comment has been minimized.

Copy link

commented Jul 28, 2019

For now, you can just check out my branch, make changes and then check out master and commit.

It would be great if @ssddanbrown might take a look at this.

For now I'm just running a mysql container via docker, then running the frontend build and app on my local machine (app via php artisan serve).

@ssddanbrown

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

Sorry for my really late response on this and thanks for offering this pull request @timoschwarzer.

I can definitely see how this would be valuable to those wanting to help develop BookStack. There's a few things I'd want to tweak before merging into master though:

  1. I'd prefer not to litter the top-level folder with additional example configs. I think we should be able to make the .env.example.docker-development redundant by setting env vars in the app container to override DB details.
  2. I think we can avoid having an additional DB connection via the same method as above.
  3. I don't think the additional dummy content seeder is needed. An admin user is created during normal migration. Think it confuses things having a different initial login for docker-dev and non-docker-dev/production.
  4. I'd prefer not to have any files or folders with docker in the name within the top-level folder, especially the docker-compose.yml file. I'm worried people will just directly use these thinking they're some kind of official docker production offerings which is something I'd prefer to avoid. Instead, we should probably put this all into a /dev folder.
@timoschwarzer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Thank you for your response :)

I think these are all valid points I agree with.

I didn't notice that the migrations create an admin user, that's why I added that seed. Is it documented somewhere?

I'll work on this the next 1-2 weeks then.

@timoschwarzer timoschwarzer force-pushed the timoschwarzer:docker-development-environment branch from 49195f1 to 9357620 Aug 12, 2019

@timoschwarzer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@ssddanbrown

I'd prefer not to litter the top-level folder with additional example configs. I think we should be able to make the .env.example.docker-development redundant by setting env vars in the app container to override DB details.

Done. Settings are now set via environment variables inside docker-compose.yml.

I think we can avoid having an additional DB connection via the same method as above.

Same.

I don't think the additional dummy content seeder is needed. An admin user is created during normal migration. Think it confuses things having a different initial login for docker-dev and non-docker-dev/production.

Removed the dummy content seeder and added docs for the initial admin user.

I'd prefer not to have any files or folders with docker in the name within the top-level folder, especially the docker-compose.yml file. I'm worried people will just directly use these thinking they're some kind of official docker production offerings which is something I'd prefer to avoid. Instead, we should probably put this all into a /dev folder.

I disagree with this one.

  • The docker directory only contains Docker related files and I don't think someone will think that these are production-ready assets because these are all contained in a dev subdirectory.
  • I'd be against putting the docker-compose.yml somewhere other than the project root directory. Typing docker-compose -f docker/dev/docker-compose.yml <command> [args] all the time is not that great and the docker-compose.yml states in its first two lines that it's intended for development purposes only.
@ssddanbrown

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Thanks for the changes.

In regards to the directory bits, I was thinking about more of a middle ground, With a structure like this:

├── dev
│   ├── docker
│   │   ├── Dockerfile
│   │   ├── entrypoint.app.sh
│   │   └── entrypoint.node.sh
│   └── docker-compose.yml

That way it keeps docker references out of the top-level and should allow someone to just cd into the dev folder then run any composer commands like normal. I know it's not as easy as having them in the top level, and I realise I'm sounding a bit paranoid about this, but I do think that by having a docker-compose.yml in the top level people will just run it without thinking or reading the comments in the file. Just the action of them having to go into the dev dir will massively defend against that.

@timoschwarzer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

I've made the changes you requested but I have to admit that I'm not happy with the docker-compose.yml not being in the project root.

  • Running composer/artisan/docker-compose commands requires switching directories all the time
  • The docker-compose.yml now is really weird with its paths because some of them are relative to context (which is now ..) and some aren't

but I do think that by having a docker-compose.yml in the top level people will just run it without thinking or reading the comments in the file.

  • I don't see a problem with this. I mean, who clones a source repository and just blindly runs docker-compose up in it in a production environment? I don't know any project with Docker development support which has problems with people just blindly running the top-level docker-compose.yml in production.
    Some could also just run artisan serve or ./server.php. Should we move these files to dev/, too? I don't think so. :)

Quoting from the Docker page: "Compose has traditionally been focused on development and testing workflows", and you cannot throw this docker-compose.yml into a Swarm cluster by accident because it won't work.

@timoschwarzer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Note to myself @timoschwarzer: Fix CI tests

@ssddanbrown ssddanbrown merged commit c6b1f36 into BookStackApp:master Aug 26, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate 1 fixed issue
Details
@ssddanbrown

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Thanks @timoschwarzer, Now merged.

I moved the compose file back into the root folder, I thought your reasonings were fair and think I was being overly paranoid. .

@timoschwarzer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

I see you fixed/reverted the tests, too. Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.