Make broker flag configurable by tcharding · Pull Request #479 · mysteriumnetwork/node

tcharding · 2018-10-24T02:52:27Z

Currently broker flag is hard coded as 4222. We can make it configurable by adding an option --broker-port.

Make broker flag configurable.

Closes #226

Testing

PR is untested. With the following patch applied and running bin/localnet/setup.sh I can confirm port is open

sudo netstat -tlpn | grep 4333
tcp6       0      0 :::4333                 :::*                    LISTEN      20868/docker-pro

I cannot really work out ATM how to do further testing using the local net. I'm a bit confused by bin/run daemon vs bin/run service. (I've not got localnet running on master branch either.)

diff --git a/bin/docker/docker-entrypoint.sh b/bin/docker/docker-entrypoint.sh
index 8be87972e9d9..80d67c28fe5d 100755
--- a/bin/docker/docker-entrypoint.sh
+++ b/bin/docker/docker-entrypoint.sh
@@ -12,4 +12,5 @@ exec /usr/bin/myst \
  --data-dir=$OS_DIR_DATA \
  --runtime-dir=$OS_DIR_RUN \
  --tequilapi.address=0.0.0.0 \
+ --broker-port=4333 \
  $@
diff --git a/bin/localnet/docker-compose.yml b/bin/localnet/docker-compose.yml
index 38f3a6373e52..9c669f195e27 100644
--- a/bin/localnet/docker-compose.yml
+++ b/bin/localnet/docker-compose.yml
@@ -4,7 +4,7 @@ services:
   broker:
     image: nats
     expose:
-      - 4222
+      - 4333
       - 8222
 
   #infrastructure - centralized api and db
diff --git a/bin/localnet/publish-ports.yml b/bin/localnet/publish-ports.yml
index f2c4867be384..ef0a41a74a57 100644
--- a/bin/localnet/publish-ports.yml
+++ b/bin/localnet/publish-ports.yml
@@ -3,7 +3,7 @@ services:
 
   broker:
     ports:
-      - 4222:4222
+      - 4333:4333
       - 8222:4222

Signed-off-by: Tobin C. Harding me@tobin.cc

soffokl

Do we really need an extra flag for broker port?
Why broker address can't be a single line testnet-broker.mysterium.network:4222?

tcharding · 2018-10-24T04:50:15Z

Yes @soffokl that was also suggested by @zyfdegh in issue #226. @Waldz suggested a separate option. My guess is this is to fit in with

bin/run -h
...
   --tequilapi.address value        IP address of interface to listen for incoming connections (default: "127.0.0.1")
   --tequilapi.port value           Port for listening incoming api requests (default: 4050)
...

but discovery address uses this method

run/bin -h
...
   --discovery-address value        Address (URL form) of discovery service (default: "https://testnet-api.mysterium.network/v1")

So we have an inconsistency here. We should make it all uniform right? Either the port is accepted as part of the address for all addresses or for none. My vote, for what its worth, goes with having the port as part of the address 'host:port' as is convention within the net package.

tcharding · 2018-10-24T06:42:38Z

What do you rekon @Waldz, I'll re-spin after you've made a call.

Waldz · 2018-10-24T12:28:45Z

I am for broker-address=testnet-broker.mysterium.network:4222.

Because, service provider will have several contacts where he announces service proposal:

contact.type="broker"
contact.address=testnet-broker.mysterium.network:4222

contact.type="email"
contact.address=64123423563asds@gmail.com

tcharding · 2018-10-25T21:19:42Z

Hey @Waldz could you explain your work flow to me please. When you merge master into a pull request like done here what do you expect me to do when I go to update the PR?

Rebase and move the original patches to the top?
git reset hard and remove the merge then add new changes to the original patch?
Add patches on top (surely not :)
Other?

In this case it doesn't matter because the whole PR will change but this happened once before and confused me also.

thanks man

tcharding · 2018-10-26T04:28:08Z

Updated this PR completely:

First patch fixes lint issues in server/
Second patch fixes handling of --broker-address

Currently we do not handle command line option --broker-address correctly. Firstly we hard code port number 4222 and append it to the string. This means a user cannot configure NATS to use a different port. We should check first if the string contains a port number and only use the default if required. Secondly we should accept the protocol as part of the string. We should support

