-
Notifications
You must be signed in to change notification settings - Fork 42
config: Allow setting config path via TMKMS_CONFIG_FILE #215
Conversation
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 did not test this but the changes look good to me!
src/commands/mod.rs
Outdated
let path = config | ||
.cloned() | ||
.or_else(|| env::var(CONFIG_ENV_VAR).ok()) | ||
.unwrap_or_else(|| CONFIG_FILE_NAME.to_owned()); |
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.
This should probably come with a test.
This is nice for using tmkms interactively via the command line (e.g. `tmkms yubihsm`) in cases where `tmkms.toml` is not in the current working directory.
8afe6f6
to
c980d47
Compare
Huh, how did we not see this before?
Update: OK, the changes in the latest commit should fix this! |
Merging. I wonder if adding a code-coverage report to circle-ci is sth. we should consider for this repo? |
@liamsi it'd be a bit tricky, because the tests are largely black box as opposed to unit tests, and as such would probably thwart a coverage tool. It'd be good to improve unit testing, but that'd require hoisting more of the code into library-level constructions. I can make an issue about improving the testing situation (or you can, either way) |
Re:
...because I was testing locally without the Perhaps we should add a |
An alternative to the above is to eliminate the need for |
This is nice for using tmkms interactively via the command line (e.g.
tmkms yubihsm
) in cases wheretmkms.toml
is not in the current working directory.