NC-1737 Enabled warning on CLI dependent options #679
Conversation
…options This mechanism is aimed at preventing the use of options if the associated service is disabled. This is not a generic system as our need is simple here. But in future version of PicoCLI some options dependency mechanism may be implemented that could replace this. See remkop/picocli#295 Our current mechanism apply to p2p, rpc (http+ws), mining, privacy and metrics. Also renamed inconsistent isRpcHttpEnabled option variable and moved --privacy-enabled option to be on top of other privacy options. Related tests where added or updated and renamed.
NicolasMassart
changed the title
There was a problem hiding this comment.
Codewise this is fine, however, I don't think we should perform this validation. It's incredibly useful to be able to temporarily disable things like p2p networking without also having to delete multiple other options which could have been easily ignored.
For mining this is particularly true - it's very common to want to configure the coinable and extra data on the command line but then only enable mining at specific times via the JSON-RPC method. With this change that's not possible and you'd have to configure it all via JSON-RPC. I'm a little less concerned about the other options but we definitely shouldn't break this mining use case.
|
How does this interact with TOML files, can I leave in all the useless config I feel like there? would this just validate CLI? |
|
As this is just enforcing the relationship between options and not changing how options work and given the fact that there's no consensus on the need of this change, I propose that we don't include it in the 0.9 release. |
|
If I'm thinking about this correctly, the goal of this change is to make it clear to the user what options are dependent on each other. The end goal is to avoid people assuming something is "on" because they've configured it. We could accomplish this by allowing them to configure the options but show a "warning" that they will have no impact until the feature is enabled. So, essentially change this from an "error" to a "warning". Does that solve our problem by communicating the dependency but still allowing things to be temporarily disabled/enabled in situations like @ajsutton called out? |
|
I like the compromise that the CLI output will be noisy when unusable CLI flags are used. "I set the --rpc-http=port to 8545, why won't it work? Oh --rpc-http-enable!" |
|
Yep, logging a warning makes a lot of sense to me. |
injects a Logger in PantheonCommand constructor to log instead of throwing an error and also provides injection capabilities for a mock in tests. adds unit tests updates tests logger name to TEST_LOGGER to prevent confusion with the mock
# Conflicts: # pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java
…ng-peers remove --max-trailing-peers after merge with master where it was deleted
…o NC-1737_api_enabled_warning # Conflicts: # pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java
pantheon/src/main/java/tech/pegasys/pantheon/cli/CommandLineUtils.java
Outdated
Show resolved
Hide resolved
…ils.java Change wording Co-Authored-By: NicolasMassart <NicolasMassart@users.noreply.github.com>
…nd.java updated rules for metrics Co-Authored-By: NicolasMassart <NicolasMassart@users.noreply.github.com>
pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java
Outdated
Show resolved
Hide resolved
PR description
This mechanism is aimed at preventing the use of options if the associated
service is disabled. This is not a generic system as our need is simple here.
But in future version of PicoCLI some options dependency mechanism may be
implemented that could replace this.
See remkop/picocli#295
Our current mechanism apply to p2p, rpc (http+ws), mining, privacy and metrics.
Also renamed inconsistent isRpcHttpEnabled option variable and moved
--privacy-enabled option to be on top of other privacy options.
Related tests where added or updated and renamed.
Fixed Issue(s)
fixes NC-1737