Add tests & Service's Configuration has optional fields that shouldn't be optional #4842

cecton · 2020-02-06T12:33:01Z

Related to #4776
Related to paritytech/polkadot#832

To summarize the changes:

I did not manage to validate with types the service's Configuration. But I did reduce the possibility of errors by moving all the "fill" functions to their respective structopts
I split params.rs to multiple modules: one module params for just CLI parameters and one module commands for CLI subcommands (and RunCmd). Every command and params are in their own file so things are grouped better together and easier to remove
I removed the run and run_subcommand helpers as they are not helping much anymore. Running a command is always a set of 3 commands: 1. init 2. update config 3. run. This still allow the user to change the config before arguments get parsed or right after.
I added tests for all subcommands.
~~I updated some feature flags with the new benchmark subcommands because client/cli couldn't build anymore. So I added a flag rocksdb~~ [edit: it has been fixed in master and this is no longer necessary so I removed it]

Overall the aim is to improve the situation with the Configuration and the optional parameters, add tests, make the API more consistent and simpler.

parity-cla-bot · 2020-02-06T12:33:07Z

It looks like @cecton signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Forked at: 099cd0f Parent branch: origin/master

Related to #4776

…/master' Commit: 4c34b64 Parent branch: origin/master Forked at: 099cd0f

…/master' Commit: 09abd3b Parent branch: origin/master Forked at: 099cd0f

Forked at: 099cd0f Parent branch: origin/master

…/master' Commit: 19b67cd Parent branch: origin/master Forked at: 099cd0f

…/master' Commit: 8b6b093 Parent branch: origin/master Forked at: 099cd0f

…/master' Commit: 26a4b73 Parent branch: origin/master Forked at: 099cd0f

Forked at: 099cd0f Parent branch: origin/master

gnunicorn

Assuming the params and commands didn't change other than being split up, because I only gave them a rough glance ...

Thanks for all the tests!

bin/node/cli/tests/build_spec_works.rs

client/cli/src/arg_enums.rs

client/cli/src/commands/runcmd.rs

(might not work)

Co-Authored-By: Benjamin Kampmann <[email protected]>

utils/frame/benchmarking-cli/src/lib.rs

client/service/src/config.rs

bkchr · 2020-02-21T10:07:06Z

client/service/src/benchmark.rs

@@ -0,0 +1,69 @@
+#![cfg(feature = "rocksdb")]


This whole benchmark command was moved to its own crate utils/frame/benchmarking-cli

client/cli/src/params/transaction_pool_params.rs

client/cli/src/params/mod.rs

client/cli/src/node_key.rs

bkchr

See others.

cecton · 2020-02-21T10:49:43Z

Assuming the params and commands didn't change other than being split up, because I only gave them a rough glance ...

Yes well it was complicated because a lot of changes have been pushed to master, especially yesterday.

TL;DR: I'm very confident nothing has been lost.

Long story: I'm experimenting a new workflow where I merge the most non-conflicting commits to my branch to update it and then I create a single merge commit with the conflicting commit where I solve the issue independently.

Because of that, I look at the commit carefully and I replicate the changes in my files properly. It's a bit like what happen with rebase (resolving conflicts commit by commit) except that it's merging instead of rebasing.

Thanks for all the tests!

You're welcome! 🤗 I have hope to improve a lot sc_cli to a point where it would be very stable and intuitive.

Co-Authored-By: Bastian Köcher <[email protected]>

bkchr · 2020-02-21T10:49:04Z

bin/node-template/node/src/command.rs

