Conversation
|
This is good! Unit Test failure seems related to changes. Please take a look |
config/configUtils.js
Outdated
| logging_rotation_workerinterval: 'logging.rotation.workerInterval', | ||
| }; | ||
|
|
||
| const CONFIG_PATH_KEYS = [ |
There was a problem hiding this comment.
There are also a number of TLS certificate paths that can be configured, and it also possible to configure paths for loggers for any component, which isn't a key that that can known ahead of it. I think we probably need to have a mechanism for reading a path from the config file and use that utility in any code that needs to get a path from config.
There was a problem hiding this comment.
I'm thinking of adding a getConfigPath(param) utility that resolves any config value against rootPath, but there are muliple existing getConfigValue() callers that expect absolute paths. Should I also keep resolving the known core paths in memory at startup for backwards compat, or do you suggest a a different approach?
There was a problem hiding this comment.
I think we really need to update all code that calls getConfigValue() to get path and change it to use your proposed getConfigPath (that's a great idea). Converting paths in memory seems like it would be problematic for saving changes to the config file, possible? (we don't want to change to absolute paths any time there is an edit to a config file).
Great catch, I didn't update the unit tests. Looks like it's passing now after the fix. |
| * Use this for any config param that represents a file/directory path. | ||
| * @param param | ||
| */ | ||
| function getConfigPath(param) { |
There was a problem hiding this comment.
In some places we are replacing resolvePath with getConfigPath, but I think resolvePath also handled tilde references to the home directory (like '~/hdb/logs'). Maybe we should add that to getConfigPath as well (at least for back-compat, if not also for convenience)?
There was a problem hiding this comment.
that's a great call, I'm thinking this should work d6676b4
|
FWIW, when I run this, I do still see one absolute path: |
4 participants
No description provided.