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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more config options via ENV & Docker #760

Merged
merged 3 commits into from Oct 21, 2020

Conversation

christian-heusel
Copy link
Contributor

Since I set up a version of the engelsystem via docker (:whale:) just now, I noticed that some of the config vars support being set via env and some dont.
I expanded the amount of variables that can be set and moved them into an extra env file.

I am very happy to discuss if people think a different approach would be more suitable! 馃槉

* correct the yaml synctax for the docker file
* create an extra env file for the optional config stuff
* modify the default config to respect the set env vars

Signed-off-by: Christian Heusel <christian@heusel.eu>
Signed-off-by: Christian Heusel <christian@heusel.eu>
Signed-off-by: Christian Heusel <christian@heusel.eu>
@MyIgel MyIgel added hacktoberfest-accepted Created during Hacktoberfest Type: Refactor Make the code better. labels Oct 17, 2020
@MyIgel
Copy link
Member

MyIgel commented Oct 17, 2020

Hi, thank you for the PR, looks good so far. I'm not sure about how useful the .env file is as it effectively doubles the places where the options have to be changed to not override them, is there another benefit of having it? (besides changing an env file instead the compose file).
Also the dockerignore part seems to be not that useful?

@christian-heusel
Copy link
Contributor Author

I'm not sure about how useful the .env file is as it effectively doubles the places where the options have to be changed to not override them, is there another benefit of having it?

Yeah I also noticed that ... I will probably just move the env vars back into the docker-compose.yml .. Its just that I am currently stuck having some options set via env vars and some as a config change and I think only one of them at a time would be better / more consistent.

I mean that is probably a design decision, but we could also remove some of the currently existing env options and remove this line from the Dockerfile and say that creating a custom config is the way to go:

RUN rm -f /app/config/config.php

I would like more env vars, thats why I wrote the PR this way :D

@MyIgel
Copy link
Member

MyIgel commented Oct 17, 2020

Yeah I also noticed that ... I will probably just move the env vars back into the docker-compose.yml .. Its just that I am currently stuck having some options set via env vars and some as a config change and I think only one of them at a time would be better / more consistent.

Imho the only config in the compose file should be that one which needs to be changed to run there (so only mysql).

I mean that is probably a design decision, but we could also remove some of the currently existing env options and remove this line from the Dockerfile and say that creating a custom config is the way to go

Baking config files into the container never was and never will be a good idea and should be avoided so the line should remain (and config files linked to containers on start). Creating a config file should only be required if some 'less used / technicaly problematic (shirts and other arrays)' configs have to be changed).

I would like more env vars, thats why I wrote the PR this way :D

Thats definitely a good way, i agree -> more env vars are better!

The plan is that at some time in the future most of them can be changed using the webinterface and the other more or less dynamically replaced by env vars but thats another issue ^^

@msquare msquare merged commit dbca216 into engelsystem:master Oct 21, 2020
@MyIgel
Copy link
Member

MyIgel commented Oct 22, 2020

I should had used *requests changes ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Created during Hacktoberfest Type: Refactor Make the code better.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants