Skip to content
This repository has been archived by the owner on Jun 3, 2020. It is now read-only.

config: Allow setting config path via TMKMS_CONFIG_FILE #215

Merged
merged 1 commit into from
Mar 11, 2019

Conversation

tarcieri
Copy link
Contributor

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.

@tarcieri tarcieri requested a review from liamsi March 11, 2019 17:14
Copy link
Contributor

@liamsi liamsi left a 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!

let path = config
.cloned()
.or_else(|| env::var(CONFIG_ENV_VAR).ok())
.unwrap_or_else(|| CONFIG_FILE_NAME.to_owned());
Copy link
Contributor

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.
@liamsi
Copy link
Contributor

liamsi commented Mar 11, 2019

Huh, how did we not see this before?

error[E0308]: match arms have incompatible types
  --> src/commands/mod.rs:58:22
   |
58 |           let config = match self {
   |  ______________________^
59 | |             KmsCommand::Start(run) => run.config.as_ref(),
60 | |             #[cfg(feature = "yubihsm")]
61 | |             KmsCommand::Yubihsm(yubihsm) => yubihsm.config_path(),
   | |                                             --------------------- match arm with an incompatible type
62 | |             _ => return None,
63 | |         };
   | |_________^ expected struct `std::string::String`, found str

Update: OK, the changes in the latest commit should fix this!

@liamsi
Copy link
Contributor

liamsi commented Mar 11, 2019

Merging. I wonder if adding a code-coverage report to circle-ci is sth. we should consider for this repo?

@liamsi liamsi merged commit 1b1b211 into master Mar 11, 2019
@tarcieri
Copy link
Contributor Author

@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)

@tarcieri tarcieri deleted the config-env-var branch March 11, 2019 17:41
@tarcieri
Copy link
Contributor Author

Re:

Huh, how did we not see this before?

...because I was testing locally without the --features=yubihsm flag 🤦‍♂️

Perhaps we should add a Makefile with a make test option that runs cargo test --all --all-features -- --test-threads 1. The testing situation has definitely gotten... trickier.

@tarcieri
Copy link
Contributor Author

tarcieri commented Mar 11, 2019

An alternative to the above is to eliminate the need for --test-threads 1. This can be accomplished by adding a lazy_static Mutex around the tests that need to run single-threaded (e.g. this mutex could be acquired any time tmkms is run as an external command)

@tarcieri tarcieri mentioned this pull request Mar 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants