Add support for chain spec modifier commands #909
Conversation
|
User @Fahrrader, please sign the CLA here. |
|
Tests (and examples and docs, probably) are required, but putting the draft out there for review :) |
| for (const key of Object.keys(processes)) { | ||
| processes[key].kill(); | ||
| } |
There was a problem hiding this comment.
Missing prettier formatting.
| commandArgs: string[], | ||
| ) { | ||
| const chainSpecSubstitutePattern = new RegExp( | ||
| RAW_CHAIN_SPEC_IN_CMD_PATTERN.source + "|" + PLAIN_CHAIN_SPEC_IN_CMD_PATTERN.source, |
There was a problem hiding this comment.
| RAW_CHAIN_SPEC_IN_CMD_PATTERN.source + "|" + PLAIN_CHAIN_SPEC_IN_CMD_PATTERN.source, | |
| RAW_CHAIN_SPEC_IN_CMD_PATTERN?.source + "|" + PLAIN_CHAIN_SPEC_IN_CMD_PATTERN?.source, |
There was a problem hiding this comment.
This is oks since RAW_CHAIN_SPEC_IN_CMD_PATTERN and PLAIN_CHAIN_SPEC_IN_CMD_PATTERN are defined as constants. (in constanst.ts)
|
@Fahrrader , also, please run |
There was a problem hiding this comment.
Hi @Fahrrader, thanks for your contribution. Add this kind of feature is awesome! I think we should have a more concrete docs about how to use it (and also some example). We should give some explanation about how this works with chainql, or any other process that can mutate the spec.
Thanks again for your contribution!
Fahrrader
force-pushed
the
feat/chain-spec-mutator
branch
from
44a7513
to
9f51226
Compare
|
Docs, examples and tests (hopefully) are up and ready! |
| for (const cmd of networkSpec.relaychain.chainSpecModifierCommands) { | ||
| await runCommandWithChainSpec(specPath, cmd, networkSpec.configBasePath); | ||
| } | ||
| } catch (err) { |
There was a problem hiding this comment.
Regarding the last commit, I believe we shouldn't let the process go on if a step in chain spec customization has failed. By removing the try-catch structure from customizePlainRelayChain, the error would go into the spawn's try-catch structure where it would print it out, dump the logs and exit the process. This was the way it was done prior to displacement of the relay's plain chain spec customization to chainSpec.ts, too.
|
@pepoviola @wirednkod I think this PR is good to go and ready for the final review! I believe all the previous concerns were addressed. Please take another look, if not already. |
|
Hi @Fahrrader, sorry about the delay. First let me say thanks for contribution and your feedback. We had discussed with the team and some security concerns arise from allow users to execute arbitrary 3rd party tools as part of the spawning process. We have a couple of options, we can built-in some kind of customization tool that allow us to have more control of what is executed, we can allow this execution (giving us more flexibility) and add some guards (like don't allow to run with root access) or we can also make the spawning process more flexible and add a command to just produce the I will a new discussion in the repo to collect some feedback before go into one of the paths. Agains, sorry about the delay and thanks for your contribution and feedback!! |
|
This pull request has been mentioned on Polkadot Forum. There might be relevant details there: |
Fahrrader
force-pushed
the
feat/chain-spec-mutator
branch
from
c7bf197
to
1be03dd
Compare
In response to #892.