moodlehq / moodle-docker

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

Add support for ./moodle-docker.env #232

Open skodak opened 1 year ago

skodak commented 1 year ago

See the README.md for more information how to use it.

My main objectives were:

NOTE: this feature is not supported in Windows because I do not know how to parse the env file there.

jpahullo commented 1 year ago

Hi @skodak,

According to docker-compose documentation, .env are also loaded in a prioritized way of loading variables. This is why I suggested add support for .env on #80. Natively it would be supported by Windows platforms.

However, it is a big and final step towards supporting several Moodle versions locally. Super! Congrats!

skodak commented 1 year ago

hi @jpahullo , docker .env loading is not applicable in moodle-docker-compose script because we need the environment variables BEFORE the real docker-compose is executed to create configuration for it.

moodle-docker.env file contains environment settings for moodle-docker-compose script (and for docker too).

With my patch you can still use .env, but the moodle-docker-compose script cannot use values from it.

I think I am onto something with the Windows support, I might try to make it work this weekend unless somebody volunteers.

jpahullo commented 1 year ago

Hi @skodak ! You're totally right! I missed this part when commenting.

Thanks!!!

skodak commented 1 year ago

hehe, I would not know if I did not try to create .env and failing badly...

skodak commented 1 year ago

I should have Windows support for this issue later today

skodak commented 1 year ago

argh, not there yet, I ned to find out how to check if %%1 env variable is set

skodak commented 1 year ago

now it should be finally ready for review with full Windows support, please note the "setlocal" command which makes the env variables behave more like in bash, without it the SET would propagate into the next execution and break stuff

stronk7 commented 1 year ago

(sorry for the delay, @skodak, will be looking to this ASAP!)

skodak commented 1 year ago

any progress @stronk7 ?

jpahullo commented 1 year ago

Any news from this?

Thanks a lot for your work!

andrewnicols commented 1 year ago

See also #249 which uses shdotenv to get a better support for dotenv files.

stronk7 commented 1 year ago

Hi,

is there any reason we cannot make the env file to be a truly "dotenv" (.env) one?

Also is the "define only if not set" fallback respected?

  1. Env variables already defined via export/set.
  2. Env files support introduced by this issue.
  3. moodle-docker own defaults.

Note I'm also looking to #249, really both are similar (add to the environment some variables, if they are not defined), using 2 different approaches.

I like here that it supports (not tested) Windows and that there aren't external dependencies. In the other side, surely the supported variables will be a little more limited, but I also think that it will be enough for what moodle-docker needs (simple 1-word contents).

Ciao :-)

jpahullo commented 1 year ago

Hi,

is there any reason we cannot make the env file to be a truly "dotenv" (.env) one?

Also is the "define only if not set" fallback respected?

  1. Env variables already defined via export/set.
  2. Env files support introduced by this issue.
  3. moodle-docker own defaults.

Note I'm also looking to #249, really both are similar (add to the environment some variables, if they are not defined), using 2 different approaches.

I like here that it supports (not tested) Windows and that there aren't external dependencies. In the other side, surely the supported variables will be a little more limited, but I also think that it will be enough for what moodle-docker needs (simple 1-word contents).

Ciao :-)

+1

mattporritt commented 1 year ago

Hi @skodak , Thanks for working on this. I’ve given it a review. In addition to @stronk7 comment about confirming the fallback precedence: The main change required here is to include the code in include/env.sh into the moodle-docker-compose. Including it via source is an unnecessary abstraction in this case.

Then (taking a couple of pointers from #249):

Cheers, Matt P

skodak commented 1 year ago

sorry @mattporritt , but the include/env.sh is essential when you need to use same environment in another shell script, such as waiting for moodle app

in any case I have decided to maintain my own fork because I need this for all my development, so I will not be submitting any more pull requests here