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

chore: make config overridable by environment variables #2

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

Conversation

alexandre-abrioux
Copy link
Contributor

Hi!

I have a use case where I need to run the node with some config values that differ from the predefined constants in config.ts.

To do so we currently need to clone the repository, edit the file config file, re-build the Docker image, and repeat for every new release, which is quite tedious.

In this PR I introduced a new .env file which contains default values, and set-up the dotenv module which loads those default values or the ones already defined by the user in the environment.

The .env file contains every variable defined in config.ts plus all the other environment variables that are used in the app. Doing so allows new users to easily get a grasp on allowed environment variable with a quick glance at the file (self-documenting).

Please tell me if you have any question or you would like to see more improvement to the work I've done. Thanks!

@tuler
Copy link
Member

tuler commented Aug 9, 2021

dotenv is often used as a method to override variables defaults, but defaults are still hardcoded.
And usually you don't commit .env files to git or bake into docker image. What are your thoughts about that?

@alexandre-abrioux
Copy link
Contributor Author

And usually you don't commit .env files to git

It all depend of the context.
Don't commit .env files that are tied to an actual environment. Don't commit secrets.
Actually, never use .env files to setup your environment if possible.

But this particular .env file only purpose is to define application defaults. We could call it .env.default or .env.dist.
However it is not meant to be redefined by the user, that is why it is simply called it .env.
The user will not create a .env file to load its custom configuration.

dotenv is often used as a method to override variables defaults

The sole purpose of dotenv here is to load the default values.
The user will use actual environment variables through its Docker or Kubernetes deployment to override those values.

This pattern is used and recommended by Symfony (leading PHP framework / industry standard) since its version 4.

A) The .env.dist file no longer exists. Its contents should be moved to your .env file (see the next point).
B) The .env file is now committed to your repository. It was previously ignored via the .gitignore file (the updated recipe does not ignore this file). Because this file is committed, it should contain non-sensitive, default values. The .env can be seen as the previous .env.dist file.

I'm used to working with this pattern and find it pretty convenient since all environment variable defaults are listed in a single file.
This is why I choose this solution.

If you feel more comfortable with just a config file and no .env file we could do something like this:

// config.ts

export const TIMEOUT = process.env.TIMEOUT ? parseInt(<string>process.env.TIMEOUT) : 24 * 60 * 60 * 1000;
export const RETRY_INTERVAL = process.env.RETRY_INTERVAL ? parseInt(<string>process.env.RETRY_INTERVAL) : 10000;
export const POLLING_INTERVAL = process.env.POLLING_INTERVAL ? parseInt(<string>process.env.POLLING_INTERVAL ) : 30000;
...

; but we would miss the opportunity to list other vars that are not present in the config, for convenience for advanced users:

- URL (documented)
- GAS_PRICE_PROVIDER (documented)
- GAS_STATION_API_KEY (documented)
- MNEMONIC (not documented atm)
- MNEMONIC_PATH (not documented atm)
- SEED (not documented atm)

We could also add all those vars is the config file but it is more rework, if you agree.

Tell me what you think :)

GAS_STATION_API_REQUEST_TIMEOUT_MS=10000

# When loading wallet from MNEMONIC
MNEMONIC=

Choose a reason for hiding this comment

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

Not a good idea to have the option to set the MNEMONIC here, some users might set this and it's definitely very unsafe.

Copy link
Contributor Author

@alexandre-abrioux alexandre-abrioux Aug 27, 2021

Choose a reason for hiding this comment

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

It is very unsafe indeed but the only way as far as I know to start the node without a TTY, in a CI pipeline for instance. We could provide an option to use Docker Swarm Secrets or Kubernetes File Secrets with an environment variables like MNEMONIC_FILE targeting the path of a mounted file in the container, but it probably is worth opening a whole new issue for this. The MNEMONIC env var is an undocumented feature but it could be nice to reference the variable here for advanced users who are able to add other layers of security to prevent leaks until a better solution is offered. We could even write a warning in the comment like so:

# [UNSAFE, DO NOT USE] When loading wallet from MNEMONIC

What do you think?

MNEMONIC_PATH=

# When loading wallet from SEED
SEED=

Choose a reason for hiding this comment

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

Same problem as MNEMONIC

Copy link

@cf-cartesi cf-cartesi left a comment

Choose a reason for hiding this comment

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

Interesting contributions, like last time 🙂

@alexandre-abrioux alexandre-abrioux changed the base branch from develop to master October 15, 2021 09:54
@alexandre-abrioux
Copy link
Contributor Author

Updated branch to resolve conflicts with master.

@alexandre-abrioux alexandre-abrioux changed the title Make config overridable by environment variables chore: make config overridable by environment variables Feb 15, 2022
@alexandre-abrioux
Copy link
Contributor Author

Rebased onto master.

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.

3 participants