-
Notifications
You must be signed in to change notification settings - Fork 0
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
Remove Michael's password from default config file #52
Conversation
- Also make it easier to run fibad commands by using ./fibad_config.toml as the user-supplied config in situations where no config is specified explicitly
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
+ Coverage 43.86% 43.98% +0.12%
==========================================
Files 16 16
Lines 554 557 +3
==========================================
+ Hits 243 245 +2
- Misses 311 312 +1 ☔ View full report in Codecov by Sentry. |
@@ -38,6 +39,10 @@ def get_runtime_config( | |||
with open(default_config_filepath, "r") as f: | |||
default_runtime_config = toml.load(f) | |||
|
|||
# If a named config exists in cwd, and no config specified on cmdline, use cwd. | |||
if runtime_config_filepath is None and DEFAULT_USER_CONFIG_FILEPATH.exists(): | |||
runtime_config_filepath = DEFAULT_USER_CONFIG_FILEPATH |
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'm guessing that this is meant to be a convenience so that you don't have to pass in a user config, yeah? If that's the case, I can understand where you're coming from.
However, it also feels like it might open a door for unintended settings. i.e. A user has a fibad_config.toml and a fibad_config2.toml, and fails to pass the config when they want to use the "2" version.
Maybe I'm misinterpreting the utility here though. Let me know if I'm missing a broader point.
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.
There's not a broader point to this part beyond the convenience of not passing --runtime-config
or config=
every time.
Generally this looks fine, but I would like to get your response to the comment I left, just to make sure I understand the intention. |
An example config you will probably need after this change merges looks something like:
For downloader and data loader.
If this merges I'm planning to reset my HSC password shortly thereafter in order to avoid the current situation where our pubic repo has live credentials to their system.