search

d-e-s-o / nitrocli

A command line tool for interacting with Nitrokey devices.
30 stars 10 forks source link

Add otp-cache extension #146

Closed robinkrahl closed 3 years ago

robinkrahl commented 3 years ago

This PR is an iteration of PR #144: Add cache extension. It introduces the NITROCLI_RESOLVED_USB_PATH environment variable to avoid having to duplicate the device selection logic, it moves the ext.rs module into a new extension support crate, nitrokey-ext, and adds the nitrocli-otp-cache extension.

robinkrahl commented 3 years ago

Thanks for the review! I’ll fix the issues and update the PR later today.

robinkrahl commented 3 years ago

I think I’ve addressed all issues.

d-e-s-o commented 3 years ago

Hi Robin, why do you think we should have a dedicated update command? Conceptually I am fine with it (just as an alternative to --force-update), but it seems unnecessary to require users to use it. Can you please share your thoughts?

robinkrahl commented 3 years ago

Good point! The cache lookup logic became pretty convoluted so I decided to separate reading and writing the cache. (For example we have to make sure that the nitrokey::Manager is dropped before calling nitrocli.) But I‘m fine with including the update logic in the get command too. I’m not sure which way is more intuitive and useful.

d-e-s-o commented 3 years ago

Sounds good. I vote for integration.

"There is no cached slot data. Run the update command to initialize the cache."

If a computer can tell me precisely what to do most of the time I just want it to do it for me. It was definitely the case for me here.

robinkrahl commented 3 years ago

I’ve removed the update command.

robinkrahl commented 3 years ago

Do we want separate man pages for extensions?

d-e-s-o commented 3 years ago

Awesome, thank you!

Do we want separate man pages for extensions?

It's definitely not a priority right now for me. I'd hope we could cover most of it with a some decent help text. I'd rather we think about tests first on the extension front.

d-e-s-o commented 3 years ago

Fantastic work, Robin! Merged into devel.