Getting configuration from commands#4643
Conversation
|
It looks like @cecton signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
tomusdrw
left a comment
tomusdrw
left a comment
There was a problem hiding this comment.
Looks ok, but I wonder if having a builder pattern here wouldn't make things simpler. Right now it seems we make it simpler by introducing yet another method (into_configuration). Do all other functions need to be public then?
All these create_config_with_* and fill_* functions are quite confusing to me. Without looking at the code I really have no idea which one should I use for my sub-command.
| { | ||
| let spec = load_spec(cli, spec_factory)?; | ||
| let base_path = base_path(cli, version); | ||
| let base_path = base_path(cli, version, default_base_path); |
There was a problem hiding this comment.
This could delegate to create_build_spec_config to avoid duplication
|
|
||
| impl<'a, CC, RP> ParseAndPrepare<'a, CC, RP> { | ||
| /// Convert ParseAndPrepare to Configuration | ||
| pub fn into_configuration<C, G, E, S>( |
There was a problem hiding this comment.
Should existing node/cli and node-template/cli be using that function?
I don't think
The 1. is normally used for the 1st node as it also sets up the logger of the app. While the 2. can be used for extra nodes.
I think no. Otherwise it would skip the logger setup (and the panic handler and some other things). |
tomusdrw
left a comment
tomusdrw
left a comment
There was a problem hiding this comment.
Looks good then, would be even better to figure out if we can unpublish the low-level functions (I guess it will require a corresponding PR for polkadot).
|
@tomusdrw I'm not sure if it's used in polkadot actually. I just grepped the code and I couldn't find anything. Are you sure? |
gavofyork
added
A8-looksgood
and removed
J0-enhancement
|
@cecton nope, just assumed it is. If it's not then even better :) |
* Expose a method that allows converting RunCmd to Configuration * WIP * WIP * WIP * WIP * WIP * WIP * WIP * WIP
It changes the way we extended the CLI functionalities of substrate to allow more flexibility. (If this was not clear, here is another version: it changes the `sc_cli` API to allow more flexibility).
This touches a few important things:
- the startup of the async task with tokei:
This was in node and node-template and I moved it to substrate. The idea is to have 1 time the code that handles unix signals (SIGTERM and SIGINT) properly. It is however possible to make this more generic to wait for a future instead and provide only a helper for the basic handling of SIGTERM and SIGINT.
- increased the version of structopt and tokei
- no more use of structopt internal's API
- less use of generics
Related to #4643 and paritytech/cumulus#42: the implementation of "into_configuration" and "get_config" are similar but with better flexibility so it is now possible in cumulus to have the command-line arguments only of the run command for polkadot if we want
Related to paritytech/cumulus#24 and paritytech/cumulus#34 : it will now be possible to make a configuration struct for polkadot with some overrides of the default parameters much more easily.
* Expose a method that allows converting RunCmd to Configuration * WIP * WIP * WIP * WIP * WIP * WIP * WIP * WIP
4 participants
This PR adds a public function
.into_configuration()onParseAndPreparethat generates aConfiguration.The use case is for cumulus and other projects that would want to parse multiple command line arguments separately in order to start multiple nodes.
Related paritytech/cumulus#34
Note: there is also a new extra argument
default_base_paththat allows the caller to define theconfig_dirbefore theConfigurationgets created. Thisdefault_base_pathcan be overwritten by the argument--base-path(as proved in the test I added).