feat: automated node config path args #85
Conversation
The user previously had to specify the location of our generated config file. This change removes this requirement -- since we generate it, we should also pass on the information to the node.
src/setup/node.rs
Outdated
| }) | ||
| }; | ||
|
|
||
| // insert the config file cmd args |
There was a problem hiding this comment.
I'm not too happy with the location of this code -- but my brain is too fried to think of a better place.
Happy to get suggestions.
There was a problem hiding this comment.
I'd probably make the change in NodeMetaData::new since that's where it's constructed from the args.
There was a problem hiding this comment.
We need access to Node::config_filepath() for the actual location of the generated file. We could move that down a layer as well?
src/setup/node.rs
Outdated
| }) | ||
| }; | ||
|
|
||
| // insert the config file cmd args |
There was a problem hiding this comment.
I'd probably make the change in NodeMetaData::new since that's where it's constructed from the args.
src/setup/node.rs
Outdated
| let start_arg = node | ||
| .meta | ||
| .start_args | ||
| .pop() |
There was a problem hiding this comment.
You could probably make an insert at i = len - 2 instead?
There was a problem hiding this comment.
I was worried that having it twice in a row might be confusing (since its two separate args). But I think yours is still better.
src/setup/node.rs
Outdated
| node.meta.start_args.push("--config".into()); | ||
| node.meta | ||
| .start_args | ||
| .push(node.config_filepath().into_os_string()); |
There was a problem hiding this comment.
These two previous steps could probably be condensed (perhaps with format! and OsString::from)?
There was a problem hiding this comment.
For some reason combining the two into one, results in extra quotes around just that pair of args which breaks the command.
So instead of
`arg1 arg2 arg3 arg4 start`
we get
`arg1 arg2 'arg3 arg4' start`
Mirko-von-Leipzig
force-pushed
the
feat/improve_config
branch
from
37b7e8e to
07014e0
Compare
Mirko-von-Leipzig
force-pushed
the
feat/improve_config
branch
from
07014e0 to
67fd234
Compare
src/setup/config.rs
Outdated
| NodeKind::Zebra => { | ||
| // Zebra's final arg must be `start`, so we insert the actual args before it. | ||
| let n = start_args.len(); | ||
| assert!(n > 1, "Expected at least one arg for Zebra (`start`)"); |
There was a problem hiding this comment.
We might actually want to return a result here instead of panicking for future proofing? (A custom runner would likely need Results to function properly).
feat: automated node config path args
The user previously had to specify the location of our generated config file. This change removes this requirement -- since we generate it, we should also pass on the information to the node.