--broker-address=[nats://]host[:port]

Fix handling of --broker-address to accept optional protocol and port number. Keep defaults if not supplied.

soffokl · 2018-10-26T04:35:33Z

+
+	url, err := url.Parse(rawurl)
+	if err != nil {
+		return nil


We should not just hide an error.
I'd change the function to return the error, and stop the application if the provided address was incorrect.

I actually did that at first. Then I thought that perhaps it was the callers responsibility to pass in a valid string. Caller can still tell we failed by checking if returned string is "". This imitates the way at least one function in the standard library works (I can't think which one off the top of my head though). Doing it this way will mean we could verify the option strings when the myst command first starts. This makes the rest of the code more simple since we know we have valid option strings. What do you think?

Function naming. GenerateNewAddress ?

I'll agree with Dmitry - it's best to return the error here. Having to check for an empty string does not feel like a Go way of doing things. I'd stick with the error being returned and the good old if err != nil.

Looks like we have consensus on this then. Will re-do with an error return. Thanks

@tcharding replying to your question - yes I think GenerateNewAddress is a better name.

lol, my dislexia was playing up @tadovas I read your original comment a bunch of times before and didn't realize you had switched the word Generate from the back of the function name to the front. Thanks for clarifying.

tadovas · 2018-10-26T05:54:40Z

+
+	url, err := url.Parse(rawurl)
+	if err != nil {
+		return nil


Function naming. GenerateNewAddress ?

vkuznecovas · 2018-10-26T06:56:01Z

+
+	url, err := url.Parse(rawurl)
+	if err != nil {
+		return nil


I'll agree with Dmitry - it's best to return the error here. Having to check for an empty string does not feel like a Go way of doing things. I'd stick with the error being returned and the good old if err != nil.

zolia · 2018-10-26T07:14:08Z

--broker-address=[nats://]host[:port]

implies that you should use nats service. Idea for the future is possibly to have multiple types of brokers. Meaning we should have a generic URI of free form whose structure is not known a priori.

tcharding · 2018-10-26T10:20:40Z

--broker-address=[nats://]host[:port]

implies that you should use nats service. Idea for the future is possibly to have multiple types of brokers. Meaning we should have a generic URI of free form whose structure is not known a priori.

This seems to add weight to the idea of doing some sort of verification on this option string (possibly when the command starts). Did you want me to drop the use of 'nats' from the commit log @zolia?

Function naming. GenerateNewAddress ?

@tadovas I don't understand this comment sorry. Do you mean you don't like this function name?

zolia · 2018-10-26T11:40:25Z

--broker-address=[nats://]host[:port]
implies that you should use nats service. Idea for the future is possibly to have multiple types of brokers. Meaning we should have a generic URI of free form whose structure is not known a priori.

This seems to add weight to the idea of doing some sort of verification on this option string (possibly when the command starts). Did you want me to drop the use of 'nats' from the commit log @zolia?

from my understanding it should simply be:
--broker-address=URI
Then, depending on what kind of broker was used, it should produce sensible error, describing format if needed.

`golint` emits warnings: interface method parameter sessionId should be sessionID method parameter sessionId should be sessionID We should us capital letters for acronyms as is standard golang convention. After this is applied server/ lints cleanly. Use sessionID instead of sessionId for parameter. Signed-off-by: tcharding <me@tobin.cc>

We currently have four functions for creating a NATS address NewAddress NewAddressGenerate NewAddressForContact newAddressWithConnection `NewAddressGenerate` is not overly clear, lets rename it. In order to fit in with the other function names use `NewAddressFromHostAndID`. Signed-off-by: tcharding <me@tobin.cc>

Add table driven testing to `nats/discovery`. Signed-off-by: tcharding <me@tobin.cc>

Currently we unconditionally add `nats://` scheme to address string of the broker. It is reasonable for the user to expect to be able to pass an address of form `nats://localhost` when configuring the broker. Only add the scheme `nats:` if it is not already present in the URI. Signed-off-by: tcharding <me@tobin.cc>

In preparation for additional NATS address construction logic add an error return to the `DialogWaiterFactory`. Signed-off-by: tcharding <me@tobin.cc>

Currently we add the hard coded broker port number 4222 unconditionally. If a user wishes to configure nats to use a different port number then we need to accept this port number as part of the configuration for running myst. If we conditionally add the port number then the following is supported bin/run --broker-addres="localhost:3333" It seems that nats library should handle an address string that does not contain the port number by using the default port but our e2e tests fail if the port number is not present. Conditionally add broker port number to the address string. Signed-off-by: tcharding <me@tobin.cc>

Use `URI` and `URL` to be precise in what we accept for the broker address and the discovery service address. Also use backticks so that `URI`/`URL` shows up in the help menu instead of `value`. Signed-off-by: tcharding <me@tobin.cc>

Currently we have help output `Url`, we should use capitals for acronyms here as we do in the rest of the help menu. Capitalise Url in help output, se 'URL' instead of 'Url'. Signed-off-by: tcharding <me@tobin.cc>

tcharding · 2018-10-31T02:47:48Z

@tadovas I used function name NewAddressFromHostAndID and not your suggested GenerateNewAddress in order to fit in with the naming of other creator functions in the file. Please feel free to improve upon the name.

Please review the changes to the help menu output since this is the first use of the back ticks. Has there been discussion already on this usage?

thanks

tcharding · 2018-10-31T05:31:45Z

@Waldz could you please remove WIP label (no longer 'Work in Progress') thanks.

stale · 2018-11-15T04:11:37Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

soffokl · 2018-11-15T04:49:42Z

@tadovas @zolia @vkuznecovas @Waldz Let's review this once again. Changes look reasonable, but it took too long without any actions.

soffokl · 2018-11-15T04:50:11Z

I'll fix CI, so it's ready to review.

tcharding requested review from Waldz and tadovas as code owners October 24, 2018 02:52

Waldz previously approved these changes Oct 24, 2018

View reviewed changes

Waldz requested review from soffokl, vkuznecovas and zolia October 24, 2018 04:34

soffokl reviewed Oct 24, 2018

View reviewed changes

Waldz added this to the Keliukis (0.5) milestone Oct 24, 2018

Waldz added the work in progress label Oct 25, 2018

tcharding dismissed Waldz’s stale review via 4bbbaf9 October 26, 2018 04:23

soffokl requested changes Oct 26, 2018

View reviewed changes

tadovas suggested changes Oct 26, 2018

View reviewed changes

vkuznecovas suggested changes Oct 26, 2018

View reviewed changes

tcharding added 8 commits October 31, 2018 13:36

Add table driven testing

5fc7f65

Add table driven testing to `nats/discovery`. Signed-off-by: tcharding <me@tobin.cc>

Add error to DialogWaiterFactory

e9652e1

In preparation for additional NATS address construction logic add an error return to the `DialogWaiterFactory`. Signed-off-by: tcharding <me@tobin.cc>

Update help menu for broker and discovery address

ca36afa

Use `URI` and `URL` to be precise in what we accept for the broker address and the discovery service address. Also use backticks so that `URI`/`URL` shows up in the help menu instead of `value`. Signed-off-by: tcharding <me@tobin.cc>

Capitalise Url in help output

6de8064

Currently we have help output `Url`, we should use capitals for acronyms here as we do in the rest of the help menu. Capitalise Url in help output, se 'URL' instead of 'Url'. Signed-off-by: tcharding <me@tobin.cc>

stale Bot added the stale label Nov 15, 2018

soffokl removed stale work in progress labels Nov 15, 2018

Merge branch 'master' into broker-port

98df49a

soffokl requested review from Waldz, soffokl, tadovas and vkuznecovas November 15, 2018 04:50

soffokl previously approved these changes Nov 15, 2018

View reviewed changes

vkuznecovas previously approved these changes Nov 15, 2018

View reviewed changes

tadovas previously approved these changes Nov 15, 2018

View reviewed changes

soffokl added 2 commits November 15, 2018 15:33

Merge branch 'master' into broker-port

dae57c0

Fixed auto merge and conflict resolution issue.

a268982

soffokl dismissed stale reviews from tadovas, vkuznecovas, and themself via a268982 November 15, 2018 10:25

soffokl requested a review from tadovas November 15, 2018 10:42

tadovas approved these changes Nov 15, 2018

View reviewed changes

soffokl approved these changes Nov 15, 2018

View reviewed changes

soffokl merged commit 29bf251 into mysteriumnetwork:master Nov 15, 2018

Uh oh!

Conversation

tcharding commented Oct 24, 2018

Testing

Uh oh!

soffokl left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding commented Oct 24, 2018

Uh oh!

tcharding commented Oct 24, 2018

Uh oh!

Waldz commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Oct 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zolia commented Oct 26, 2018

Uh oh!

tcharding commented Oct 26, 2018

Uh oh!

zolia commented Oct 26, 2018

Uh oh!

tcharding commented Oct 31, 2018

Uh oh!

tcharding commented Oct 31, 2018

Uh oh!

stale Bot commented Nov 15, 2018

Uh oh!

soffokl commented Nov 15, 2018

Uh oh!

soffokl commented Nov 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Waldz commented Oct 24, 2018 •

edited

Loading

tcharding commented Oct 25, 2018 •

edited

Loading

tcharding Oct 26, 2018 •

edited

Loading