Refactor CONFIG_LOCATION to use CONFIG_DATADIR #3547 - #3549
guptapratykshh wants to merge 3 commits into
Conversation
|
|
||
| # load configuration files and initialize globals | ||
| configFile = Path(env.setdefault("CONFIG_LOCATION", "config.yml")) | ||
| config_location = env["CONFIG_LOCATION"] |
There was a problem hiding this comment.
i dont like the square bracket syntax on env. It seems like this is liable to crash if CONFIG_LOCATION isnt set. Also CONFIG_LOCATION was removed in this PR, so its more likely to be unset
There was a problem hiding this comment.
I have updated code to use os.getenv('CONFIG_LOCATION') which returns None safely if variable is unset.
| if config_location: | ||
| configFile = Path(config_location) | ||
| elif env["CONFIG_DATADIR"]: | ||
| configFile = Path(env["CONFIG_DATADIR"]) / "config.yml" | ||
| else: | ||
| configFile = Path("config.yml") |
There was a problem hiding this comment.
Did you choose to have three branches of if/else for backwards compatibility?
There was a problem hiding this comment.
Yes,exactly. The first branch supports legacy CONFIG_LOCATION variable if it is still present, second handles new CONFIG_DATADIR standard, and third provides safe default fallback.
| @@ -1,4 +1,4 @@ | |||
| export CONFIG_LOCATION="config.yml" | |||
| export CONFIG_DATADIR="." | |||
There was a problem hiding this comment.
im unsure about defaulting this to the current directory. would want to understand where this file is run first.
@sgoggins is this part of a manual install?
There was a problem hiding this comment.
Yes, this preserves the exact previous behavior. Since old script exported config.yml (implicitly in the current directory), setting CONFIG_DATADIR='.' ensures new resolution logic still finds ./config.yml in that same location."
Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>
Signed-off-by: guptapratykshh <pratykshgupta9999@gmail.com>
ba9d02d to
c63d420
Compare
|
Hello! Just wanted to check in to see if you were still interested in helping the maintainers merge this PR. We noticed it has been a little while since this last had activity, and are considering closing it or taking it over if it remains in its current state. Please react to or reply to this to confirm your interest in the next 7 days or let us know if you are no longer interested in this so we can best prioritize everyone's contributions. Thanks! |
Description
I have changed how the configuration file path is resolved to match the new config directory standards. Instead of using separate CONFIG_LOCATION variable, the system now builds the path using CONFIG_DATADIR. This simplifies docker-compose.yml file by getting rid of unnecessary variables while maintaining backward compatibility.
This PR fixes #3547.
Notes for Reviewers
Signed commits