drogon icon indicating copy to clipboard operation
drogon copied to clipboard

Add dotenv config support

Open akdotio opened this issue 2 years ago • 10 comments

  • Fix #1184

akdotio avatar Feb 26 '22 10:02 akdotio

Thanks for the PR! May you explain the use case for this any why not Json?

Thanks for taking the time to review the PR.

Now, let's say we have a server running in Docker, a very common use case would be a docker-compose file that configures the server and the database and maybe a cache database and a proxy server, the question is, how can I share the config between the docker containers and the Drogon server, config like the database host and port.

Given the example above a good solution would be using a .env file since Docker already have built-in support to reading config from a .env file.

An example config structure would be like that:

  1. docker-compose.yaml
services:
  drogon:
    image: drogon
    ports:
      - ${HTTP_PORT}:${HTTP_PORT}
      - ${HTTPS_PORT}:${HTTPS_PORT}

  redis:
    image: redis
    ports:
      - ${REDIS_PORT}:${REDIS_PORT}

  postgres:
    image: postgres
    environment:
      POSTGRES_DB: ${POSTGRES_DB}
      POSTGRES_USER: ${POSTGRES_USER}
    ports:
      - ${POSTGRES_PORT}:${POSTGRES_PORT}
  1. .env
HTTP_PORT=80
HTTPS_PORT=443
REDIS_PORT=6379
POSTGRES_DB=postgres
POSTGRES_USER=postgres
POSTGRES_PORT=5432

Now, if Drogon has build-in support for the .env file one will be able to share the config between database, cache, proxy, etc. from a single source which is the .env file.


An alternative to reading the Drogon config from the .env file would be reading/parsing the .env file and assigning the variable to the environment variables and then use that instead, which is actually how .env files was designed to work.

If that alternative makes more since I can update the implementation to do it that way instead!

akdotio avatar Feb 26 '22 23:02 akdotio

@ahmedmkamal Thanks for the quick and detailed reply. Please give me and @an-tao some time to think through it. It's quite a big change and has a lot of implications.

marty1885 avatar Feb 28 '22 01:02 marty1885

Generally speaking I like the idea, but I think we need a more generic implementation in order to be mergeable into main.

There’s also still the question open, how exactly “environment” variables should be supported, i. e. is this feature about INI file support or environment variables support?

rbugajewski avatar Feb 28 '22 15:02 rbugajewski

@rbugajewski It's about environment variables support, not INI, actually the .env file is not an INI file it's just a plain text file that declares environment variables in the form of VAR=value it may be a valid subset of INI but it's not fully compatible.

I've been looking into my implementation (trying different solutions) over the weekend and I found that this problem can be split into two parts...

  1. Adding support to parsing a .env file and assigning the parsed values to the "environment variables" ONLY if the variable is not already assigned.
  2. Adding support to reading all configuration parameters from environment variables.

Regarding the format of the .env file, after looking into how other software is parsing the .env file (specifically Docker) I found out that to be compatible with other software the parsing rules will be as follows:

  1. variables are assigned in the form of VAR=value separated by a new line.
  2. anything that comes after a # sign is a comment unless it's wrapped in quotes.
  3. empty values are treated as empty strings VAR= equals VAR="".
  4. white spaces are trimmed unless it's wrapped in quotes.
  5. namespaces [NAMESPACE] are not supported.

Side Note: I'm usually referencing Docker in this context because one of the main use cases of this feature will be sharing the configuration with Docker or rather reading the configuration parameters from environment variables that are being assigned with Docker.

akdotio avatar Feb 28 '22 18:02 akdotio

@rbugajewski It's about environment variables support, not INI, actually the .env file is not an INI file it's just a plain text file that declares environment variables in the form of VAR=value it may be a valid subset of INI but it's not fully compatible.

I've been looking into my implementation (trying different solutions) over the weekend and I found that this problem can be split into two parts...

Thanks for the clarification. So if Docker’s .env files are a subset of INI files, I think we could reuse an existing library instead of reinventing the wheel (e. g. iniparser is license-compatible to Drogon, and has Chinese & English documentation, so in some way it fits into this project). I would also think it is better to have this dependency optional as I don’t see it as a core feature required for the framework to work. What do you think @an-tao, @marty1885?

Regarding the format of the .env file, after looking into how other software is parsing the .env file (specifically Docker) I found out that to be compatible with other software the parsing rules will be as follows:

  1. variables are assigned in the form of VAR=value separated by a new line.
  2. anything that comes after a # sign is a comment unless it's wrapped in quotes.
  3. empty values are treated as empty strings VAR= equals VAR="".
  4. white spaces are trimmed unless it's wrapped in quotes.

This is basically how almost every INI parser works.

  1. namespaces [NAMESPACE] are not supported.

I’m afraid this is incompatible with Drogon. How would you map the environment variables to a real configuration without sections, @ahmedmkamal?

Side Note: I'm usually referencing Docker in this context because one of the main use cases of this feature will be sharing the configuration with Docker or rather reading the configuration parameters from environment variables that are being assigned with Docker.

I think it’s a great use case and it saves some work. It’s a pragmatic feature and I like the concept 👍

rbugajewski avatar Feb 28 '22 19:02 rbugajewski

I just had a completely different thought: Maybe it would be better for a first step to extend drogon_ctl with a subcommand to generate a .env file from a JSON configuration?

This could be easily automated.

rbugajewski avatar Feb 28 '22 19:02 rbugajewski

@rbugajewski

I’m afraid this is incompatible with Drogon. How would you map the environment variables to a real configuration without sections?

maybe using DROGON_SSL_CERT=path/to/cert for the parameter { "ssl": { "cert": "path/to/cert" } } and DROGON_APP_NUM_THREADS=0 for { "app": { "number_of_threads": 0 } } etc... do you think that will work?

akdotio avatar Feb 28 '22 22:02 akdotio

I just had a completely different thought: Maybe it would be better for a first step to extend drogon_ctl with a subcommand to generate a .env file from a JSON configuration?

This could be easily automated.

I believe I can do that, should I start working on it?

akdotio avatar Feb 28 '22 22:02 akdotio

I’m afraid this is incompatible with Drogon. How would you map the environment variables to a real configuration without sections?

maybe using DROGON_SSL_CERT=path/to/cert for the parameter { "ssl": { "cert": "path/to/cert" } } and DROGON_APP_NUM_THREADS=0 for { "app": { "number_of_threads": 0 } } etc... do you think that will work?

I think in this case there will be issues to create a generalized mapping, because you will have to somewhere encode that DROGON_APP_NUM_THREADS maps to app, then to number_of_threads.

Even if we would add some heuristic this would be an issue. You can’t know if APP_NUMBER_OF_THREADS maps to app_number and of_threads, or app and number_of_threads.

We could introduce a separator like double underscore (APP__NUMBER_OF_THREADS). But this could easily break, and would look unmanageable in the configuration file.

rbugajewski avatar Mar 01 '22 09:03 rbugajewski

I think we need an intermediate layer, consider the scenario of adding a new option, if we don't have an intermediate layer, we have to add support for this in json parser and env parser.

json parser ---------
evn parser -------------- intermediate layer-------drogon::app().configInterface()
yml parser ----------
...

an-tao avatar Mar 01 '22 11:03 an-tao