This repository was archived by the owner on Nov 15, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
CLI improvements & fixes #4812
Merged
Merged
CLI improvements & fixes #4812
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
It looks like @cecton signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
cecton
commented
Feb 3, 2020
cecton
commented
Feb 3, 2020
bkchr
reviewed
Feb 3, 2020
b987f86 to
82ebab0
Compare
e7a7aa9 to
c447ef0
Compare
304e6a7 to
725cf06
Compare
e5df461 to
eec9c0e
Compare
bkchr
suggested changes
Feb 5, 2020
Co-Authored-By: Bastian Köcher <[email protected]>
bkchr
reviewed
Feb 6, 2020
Co-Authored-By: Bastian Köcher <[email protected]>
bkchr
approved these changes
Feb 6, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are a few changes I missed during the refactoring.
Initialization issue and boilerplate
Most importantly: part of the
Configurationinitialization was done insc_cli::init. This means the user can not benefit from this initialization boilerplate if they have multipleConfigurationsincesc_cli::initcan only be called once.Boilerplate for
VersionInfoandConfigurationI'm also answering to the critic of @bkchr on the initialization using version: https://github.com/paritytech/substrate/pull/4692/files/bea809d4c14a2ede953227ac885e3b3f9771c548#r372047238 This will allow initializing a
Configurationand provide the version by default.Loading the
chain_specexplicitlyIn the past it was done automatically but in some cases we want to delay this. I moved the code to
Configuration.load_spec()so it can be called later on.chain_speccan also be written directly to theConfigurationwithout using thisload_spechelper.Makeparse_addresspublicThis change is necessary to allow the user to change the default addresses ofrpc_http,rpc_wsandgrafana_port.[edit: I don't need that!]
Fixing issue that prevents the user to override the port
In the refactoring I introduced a bug by mistake that could potentially prevent the CLI user to override the ports if defaults where provided for these ports (only on cumulus).
Change task_executor from Box to Arc
This is useful for cumulus where we have 2 nodes with 2 separate Configuration that need to spawn tasks to the same runtime.
Renamed TasksExecutorRequired to TaskExecutor
For consistency.
This is related to paritytech/cumulus#24
This is the continuation (and hopefully the end of) #4692