moodlehq / moodle-docker

A docker environment for moodle developers
GNU General Public License v3.0
373 stars 244 forks source link

Add support for reading a .env file #249

Open andrewnicols opened 1 year ago

andrewnicols commented 1 year ago

This commit adds support for a .env file to provide default environment configuration using the shdotenv project with an environment file in the POSIX format.

I have not (yet) been able to find a Window variant of shdotenv, and I'm not sure if there's already something built-in to Powershell or similar so this remains a Linux/MacOS feature for now.

skodak commented 1 year ago

By any chance did you see https://github.com/moodlehq/moodle-docker/pull/232 ?

stronk7 commented 1 year ago

So... the order of precedence (fallback if not defined) for env variables is:

  1. Already defined (via export).
  2. dotenv file.
  3. moodle-docker default.

Correct? And the fallback won't ever "overwrite" already existing ones.

Pity we cannot do this under Windows, we also try hard to keep feature-parity.

Note I'm also looking to #232, that doesn't require any external stuff (surely it's a little more "fragile" about what can process, but also, surely, what it supports is enough for moodle-docker) and comes with Windows support.

Ciao :-)

mattporritt commented 1 year ago

Hi @andrewnicols , Thanks for working on this. I’ve given it a review. I agree with @stronk7 points.

Pros: simple change Cons: doesn’t work on windows, uses a third party app (that isn’t very active)

I’ve read the shdotenv readme and understand their rationale. I’m not sure it applies so much in this case, as this is a dev tool. I’d probably prefer a solution that works in windows (as well as mac and linux), and that doesn’t require extra dependencies, over one that’s a bit more secure (again, only because this is a dev tool).

In this regard the proposed solution in #232 is a bit better. However, I don’t think it’s quite ready either. I’ll update #232 with similar feedback to here, but I think the use of source to include another bash file is an abstraction that doesn’t add a huge amount of value. I’d be happy for the logic to be all in the moodle-docker-compose wrapper. There are a couple of other small things that I’ll add to my review of that issue.

So I think the way forward here is go with #232 plus the suggested changes. Or update this issue to implement it in a similar way. I’m keen to see this functionality merged in.

Cheers, Matt P

kabalin commented 2 months ago

Just for the record, I don't see much value in either solutions TBH, they seem like an overkill for the simple env vars loading from the file.

I am using very simple approach for this. I have a folder with collections of env files and I load the one I currently need for tests. For example, the content of /home/user/docker/moodle-docker.env may look like this:

# Set up path to Moodle code
export MOODLE_DOCKER_WWWROOT=/home/user/git/moodledev
# Choose a db server (Currently supported: pgsql, mariadb, mysql, mssql, oracle)
export MOODLE_DOCKER_DB=pgsql
# PHP version.
export MOODLE_DOCKER_PHP_VERSION=8.1
# VNC port
export MOODLE_DOCKER_SELENIUM_VNC_PORT=4444
# Browser
export MOODLE_DOCKER_BROWSER=chrome

Then to start the containers for new environment, execute:

bin/moodle-docker-compose down
. ~/docker/moodle-docker.env
bin/moodle-docker-compose up -d

(the disadvantage of this approach is that each env file should override previously loaded env, i.e. they should match in terms of vars they define, but it works for my local dev requirements)

kabalin commented 2 months ago

I just done a quick test with a patch:

diff --git a/bin/moodle-docker-compose b/bin/moodle-docker-compose
index 5ae1baf..af04ac9 100755
--- a/bin/moodle-docker-compose
+++ b/bin/moodle-docker-compose
@@ -7,6 +7,8 @@ set -e
 thisfile=$( readlink "${BASH_SOURCE[0]}" ) || thisfile="${BASH_SOURCE[0]}"
 basedir="$( cd "$( dirname "$thisfile" )/../" && pwd -P )"

+[ ! -f .env ] || export $(grep -v '^#' .env | xargs)
+
 if [ ! -d "$MOODLE_DOCKER_WWWROOT" ];
 then
     echo 'Error: $MOODLE_DOCKER_WWWROOT is not set or not an existing directory'

It actually works and good thing that env vars are runtime specific (not propagated to shell like in my example in the comment above).

To test, apply the patch and execute moodle-docker-compose from moodle dir (you need to create .env file also): moodledev:main> ~/docker/moodle-docker/bin/moodle-docker-compose up -d

stronk7 commented 2 months ago

It actually works and good thing that env vars are runtime specific

Yeah, I think that to load from env file (ideally dotenv file) is quite possible, like both this and #232 are proposing.

But we need the solution to be:

With both points fulfilled, I'm happy with any solution, really. I'm also using here some pre-configured "env" files that I source all the time.

Ciao :-)