Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

NC-2120 --discovery-enabled option refactoring #661

Merged

Conversation

NicolasMassart
Copy link
Contributor

PR description

now able to have --discovery-enabled option with true as default
and --discovery-enabled=true or --discovery-enabled=false on CLI
and discovery-enabled=true or discovery-enabled=false in YAML config file.

Fixed Issue(s)

fixes NC-2120

now able to have --discovery-enabled option with true as default
and --discovery-enabled=true or --discovery-enabled=false on CLI
and discovery-enabled=true or discovery-enabled=false in YAML config file.
@NicolasMassart NicolasMassart added documentation Related to any type of documentation api Related to public APIs labels Jan 25, 2019
@NicolasMassart NicolasMassart self-assigned this Jan 25, 2019
Copy link
Contributor

@MadelineMurray MadelineMurray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of minor comments

@@ -36,7 +36,9 @@ Trailing peers cannot be used to get new blocks and are more likely to be reques

## No Discovery

The [`--no-discovery`](../Reference/Pantheon-CLI-Syntax.md#no-discovery) command line option disables P2P peer discovery. Only use this option if you are running a test node or a test network with fixed nodes.
The [`--rpc-ws-enabled`](../Reference/Pantheon-CLI-Syntax.md#rpc-ws-enabled) command line option
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should rpc-ws-enabled be discover-enabled?

The [`--no-discovery`](../Reference/Pantheon-CLI-Syntax.md#no-discovery) command line option disables P2P peer discovery. Only use this option if you are running a test node or a test network with fixed nodes.
The [`--rpc-ws-enabled`](../Reference/Pantheon-CLI-Syntax.md#rpc-ws-enabled) command line option
enables P2P peer discovery.
Only set this option to `false` if you are running a test node or a test network with fixed nodes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Only set this option to' -> 'Set to' -> uses less words and still has the same meaning


```bash tab="Syntax"
--no-discovery
--discovery-enabled=false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For p2p-enabled I specified the syntax as --p2p-enabled=<true|false> because you can specify true. I think either way is fine but we should make them the same so just let me know what you end up doing.

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@NicolasMassart NicolasMassart merged commit fa901d5 into PegaSysEng:master Jan 25, 2019
rain-on pushed a commit to rain-on/pantheon that referenced this pull request Jan 29, 2019
fixes NC-2120 --discovery-enabled option refactoring

now able to have --discovery-enabled option with true as default
and --discovery-enabled=true or --discovery-enabled=false on CLI
and discovery-enabled=true or discovery-enabled=false in YAML config file.

updates doc
vinistevam pushed a commit to vinistevam/pantheon that referenced this pull request Jan 29, 2019
fixes NC-2120 --discovery-enabled option refactoring

now able to have --discovery-enabled option with true as default
and --discovery-enabled=true or --discovery-enabled=false on CLI
and discovery-enabled=true or discovery-enabled=false in YAML config file.

updates doc
@NicolasMassart NicolasMassart deleted the NC-2120_discovery_enabled branch January 30, 2019 16:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api Related to public APIs documentation Related to any type of documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants