-
Notifications
You must be signed in to change notification settings - Fork 23
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
Load config from well-known locations + add init
command for initializing config
#582
Conversation
689ae4a
to
8964d8a
Compare
I pushed up some small tweaks, including converting Exceptional errors still currently panic, which I think is possibly the right call for now, until we do the work of translating those into a |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #582 +/- ##
==========================================
- Coverage 69.17% 69.05% -0.12%
==========================================
Files 95 98 +3
Lines 12953 13491 +538
==========================================
+ Hits 8960 9316 +356
- Misses 3993 4175 +182
|
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.
Nice work on this! Looks great ✨
@QuinnWilton could we add some integration/CLI (no runtime needed) tests to have tests around the functionality here? |
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.
Added some comments thus far.
- Q: Shouldn't we also have the option to generate a PEM/DER file for users (not just provide your own) as another option? Essentially, replace the OpenSSL genkey we've/users have been doing manually? I thought this was one goal of the work?
❤️ the experience btw.
71ec5f3
to
0ab433b
Compare
I don't see any APIs exposed by the Either way, I just pushed up fixes for all of your other comments. |
0ab433b
to
bb429e8
Compare
For windows: $USERPROFILE/.config/homestar/settings.toml For macos + linux: $XDG_CONFIG_HOME/homestar/settings.toml or: $HOME/.config/homestar/settings.toml
The comments erroneously listed only ed25519 as an allowed public key type, however secp256k1 keys are also supported.
This skips writing the newly generated configuration file to disk, and instead outputs it over stdout
This is in preparation for including richer forms of interactivity around options like keypair generation and management.
This suppresses all auxiliary output, like that intended for humans, to aid in non-interactive use from a script.
This runs the command in non-interactive mode, by disabling all prompts, and instead erroring if interactive inpupt is required.
Even though Inquire will already error if we attempt to prompt for input without being attached to a TTY, explicitly running the command in non-interactive mode will ensure that our error messaging will be consistent whether or not a TTY is available
bb429e8
to
50a2543
Compare
@zeeshanlakhani I just pushed up PEM generation for ed25519, improved PEM parsing for ed25519, and the tests you requested! Headed to dinner now, but merge conflicts are fixed and I'll check CI in the morning 😊 |
homestar-runtime/src/cli/init.rs
Outdated
PubkeyConfig::GenerateFromSeed(RNGSeed::new(key_type, seed.0)) | ||
} | ||
PubkeyConfigOption::FromFile => { | ||
// HACK: We need to manually instantiate the struct with a custom formatter, |
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.
not sure we need to call this a hack vs constraints on the type/impls?
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 guess my other Q is if we had to use PathBuf, but I'm guessing yes.
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.
We don't necessarily need PathBuf
, but my original reasoning was:
- Supporting non-UTF-8 paths
- Hooking into
Inquire
's error handling flow, for handling parse errors by displaying an error and re-prompting the user
(1) may not apply because we still construct the PathBuf
by going through the parser
closure, which is over a &str
, and so will only contain UTF-8 codepoints anyway.
(2) also doesn't look important, because it seems like the FromStr
impl for PathBuf
is Infallible
, so we shouldn't need to worry about error handling there.
Want me to rip this out and just return the input as a String
?
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.
@QuinnWilton yeah, seeing your responses, I think we can rip this out easily and go with String.
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.
Cool, I'll do that, thanks!
@QuinnWilton Code is looking pretty good, but there was a new setting added in |
This makes it easier to reuse the flag for other operations
This resolves an issue with loading keys from PEM files generated using some implementations of openssl, like libressl, which concatenate parameters onto the raw key material, and preventing libp2p from recognizing the serialization as containing a valid encoding of the key.
50a2543
to
5543e86
Compare
Hey @QuinnWilton! Trying out the new PEM key file features. They work great, and I love them. ❤️ I have a couple of minor DX requests:
|
Just pushed up these two changes; thanks! |
@QuinnWilton last minor thing:
|
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.
@QuinnWilton left a few last small changes/details. Great work.
392c517
to
9c8b07a
Compare
Description
This adds the start of an
init
command for configuring Homestar from the CLI. When run interactively, it currently provides a wizard for configuring the keys used by homestar, however it can also be run non-interactively using values configured through CLI flags.It also includes allowances for outputting a generated configuration over stdout, and the start of some patterns for handling some common CLI knobs, like
--no-input
,--quiet
, and--force
, though these are only likely to see more use as we expose more configuration options to this interactive wizard.There's still a lot of polish that can be done around how we handle error reporting and interrupts, like control+c, during interactive usage, but this is a start that should make it a lot easier to begin expanding the configuration options we expose to the user.
Link to issue
Please add a link to any relevant issues/tickets.
Screenshots/Screencaps