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 setup #232

Closed
wants to merge 11 commits into from
Closed

Add Docker setup #232

wants to merge 11 commits into from

Conversation

teohhanhui
Copy link
Contributor

@teohhanhui teohhanhui commented Jun 1, 2018

Q A
Branch? 1.2 (or master?)
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets Sylius/Sylius#9414
License MIT

TODO:

  • Add mailhog

host: '%database_host%'
port: '%database_port%'
dbname: '%database_name%'
user: '%database_user%'
password: '%database_password%'
driver: pdo_mysql
Copy link
Contributor Author

@teohhanhui teohhanhui Jun 1, 2018

Choose a reason for hiding this comment

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

It's pointless to have database driver configurable via environment variable when the server_version is fixed here. It's also something that does not change across deployments in an application, so following Symfony Best Practices:

Application-Related Configuration

Define the application behavior related configuration options in the app/config/config.yml file.

Defining these values in parameters.yml file would add an extra layer of configuration that's not needed because you don't need or want these configuration values to change on each server.

@teohhanhui teohhanhui force-pushed the add/docker-setup branch 4 times, most recently from 490ff7a to 857a00f Compare June 1, 2018 18:01
@@ -81,7 +81,7 @@
"symfony-var-dir": "var",
"symfony-web-dir": "web",
"symfony-tests-dir": "tests",
"symfony-assets-install": "relative",
"symfony-assets-install": "copy",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary as symlinks don't work across containers.

ports:
- "80:80"

mailhog:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's unfortunate that we've set

swiftmailer:
    disable_delivery: true

in the dev and staging environments, where this would be useful.

So the user will have to change this value manually in the config files, if they use something like Mailhog.

SwiftmailerBundle does not support using an environment variable for disable_delivery, similar to symfony/swiftmailer-bundle#159

pamil added 6 commits June 7, 2018 16:35
* 1.2:
  Update to Sylius v1.2.0-RC
  Remove vendorPath command-line argument from root gulpfile
* 1.2:
  Update composer lockfile
  Remove Selenium and PerformanceExtension from dependencies
* 1.2:
  Upgrade Composer & Yarn dependencies
  Upgrade Composer & Yarn dependencies
  Upgrade Composer & Yarn dependencies
pamil and others added 4 commits June 12, 2018 15:53
* 1.2:
  Update to Sylius v1.1.7
  Update to Sylius v1.0.16
  Update to Sylius v1.2.0
* 1.2:
  Test against both Symfony 3.4 and 4.1
Copy link
Contributor

@pamil pamil left a comment

Choose a reason for hiding this comment

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

Looks quite good, could you add Travis job to run tests inside of Behat containers?

@teohhanhui
Copy link
Contributor Author

teohhanhui commented Jun 19, 2018

Sure, and I'll have to do a rebase. You used the awful GitHub "update branch" button which doesn't work doesn't do what we need it to do. 🤣

@teohhanhui teohhanhui closed this Jul 23, 2018
@teohhanhui teohhanhui deleted the add/docker-setup branch July 23, 2018 13:04
@teohhanhui teohhanhui mentioned this pull request Jul 23, 2018
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