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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

skodak
Copy link
Contributor

@skodak skodak commented Oct 7, 2022

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

My main objectives were:

  • do not require setting MOODLE_DOCKER_WWWROOT when executing compose from Moodle checkout dir
  • do not require COMPOSE_PROJECT_NAME when having multiple Moodle checkouts dir
  • allow the db default to be stored in some file in Moodle checkout dir

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

@jpahullo
Copy link
Contributor

jpahullo commented Oct 7, 2022

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
Copy link
Contributor Author

skodak commented Oct 7, 2022

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
Copy link
Contributor

jpahullo commented Oct 7, 2022

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

Thanks!!!

@skodak
Copy link
Contributor Author

skodak commented Oct 7, 2022

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

@skodak
Copy link
Contributor Author

skodak commented Oct 10, 2022

I should have Windows support for this issue later today

@skodak
Copy link
Contributor Author

skodak commented Oct 10, 2022

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

@skodak
Copy link
Contributor Author

skodak commented Oct 10, 2022

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
Copy link
Member

stronk7 commented Oct 13, 2022

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

@skodak
Copy link
Contributor Author

skodak commented Oct 20, 2022

any progress @stronk7 ?

@jpahullo
Copy link
Contributor

Any news from this?

Thanks a lot for your work!

@andrewnicols
Copy link
Contributor

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

@stronk7
Copy link
Member

stronk7 commented Mar 21, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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):

  • Make an .env.example file and commit that
  • Update the readme to refer to using and renaming the .env.example file
  • Update the script that parses the file to expect .env as the file name
  • Add .env to .gitignore so no one accidentally ever raises a PR with their credentials in it.

Cheers,
Matt P

@stronk7 stronk7 added the waiting label Apr 8, 2023
@skodak
Copy link
Contributor Author

skodak commented Apr 24, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants