Zuehlke/ConfZ

Make file_from_cl command line argument and file_from_env environment variable optional

ajanon opened this issue · 2 comments

As of ConfZ 1.6.0, it is not possible to set ConfZFileSource sources at runtime through the command line or environment while still allowing the CL argument or environment variable to be empty or unset. I will use file_from_cl mostly here, but the same behavior can be seen with file_from_env.

Given the following sources:

CONFIG_SOURCES = ConfZFileSource(file_from_cl="--config-file")

ConfZ will try to get the config from the command line, which works as expected if provided. However, if I would also like my Config to fall back to another source down the line, or even to hard-coded fallback values defined in the class itself, this does not work. ConfZ now requires the argument to be passed to the application. Adding optional=True does not make the command line argument optional, it only means that it will not fail if the file does not exist.

For instance, if I want to use the following config:

class Config(ConfZ):
    CONFIG_SOURCES = [
        ConfZFileSource(file_from_cl="--config-file", optional=True),
        ConfZCLArgSource()
    ]

    value: int

To start such an application without any config files (e.g., for testing purposes) and to pass all values in the command line, you need to call it with:

app --config-file "not_a_config.yml" --value 10

With not_a_config.yml not existing on the disk.

This works fine, but is a bit awkward.

Calling the same application with app --value 10 fails with: Command-line argument '--config' not found.

For this use case, I would expect optional to also make the command line argument optional.

This issue seems related to #23.

I am not sure what behavior would make sense here.

For my proposal, it seems most if not all changes are in file_loader.py. Something like this should work (not tested):

@classmethod
def populate_config(cls, config: dict, confz_source: ConfZFileSource):
    try:
        file_path = cls._get_filename(confz_source)
    except ConfZFileException as e:
        if confz_source.optional:
            return
        raise e
    file_format = cls._get_format(file_path, confz_source.format)
    file_content = cls._read_file(
        file_path, file_format, confz_source.encoding, confz_source.optional
    )
    cls.update_dict_recursively(config, file_content)

This is a quick hack though, maybe you would like something more robust and maintainable.

If however, this looks good enough to you, I can submit a PR for this feature.

I agree, the current bebaviour is unexpected, optional should also apply to file_from_cl and file_from_env. I would welcome a solution / PR as you proposed, catching the error, as the alternative (in each specific case preventing the error if it is an optional source) is not really more maintainable / robust in my opinion.
One thing I think would make sense it so make it a bit more consistent by also catching the file read error outside the _read_file method and removing the prevention of the error within the function.

So if you would find the time for a PR, this would be great!