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

Fix multi-process image when not specifying POSTGRES_PORT_5432_TCP_ADDR #1922

Merged
merged 2 commits into from
Mar 17, 2017

Conversation

dsander
Copy link
Collaborator

@dsander dsander commented Mar 5, 2017

When DATABASE_ADAPTER is set to postgresql we do not need to start
the internal mysql daemon. The same is true when DATABASE_ADAPTER is
set to mysql2 and DATABASE_HOST is set.

Fixes #1921

When `DATABASE_ADAPTER` is set to `postgresql` we do not need to start
the internal mysql daemon. The same is true when `DATABASE_ADAPTER` is
set to `mysql2` and `DATABASE_HOST` is set.

Fixes huginn#1921
@@ -13,12 +13,12 @@ fi
# is a mysql or postgresql database linked?
# requires that the mysql or postgresql containers have exposed
# port 3306 and 5432 respectively.
if [ -n "${MYSQL_PORT_3306_TCP_ADDR}" ]; then
if [[ -n "${MYSQL_PORT_3306_TCP_ADDR}" || ("${DATABASE_ADAPTER}" == "mysql2" && -n "${DATABASE_HOST}") ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I thought you couldn't use || and had to use -o, but I think that's with [, not [[. Bash scripting is insane.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is kind of weird, as far as I know [[ supports more expressions but is not POSIX compatible, but since we only use bash anyways we can use it everywhere.

@cantino
Copy link
Member

cantino commented Mar 7, 2017

Makes sense! At this point I don't feel qualified to review Docker-related changes. I trust you to test it, and/or you can get a review from someone who's more up-to-speed than I am these days.

@dsander
Copy link
Collaborator Author

dsander commented Mar 10, 2017

Thanks, testing it manually is really the only way, there are soo many variables and different configurations. It might be worth to add some kind of automatic test, but until now the pain of manually testing has not been big enough 😛

I am waiting with the merge for a few days to give @ckir another chance to test the image.

@dsander dsander merged commit ee95a11 into huginn:master Mar 17, 2017
@dsander dsander deleted the fix-multi-process-postgres branch October 16, 2017 19:19
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.

None yet

2 participants