-
Notifications
You must be signed in to change notification settings - Fork 9
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
Adds option to specify config location using env variable #110
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #110 +/- ##
==========================================
- Coverage 89.23% 88.33% -0.91%
==========================================
Files 14 14
Lines 548 557 +9
==========================================
+ Hits 489 492 +3
- Misses 59 65 +6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a useful addition. It would be good to have a test case but I think that's easy given what we have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect there will be a merge conflict for this file if #111 gets merged first. Should be easy to resolve.
# if no path provided, try to use config environment variable | ||
cfile = os.getenv("CATS_CONFIG_FILE") | ||
if cfile is not None: | ||
try: | ||
with open(cfile, "r") as f: | ||
return yaml.safe_load(f) | ||
logging.info("Using config.yml found in CATS_CONFIG_FILE\n") | ||
except FileNotFoundError: | ||
logging.warning("CATS_CONFIG_FILE config file not found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would benefit from a test being added to test_configure.py (which will stop codecov throwing a fit). I'll see if I can add something to this PR for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a PR against your main branch - I think that will add a test to this PR if you merge it.
I've rebased this and added a test. See PR #121 which contains this patch |
Adds the option to set
CATS_CONFIG_FILE
environment variable to specify config file location. Config file location precedence becomes:CATS_CONFIG_FILE
./config.yaml