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

Fix RunnerBuilder method names #1284

Merged
merged 7 commits into from
Apr 17, 2019
Merged

Fix RunnerBuilder method names #1284

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8126a80
Fix RunnerBuilder method names
mbaxter Apr 15, 2019
f552ef9
Merge branch 'master' into minor/fix-runner-builder-method-names
mbaxter Apr 16, 2019
cd78395
Use more specific names for p2p config in RunnerBuilder
mbaxter Apr 16, 2019
ee6e3b4
Fix inaccurate references to discovery in PantheonCommand
mbaxter Apr 16, 2019
7170c01
Merge branch 'master' into minor/fix-runner-builder-method-names
mbaxter Apr 16, 2019
bd17897
Tweak language for p2p options
mbaxter Apr 16, 2019
383d65d
Merge branch 'master' into minor/fix-runner-builder-method-names
mbaxter Apr 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...c/test/java/tech/pegasys/pantheon/tests/acceptance/dsl/node/ThreadPantheonNodeRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ public void startNode(final PantheonNode node) {
.pantheonController(pantheonController)
.ethNetworkConfig(ethNetworkConfig)
.discovery(node.isDiscoveryEnabled())
.discoveryHost(node.hostName())
.discoveryPort(0)
.advertisedHost(node.hostName())
.listenPort(0)
.maxPeers(25)
.jsonRpcConfiguration(node.jsonRpcConfiguration())
.webSocketConfiguration(node.webSocketConfiguration())
Expand Down
12 changes: 6 additions & 6 deletions pantheon/src/main/java/tech/pegasys/pantheon/RunnerBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public class RunnerBuilder {
private boolean p2pEnabled = true;
private boolean discovery;
private EthNetworkConfig ethNetworkConfig;
private String discoveryHost;
private String advertisedHost;
private int listenPort;
private int maxPeers;
private JsonRpcConfiguration jsonRpcConfiguration;
Expand All @@ -107,7 +107,7 @@ private EnodeURL getSelfEnode() {
BytesValue nodeId = pantheonController.getLocalNodeKeyPair().getPublicKey().getEncodedBytes();
return EnodeURL.builder()
.nodeId(nodeId)
.ipAddress(discoveryHost)
.ipAddress(advertisedHost)
.listeningPort(listenPort)
.build();
}
Expand Down Expand Up @@ -137,12 +137,12 @@ public RunnerBuilder ethNetworkConfig(final EthNetworkConfig ethNetworkConfig) {
return this;
}

public RunnerBuilder discoveryHost(final String discoveryHost) {
this.discoveryHost = discoveryHost;
public RunnerBuilder advertisedHost(final String advertisedHost) {
this.advertisedHost = advertisedHost;
return this;
}

public RunnerBuilder discoveryPort(final int listenPort) {
public RunnerBuilder listenPort(final int listenPort) {
Copy link
Contributor

Choose a reason for hiding this comment

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

listenPort feels too generic here. I wonder if these should be p2pAdvertisedHost and p2pListenPort? A bigger refactoring would be to make an actual P2pConfiguration object to match JsonRpcConfiguration and friends but that's probably scope creep on what you were working on. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the names. We do have a NetworkingConfiguration object, although it's probably due for some cleanup.

this.listenPort = listenPort;
return this;
}
Expand Down Expand Up @@ -208,7 +208,7 @@ public Runner build() {
discoveryConfiguration =
DiscoveryConfiguration.create()
.setBindPort(listenPort)
.setAdvertisedHost(discoveryHost)
.setAdvertisedHost(advertisedHost)
.setBootstrapPeers(bootstrap);
} else {
discoveryConfiguration = DiscoveryConfiguration.create().setActive(false);
Expand Down
4 changes: 2 additions & 2 deletions pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -976,8 +976,8 @@ private void synchronize(
.p2pEnabled(p2pEnabled)
.discovery(peerDiscoveryEnabled)
.ethNetworkConfig(ethNetworkConfig)
.discoveryHost(discoveryHost)
.discoveryPort(discoveryPort)
.advertisedHost(discoveryHost)
.listenPort(discoveryPort)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it weird to have the names mismatching here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, thats weird :) . Updated the variable names. Also updated the description of the corresponding options.

.maxPeers(maxPeers)
.jsonRpcConfiguration(jsonRpcConfiguration)
.webSocketConfiguration(webSocketConfiguration)
Expand Down
4 changes: 2 additions & 2 deletions pantheon/src/test/java/tech/pegasys/pantheon/RunnerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ private void syncFromGenesis(final SyncMode mode) throws Exception {
new RunnerBuilder()
.vertx(Vertx.vertx())
.discovery(true)
.discoveryHost(listenHost)
.discoveryPort(0)
.advertisedHost(listenHost)
.listenPort(0)
.maxPeers(3)
.metricsSystem(noOpMetricsSystem)
.bannedNodeIds(emptySet())
Expand Down
4 changes: 2 additions & 2 deletions pantheon/src/test/java/tech/pegasys/pantheon/cli/CommandTestAbstract.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ public void initMocks() throws Exception {
when(mockRunnerBuilder.pantheonController(any())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.discovery(anyBoolean())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.ethNetworkConfig(any())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.discoveryHost(anyString())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.discoveryPort(anyInt())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.advertisedHost(anyString())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.listenPort(anyInt())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.maxPeers(anyInt())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.p2pEnabled(anyBoolean())).thenReturn(mockRunnerBuilder);
when(mockRunnerBuilder.jsonRpcConfiguration(any())).thenReturn(mockRunnerBuilder);
Expand Down
20 changes: 10 additions & 10 deletions pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ public void callingPantheonCommandWithoutOptionsMustSyncWithDefaultValues() thro
EthNetworkConfig.jsonConfig(MAINNET),
EthNetworkConfig.MAINNET_NETWORK_ID,
MAINNET_BOOTSTRAP_NODES));
verify(mockRunnerBuilder).discoveryHost(eq("127.0.0.1"));
verify(mockRunnerBuilder).discoveryPort(eq(30303));
verify(mockRunnerBuilder).advertisedHost(eq("127.0.0.1"));
verify(mockRunnerBuilder).listenPort(eq(30303));
verify(mockRunnerBuilder).maxPeers(eq(25));
verify(mockRunnerBuilder).jsonRpcConfiguration(eq(defaultJsonRpcConfiguration));
verify(mockRunnerBuilder).webSocketConfiguration(eq(defaultWebSocketConfiguration));
Expand Down Expand Up @@ -281,8 +281,8 @@ public void overrideDefaultValuesIfKeyIsPresentInConfigFile() throws IOException

verify(mockRunnerBuilder).discovery(eq(false));
verify(mockRunnerBuilder).ethNetworkConfig(ethNetworkConfigArgumentCaptor.capture());
verify(mockRunnerBuilder).discoveryHost(eq("1.2.3.4"));
verify(mockRunnerBuilder).discoveryPort(eq(1234));
verify(mockRunnerBuilder).advertisedHost(eq("1.2.3.4"));
verify(mockRunnerBuilder).listenPort(eq(1234));
verify(mockRunnerBuilder).maxPeers(eq(42));
verify(mockRunnerBuilder).jsonRpcConfiguration(eq(jsonRpcConfiguration));
verify(mockRunnerBuilder).webSocketConfiguration(eq(webSocketConfiguration));
Expand Down Expand Up @@ -599,8 +599,8 @@ public void noOverrideDefaultValuesIfKeyIsNotPresentInConfigFile() throws IOExce
EthNetworkConfig.jsonConfig(MAINNET),
EthNetworkConfig.MAINNET_NETWORK_ID,
MAINNET_BOOTSTRAP_NODES));
verify(mockRunnerBuilder).discoveryHost(eq("127.0.0.1"));
verify(mockRunnerBuilder).discoveryPort(eq(30303));
verify(mockRunnerBuilder).advertisedHost(eq("127.0.0.1"));
verify(mockRunnerBuilder).listenPort(eq(30303));
verify(mockRunnerBuilder).maxPeers(eq(25));
verify(mockRunnerBuilder).jsonRpcConfiguration(eq(jsonRpcConfiguration));
verify(mockRunnerBuilder).webSocketConfiguration(eq(webSocketConfiguration));
Expand Down Expand Up @@ -984,8 +984,8 @@ public void p2pHostAndPortOptionMustBeUsed() {
final int port = 1234;
parseCommand("--p2p-host", host, "--p2p-port", String.valueOf(port));

verify(mockRunnerBuilder).discoveryHost(stringArgumentCaptor.capture());
verify(mockRunnerBuilder).discoveryPort(intArgumentCaptor.capture());
verify(mockRunnerBuilder).advertisedHost(stringArgumentCaptor.capture());
verify(mockRunnerBuilder).listenPort(intArgumentCaptor.capture());
verify(mockRunnerBuilder).build();

assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
Expand All @@ -1001,7 +1001,7 @@ public void p2pHostMayBeLocalhost() {
final String host = "localhost";
parseCommand("--p2p-host", host);

verify(mockRunnerBuilder).discoveryHost(stringArgumentCaptor.capture());
verify(mockRunnerBuilder).advertisedHost(stringArgumentCaptor.capture());
verify(mockRunnerBuilder).build();

assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
Expand All @@ -1016,7 +1016,7 @@ public void p2pHostMayBeIPv6() {
final String host = "2600:DB8::8545";
parseCommand("--p2p-host", host);

verify(mockRunnerBuilder).discoveryHost(stringArgumentCaptor.capture());
verify(mockRunnerBuilder).advertisedHost(stringArgumentCaptor.capture());
verify(mockRunnerBuilder).build();

assertThat(stringArgumentCaptor.getValue()).isEqualTo(host);
Expand Down