Skip to content

Use dynamic env params for critical settings and restore Heroku deploys #399

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

Closed
wants to merge 4 commits into from

Conversation

dzuelke
Copy link
Contributor

@dzuelke dzuelke commented Nov 8, 2016

Database, log destination, and secrets are now configurable via environment variables (via the quasi standard DATABASE_URL, and SYMFONY_LOG, and SYMFONY_SECRET). All three fall back to defaults from parameters.yml(.dist).

For simplicity (one param versus many), a URL is now used again for SQLite, because that works fine since Doctrine DBAL 2.5.5.

Deploys to Heroku are now fixed again; @bocharsky-bw's PR #377 was not a fix for #371, because SQLite does not scale horizontally :p The original problem was @javiereguiluz's 56cfa66 change to Symfony 3.1, which used individual parameters for config. Again, no longer a problem since DBAL 2.5.5. It also uses php://stderr for env prod on Heroku only, which @javiereguiluz had also removed in 56cfa66 (for the future, I'd encourage separate commits for stuff like this, as it makes it much easier to untangle individual parts and reasons for changes).

This demo app ships with sample data in SQLite, but for other databases, the data needs to be seeded, and that can even happen in a prod env. @javiereguiluz' commit e2264b0 removed the fixtures bundle needed for that from the kernel init; it's now back.

This app now works just fine with the usual defaults both in dev and prod envs, but users who want to try running it on Docker or on a PaaS like Heroku can now trivially adjust relevant settings via environment variables.

/CC @stof @fabpot

Default values via parameters.yml(.dist) mean this still works like before both in dev and prod, but DATABASE_URL and SYMFONY_SECRET env vars can override each setting dynamically at runtime in Docker or on PaaSes.
Inadvertently removed in e2264b0; this is a demo app that ships with sample data in SQLite, but for other databases, the data needs to be seeded, and that can even happen in a prod env.
Commit 662e78f / PR symfony#377 removed the Postgres add-on and the initial seeding. This is not the correct solution to issue symfony#371, which arose because the config got changed back to individual parameters rather than a URL in 56cfa66. But we now use Doctrine DBAL 2.5.5, which correctly handles SQLite URLs, so there is no need for individual parameters anymore.

Also, using SQLite in production very obviously does not allow for horizontal scalability...
So users can do the right thing in Docker or on PaaSes by setting SYMFONY_LOG to "php://stderr".
# if you don't want to use SQLite, change the URL in parameters.yml or set the DATABASE_URL environment variable
url: "%env(DATABASE_URL)%"

# instead of using a URL, you may also uncomment the following lines to configure your database
Copy link
Member

Choose a reason for hiding this comment

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

you also need to comment the URL (as the URL wins over these params if you configure both)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why it says "instead of" ;)

@@ -21,6 +21,7 @@ public function registerBundles()
new WhiteOctober\PagerfantaBundle\WhiteOctoberPagerfantaBundle(),
new CodeExplorerBundle\CodeExplorerBundle(),
new AppBundle\AppBundle(),
new Doctrine\Bundle\FixturesBundle\DoctrineFixturesBundle(), // used for initial population of non-SQLite databases in production envs
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it makes sense in a "real" application, might make sense as a demo app though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and no. Even a real application, when first deployed somewhere, may need e.g. an initial user inserted into the database ;)

@@ -9,12 +9,19 @@
"repository": "https://github.com/symfony/symfony-demo",
"logo": "https://symfony.com/images/v5/pictos/demoapp.svg?v=4",
"success_url": "/",
"scripts": {
"postdeploy": "php bin/console doctrine:schema:create && php bin/console doctrine:fixtures:load -n"
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it will fail on the 2nd push, won't it? Maybe php bin/console doctrine:schema:update --force is a better command here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The name postdeploy for the script is, unfortunately, misleading; it is run only after the very first deploy using the "Deploy to Heroku" button.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now I see, thanks!

@dzuelke
Copy link
Contributor Author

dzuelke commented Nov 9, 2016

So... merge? ;)

@javiereguiluz
Copy link
Member

@dzuelke thanks for fixing all the issues that I introduced a while ago. Your solution is exactly what we were looking for: something that works perfectly for cloud and non-cloud users. It's merged now. Thanks!

@dzuelke
Copy link
Contributor Author

dzuelke commented Nov 10, 2016

Awesome, thank you so much @javiereguiluz! Like I said, if there's ever again any problems related to Heroku deploys, or just generally Twelve Factor, Cloud, PaaS, env vars... just ping me and I can chime in!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants