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 dotenv config support #1185

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ahmedkamalhq
Copy link

Copy link
Member

@marty1885 marty1885 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/src/ConfigLoader.cc Outdated Show resolved Hide resolved
lib/src/DotenvParser.cc Outdated Show resolved Hide resolved
lib/src/DotenvParser.cc Outdated Show resolved Hide resolved
Comment on lines 128 to 139
if (isEqual(ns, "app"))
{
if (isEqual(pair.first, "number_of_threads"))
{
configJsonValue_["app"]["number_of_threads"] = std::stoi(pair.second);
}
else if (isEqual(pair.first, "enable_session"))
{
configJsonValue_["app"]["enable_session"] = isEqual(pair.second, "true");
}
else if (isEqual(pair.first, "session_timeout"))
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a more modular way to do this? I feel maintaining this is going to be hard...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, since .env doesn't support data types I can't think of a better way to do it, but, please take a look at my comment below about assigning the variables to the environment variables and manually using that in the app.

@ahmedkamalhq
Copy link
Author

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!

@marty1885
Copy link
Member

@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.

@rbugajewski
Copy link
Collaborator

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?

@ahmedkamalhq
Copy link
Author

ahmedkamalhq commented Feb 28, 2022

@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.

@rbugajewski
Copy link
Collaborator

@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
Copy link
Collaborator

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.

@ahmedkamalhq
Copy link
Author

@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?

@ahmedkamalhq
Copy link
Author

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?

@rbugajewski
Copy link
Collaborator

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.

@an-tao
Copy link
Member

an-tao commented Mar 1, 2022

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 ----------
...

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

Successfully merging this pull request may close these issues.

Support .env config
4 participants