Skip to content
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

Make data and challenge directory configurable in config file #22

Merged
merged 5 commits into from
May 7, 2021

Conversation

kpcyrd
Copy link
Owner

@kpcyrd kpcyrd commented May 6, 2021

Resolves #20

Copy link
Contributor

@alerque alerque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. As the subject for a possible future enhancement it might be worth looking at the config crate for handling this. I've had good luck with it paired with Clap for handling the logic of setting fallback from CLI args → runtime ENV vars → config files → defaults. It could be overkill for this usage, but the current manual handling of merge logic individually for relevant args vs. TOML keys and so forth is bound to eventually be brittle and/or repetitive.

@kpcyrd
Copy link
Owner Author

kpcyrd commented May 6, 2021

Thanks, I've been looking for something like that for some other projects too!

This seems to be working fine (and I ran into a bug with acme_email and acme_url not actually being read from the config file). Do you have any suggestions if I could write the structopt integration differently?

@alerque
Copy link
Contributor

alerque commented May 6, 2021

I've never worked directly with structopt, when I started learning Rust I went straight for clap, but they are pretty similar for this usage.

You can see one implementation I did with a set of Clap CLI definitions here, an internal runtime config manager here, and finally the binary here where the order of conf priority is actually accomplished.

Confusingly in that implementation I started out rolling my own, then later introduced config-rs. As a result CONFIG is basically my old implementation acting as a wrapper around Config. It would probably be easier to completely eliminate the wrapper and use Config directly, although the way I have it setup it then later doubles as a runtime key/value store that is usable across threads. That probably should be implemented separately from config data, but is implemented as another priority level on top of it.

@kpcyrd kpcyrd merged commit e1f1282 into main May 7, 2021
@kpcyrd kpcyrd deleted the config-paths branch May 7, 2021 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow certificate storage data dir to be permanently configured
2 participants