dorianim / mrbs-docker

Docker image for the meeting-room-booking-system including my modern-mrbs-theme
https://hub.docker.com/r/dorianim/mrbs
GNU Affero General Public License v3.0
19 stars 6 forks source link

feat: Add postgresql database support #3

Closed claneys closed 1 year ago

claneys commented 1 year ago

Hello,

I find your repo quite useful and just wanted to use postgresql db instead. So I did some changes to make it works. I also added some improvements. I use it in kubernetes at the moment and works nice.

It makes image heavier though, maybe could be better to split in a separate images.

dorianim commented 1 year ago

Thanks for the Update @claneys ! I've looked at some other containers from linuxserver.io, and noticed, that they do not prefix the database variables there. So I'd prefer not to do that either. Is there any good reason to do so?

claneys commented 1 year ago

I still have some problems but it should improve from your side. I have to deal with some issue from my environment before being certain that it fixes the problems.

dorianim commented 1 year ago

Unfortunately, it still doesn't work properly. It only works after removing the getenv('MRBS_DB_*') ?? part. I think, it would be easier to just not add the MRBS prefix and leave it as it is.

claneys commented 1 year ago

Ok found. Have to dig a bit on how php retrieve its env var, explanation in the commit message :

Null coalescence operator ?? works on 'null' value, undefined variable, and getenv() returns false or something so that variable is defined with an empty but not undefined value.
So using a ternary operator works better on using getenv() function. We could use $_ENV but env var could be found in $_ENV or $_SERVER depending which is the
source of the environment variable, it seems more reliable to use getenv() then.

This is a bit more verbose way to write it but works every times.
dorianim commented 1 year ago

I decided to drop the MRBS prefix. Thanks for your contribution!