-			&version,
-		)
+		Some(subcommand) => {
+			subcommand.init(&version)?;


Why aren't init and update_config not done inside of run()? Someone could just call run() without calling the other methods before and it would be incorrect.

Because for cumulus we want to be able to modify the config before running the node.

We could have another "run" command that would do all 3 commands but we need to keep the existing 3.

Since a helper would simply call init & update_config & run, I thought it wouldn't be super helpful and I have not added it.

I removed the run and run_subcommand helpers as they are not helping much anymore. Running a command is always a set of 3 commands: 1. init 2. update config 3. run. This still allow the user to change the config before arguments get parsed or right after.

😱 you didn't read the PR description

Code is law :P

Yeah it is okay for now. However it could happen that the user just calls run() (because nothing prevents them) and it will break.

And this "pattern" is a let's repeat some code "pattern" :D

bin/node/cli/src/command.rs

bin/node/cli/tests/build_spec_works.rs

bin/node/cli/tests/common.rs

Co-Authored-By: Bastian Köcher <[email protected]>

bkchr · 2020-02-21T12:26:03Z

bin/node-template/node/src/command.rs

-			&version,
-		)
+		Some(subcommand) => {
+			subcommand.init(&version)?;


Code is law :P

Yeah it is okay for now. However it could happen that the user just calls run() (because nothing prevents them) and it will break.

bkchr · 2020-02-21T12:28:44Z

bin/node-template/node/src/command.rs

-			&version,
-		)
+		Some(subcommand) => {
+			subcommand.init(&version)?;


And this "pattern" is a let's repeat some code "pattern" :D

bkchr · 2020-02-21T12:29:16Z

bin/node/cli/tests/common.rs

@@ -14,7 +14,14 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.

-use std::{process::{Child, ExitStatus}, thread, time::Duration};
+#![cfg(unix)]
+#![allow(dead_code)]


What is dead inside here? :P

I don't know. I think common.rs is executed on its own...

So everything looks like dead code when it's compiled

…t be optional (paritytech#4842) Related to paritytech#4776 Related to paritytech/polkadot#832 To summarize the changes: 1. I did not manage to validate with types the service's Configuration. But I did reduce the possibility of errors by moving all the "fill" functions to their respective structopts 2. I split params.rs to multiple modules: one module params for just CLI parameters and one module commands for CLI subcommands (and RunCmd). Every command and params are in their own file so things are grouped better together and easier to remove 3. I removed the run and run_subcommand helpers as they are not helping much anymore. Running a command is always a set of 3 commands: 1. init 2. update config 3. run. This still allow the user to change the config before arguments get parsed or right after. 4. I added tests for all subcommands. 5. [deleted] Overall the aim is to improve the situation with the Configuration and the optional parameters, add tests, make the API more consistent and simpler.

cecton changed the title ~~Fixing: Service's Configuration has optional fields that shouldn't be optional~~ WIP: Fixing: Service's Configuration has optional fields that shouldn't be optional Feb 6, 2020

cecton added 2 commits February 6, 2020 15:52

Initial commit

ff20e5e

Forked at: 099cd0f Parent branch: origin/master

Moving out pub functions from sc_cli

d0fc553

Related to #4776

cecton force-pushed the cecton-cli-service-configurations branch from 5ab624e to d0fc553 Compare February 6, 2020 14:53

cecton added 8 commits February 7, 2020 15:58

Update branch 'cecton-cli-service-configurations' from parent 'origin…

6683ebe

…/master' Commit: 4c34b64 Parent branch: origin/master Forked at: 099cd0f

Update branch 'cecton-cli-service-configurations' from parent 'origin…

1b6a9e8

…/master' Commit: 09abd3b Parent branch: origin/master Forked at: 099cd0f

WIP

057d0fc

Forked at: 099cd0f Parent branch: origin/master

WIP

b1dc8ae

Forked at: 099cd0f Parent branch: origin/master

WIP

de4bd11

Forked at: 099cd0f Parent branch: origin/master

WIP

2b70a8a

Forked at: 099cd0f Parent branch: origin/master

WIP

562c8be

Forked at: 099cd0f Parent branch: origin/master

WIP

8218473

Forked at: 099cd0f Parent branch: origin/master

cecton mentioned this pull request Feb 10, 2020

Test node exits properly paritytech/polkadot#831

Merged

cecton added 16 commits February 10, 2020 10:33

WIP

025b88f

Forked at: 099cd0f Parent branch: origin/master

WIP

f4290e2

Forked at: 099cd0f Parent branch: origin/master

WIP

f84cbc5

Forked at: 099cd0f Parent branch: origin/master

WIP

58521d1

Forked at: 099cd0f Parent branch: origin/master

Update branch 'cecton-cli-service-configurations' from parent 'origin…

8b618ac

…/master' Commit: 19b67cd Parent branch: origin/master Forked at: 099cd0f

Update branch 'cecton-cli-service-configurations' from parent 'origin…

76185d4

…/master' Commit: 8b6b093 Parent branch: origin/master Forked at: 099cd0f

Update branch 'cecton-cli-service-configurations' from parent 'origin…

ce4bcbf

…/master' Commit: 26a4b73 Parent branch: origin/master Forked at: 099cd0f

WIP

c0d27a5

Forked at: 099cd0f Parent branch: origin/master

WIP

0a6b34f

Forked at: 099cd0f Parent branch: origin/master

WIP

7ccbf6d

Forked at: 099cd0f Parent branch: origin/master

WIP

c6acedd

Forked at: 099cd0f Parent branch: origin/master

WIP

d488431

Forked at: 099cd0f Parent branch: origin/master

WIP

7a42b48

Forked at: 099cd0f Parent branch: origin/master

WIP

0b46330

Forked at: 099cd0f Parent branch: origin/master

WIP

faaf28d

Forked at: 099cd0f Parent branch: origin/master

WIP

0c0d979

Forked at: 099cd0f Parent branch: origin/master

cecton mentioned this pull request Feb 21, 2020

Service's Configuration has optional fields that shouldn't be optional #4776

Closed

gnunicorn approved these changes Feb 21, 2020

View reviewed changes

bin/node/cli/tests/build_spec_works.rs Outdated Show resolved Hide resolved

client/cli/src/arg_enums.rs Show resolved Hide resolved

client/cli/src/commands/runcmd.rs Outdated Show resolved Hide resolved

cecton and others added 2 commits February 21, 2020 11:33

Include test on Windows

1c48788

(might not work)

Update client/cli/src/commands/runcmd.rs

be53825

Co-Authored-By: Benjamin Kampmann <[email protected]>

bkchr reviewed Feb 21, 2020

View reviewed changes

bkchr suggested changes Feb 21, 2020

View reviewed changes

cecton and others added 10 commits February 21, 2020 11:50

Update utils/frame/benchmarking-cli/src/lib.rs

8841dfb

Co-Authored-By: Bastian Köcher <[email protected]>

Update utils/frame/benchmarking-cli/src/lib.rs

c61f433

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/service/src/config.rs

399a7ca

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/service/src/config.rs

7734c2b

Co-Authored-By: Bastian Köcher <[email protected]>

Removed extra module

861a854

Update client/cli/src/params/transaction_pool_params.rs

1e54938

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/cli/src/params/transaction_pool_params.rs

8a289e2

Co-Authored-By: Bastian Köcher <[email protected]>

Update client/cli/src/params/mod.rs

5780c90

Co-Authored-By: Bastian Köcher <[email protected]>

Removed another extra module

eb49376

Update client/cli/src/params/mod.rs

2173d22

Co-Authored-By: Bastian Köcher <[email protected]>

bkchr reviewed Feb 21, 2020

View reviewed changes

cecton and others added 4 commits February 21, 2020 12:08

Added comment and removed unused import

c57fd85

Update bin/node/cli/src/command.rs

df0522f

Co-Authored-By: Bastian Köcher <[email protected]>

Update bin/node/cli/tests/build_spec_works.rs

8e3118a

Co-Authored-By: Bastian Köcher <[email protected]>

Fixed missing import

f11a6d5

bkchr approved these changes Feb 21, 2020

View reviewed changes

cecton merged commit 97ad802 into master Feb 21, 2020

cecton deleted the cecton-cli-service-configurations branch February 21, 2020 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests & Service's Configuration has optional fields that shouldn't be optional #4842

Add tests & Service's Configuration has optional fields that shouldn't be optional #4842

cecton commented Feb 6, 2020 •

edited

Loading

parity-cla-bot commented Feb 6, 2020

gnunicorn left a comment

bkchr Feb 21, 2020

bkchr left a comment

cecton commented Feb 21, 2020

bkchr Feb 21, 2020

cecton Feb 21, 2020

cecton Feb 21, 2020

bkchr Feb 21, 2020

bkchr Feb 21, 2020

bkchr Feb 21, 2020

bkchr Feb 21, 2020

bkchr Feb 21, 2020

cecton Feb 21, 2020

cecton Feb 21, 2020

Add tests & Service's Configuration has optional fields that shouldn't be optional #4842

Add tests & Service's Configuration has optional fields that shouldn't be optional #4842

Conversation

cecton commented Feb 6, 2020 • edited Loading

parity-cla-bot commented Feb 6, 2020

gnunicorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

cecton commented Feb 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Feb 6, 2020 •

edited

Loading