-
-
Notifications
You must be signed in to change notification settings - Fork 436
hello docker! #640
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
hello docker! #640
Conversation
src/Maker/MakeDockerDatabase.php
Outdated
'What name should we call the new %s service? e.g. %s', | ||
$this->serviceName, | ||
str_replace(' ', '-', Str::getRandomTerm()) | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we need to be opinionated here. If the service is called database
, then, iirc, the Symfony binary will expose DATABASE_URL
. So, we should suggest database
as the default. We should maybe even print a note about how the name will map to the exposed env var with the Symfony binary... but I'm not positive on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefix used by the Symfony is DATABASE_
by default. So, I suggest to keep it by default and strongly suggest to do so.
Then Symfony CLI does not really care about the name. It takes the service name from the Docker Compose config:
services:
db:
ports: [3306]
Here, the name is db
, so the exposed env var will be DB_URL
. So, the default service name should be database
to stick with the default Doctrine env var used by the recipe.
If one has more than one DB connection, then use a different service name like above. The command can probably tell the user the expose env vars.
src/Docker/DatabaseServices.php
Outdated
case 'postgres': | ||
return [ | ||
'image' => sprintf('postgres:%s', $version), | ||
'ports' => ['5432:5432'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can (as Fabien does) simplify these to just 5432
- e.g. ports: [5432]
And I guess this is "sort of" duplicated below in getDefaultPorts()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll have to hunt down exactly why, but i ran into a problem on a debian host - if the ports were not mapped out like [$hostPort:$containerPort]
docker wouldn't expose the port to the host.. I'll reconfirm that edge..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you only specify the container port (5432) docker will choose an ephemeral host port, you run into problems if you choose your database host as localhost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is tricky then. The Symfony binary (I believe) automatically detects the random port and uses it to expose the environment variable to the server. In that case, we do want the random behavior because it allows you to have multiple database servers (from multiple projects) running on your machine at once.
This means that this is a decision point that largely depends on whether or not you'll be using the Symfony binary to interface with Docker.
Before we figure out what to do, is my understanding accurate?
Thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5432
means: expose the port but choose a random one on the host.
5432:5432
means: expose the port and use 5432
as the port on the host.
The problem of the second option is that you cannot work on 2 projects at the same time as the 5432
port can only be mapped once. The way we solve that with the Symfony CLI is to let Docker choose a random port and automatically expose the DATABASE_URL
env var with the correct local port. That way, you can work on as many projects as you want without having to close your Docker containers whenever you switch from one project to the next.
I would use 5432
here by default as this is the most flexible one and the one used by the Symfony CLI. For people not using the Symfony CLI, we can probably assume that they know what they are doing.
$this->serviceName = strtolower($this->databaseChoice); | ||
|
||
$io->text([sprintf( | ||
'For a list of supported versions, check out https://hub.docker.com/_/%s', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use the docker hub api to get version list ?
https://hub.docker.com/v2/repositories/library/mysql/tags/?page_size=25&page=1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would happen if the api call fails? would that lockup maker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manualy ask, as you did here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only other concern would be a degraded UX on a slow connection.. don't get me wrong, i love the idea.. just trying to think of any pitfalls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're thinking about showing a few common options, like alpine
, latest
and 12
for pgsql. If you care to get a specific version, then you'll know what you want :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we grab the "suggested version" from DockerDatabaseServices::getSuggestedServiceVersion
depending on which database is selected.. right now thats either "alpine" or "latest"...
36adfa5
to
8a08c01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Nit picks :)
|
||
$io->text($statusMessage); | ||
|
||
$this->composeFileManipulator = new ComposeFileManipulator($composeFileContents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice flow
77cfe1c
to
59ba9ff
Compare
Fixing bug with YamlSourceManipulator related to comments matching values When we are walking an existing YAML structure, we try to advance to beyond a value, but YSM got confused because a comment containing that exact string value appeared before the value. This caused YSM to incorrectly put its pointer in the middle of that comment, instead of advancing to the end of the real value.
b0ec4ce
to
f272900
Compare
Thank you @jrushlow! This works great and I'm super excited about it :) |
Spin up a MySQL / MariaDB / Postgres database via docker compose! Whether you already use docker, or want to give it a try -
make:docker:database
will create or update adocker-compose.yaml
in your projects root directory...