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

Read From Environment Variable Even If Parse File Failed #94

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

Conversation

xhsun
Copy link

@xhsun xhsun commented Aug 12, 2022

Related Issue

#79

Motivation and Context

Full Snippet of Read Configuration section of README

You can read a configuration file and environment variables in a single function call.

import "github.com/ilyakaznacheev/cleanenv"

type ConfigDatabase struct {
    Port     string `yaml:"port" env:"PORT" env-default:"5432"`
    Host     string `yaml:"host" env:"HOST" env-default:"localhost"`
    Name     string `yaml:"name" env:"NAME" env-default:"postgres"`
    User     string `yaml:"user" env:"USER" env-default:"user"`
    Password string `yaml:"password" env:"PASSWORD"`
}

var cfg ConfigDatabase

err := cleanenv.ReadConfig("config.yml", &cfg)
if err != nil {
    ...
}

This will do the following:

  1. parse configuration file according to YAML format (yaml tag in this case);
  2. reads environment variables and overwrites values from the file with the values which was found in the environment (env tag);
  3. if no value was found on the first two steps, the field will be filled with the default value (env-default tag) if it is set.

Current behavior of ReadConfig is that it will return an error if it failed to read configuration from file and the method will not attempt to read configuration from environment variables. Thus, this statement from Read Configuration section of README is not necessarily true:

reads environment variables and overwrites values from the file with the values which was found in the environment (env tag);

Because parseFile does not fill the config struct with default values and readEnvVars is the method that will fill the config struct with default values, once parseFile returns an error ReadConfig will not execute readEnvVars. Which causes the current behavior of ReadConfig invalidating this statement:

if no value was found on the first two steps, the field will be filled with the default value (env-default tag) if it is set.

To address the discrepancy, we decided to open this PR to update ReadConfig so that the method:

  • Will continue to attempt to read configuration from environment variables even if it failed to read configuration from file
  • Only returns an error if it can't read configuration from both file and environment variable
  • Fill config struct with default values even if parseFile or readEnvVars returns an error

How Has This Been Tested?

All preexisting tests passed except unknown subtest and parsing error subtest for TestReadConfig due to the behavior change we implemented for ReadConfig. Thus we modified unknown and parsing error to become unknown filetype fallback to env and parsing error fallback to env respectively. So that instead of testing to ensure ReadConfig returns an error if it failed to parse file, the updated test cases testing to ensure ReadConfig will run readEnvVars if it failed to parse file.

Two new subtests were added to TestReadConfig. The first one, called file and env parse error, was added to cover the case where neither the file or environment variables could be successfully read. The second one, called parsed file and failed to parse env, was added to cover the case where ReadConfig is able to read from file but not able to read from environment variables.

Since readDefaults method was added to prepopulate the config struct with default values, new test called TestReadDefaults, with two subtests, was added to ensure the method is properly tested.

Besides ReadConfig and the newly created readDefaults, functionalities of other methods were not modified. Thus, changes implemented in this PR should not affect other areas of this library.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

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.

None yet

2 participants