Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MYST 361: add -discovery-address and -broker-address command line options #172

Merged
merged 8 commits into from Feb 26, 2018

Conversation

Projects
None yet
5 participants
@zolia
Copy link
Member

commented Feb 21, 2018

PS adds -discovery-address and -broker-address command line options to override compiled-in options

@@ -4,6 +4,7 @@
function get_linker_ldflags {
echo "
-X 'github.com/mysterium/node/server.mysteriumApiUrl=${MYSTERIUM_API_URL}'
-X 'github.com/mysterium/node/communication/nats/discovery.natsServerIP=${NATS_SERVER_IP}'
-X 'github.com/mysterium/node/server.natsServerIP=${NATS_SERVER_IP}'
-X 'github.com/mysterium/node/client.mysteriumApiUrl=${MYSTERIUM_API_URL}'

This comment has been minimized.

Copy link
@Waldz

Waldz Feb 21, 2018

Member

Why mysteriumApiUrl several times

This comment has been minimized.

Copy link
@zolia

zolia Feb 22, 2018

Author Member

its in different packages. to remove duplication I could make it public..

@shroomist
Copy link
Contributor

left a comment

lgtm

@@ -17,6 +17,8 @@ type mockedSigner struct {
signatureToReturn identity.Signature
}

var testMysteriumApiUrl = "http://testUrl"

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

We shouldn't depend that url is mysterium - it can be just testRequestURL.
Also, URL has to be upper-case.

}

const defaultLocationDatabase = "GeoLite2-Country.mmdb"

var MysteriumApiUrl string

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

Undocumented.

source .env

[ -n "$MYSTERIUM_API_URL" ] && DISCOVERY="-discovery-address=$MYSTERIUM_API_URL"
[ -n "$NATS_SERVER_IP" ] && BROKER="-broker-address=$NATS_SERVER_IP"

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

Isn't too dashes common practice for key-value params?

--discovery-address=FOO

This comment has been minimized.

Copy link
@zolia

zolia Feb 26, 2018

Author Member

I agree, but its true for all options. Legacy.

flags.StringVar(
&options.DiscoveryAPIAddress,
"discovery-address",
server.MysteriumApiUrl,

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

client shouldn't rely on server for options - they should be independant.
Can we extract this param into common class in cmd package, i.e. cmd.CommonOptions?

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 26, 2018

Member

This is gotcha :) and example of bad naming. "server" package has nothing to do with server functionality, it's simply part of discovery api functionality.

This comment has been minimized.

Copy link
@zolia

zolia Feb 26, 2018

Author Member

true, moved to cmd package.

}

const defaultLocationDatabase = "GeoLite2-Country.mmdb"

var MysteriumApiUrl string

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

MysteriumAPIURL

var mysteriumApiUrl string

func newGetRequest(path string, params url.Values) (*http.Request, error) {
func newGetRequest(apiURI, path string, params url.Values) (*http.Request, error) {

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

Since mApi.discoveryAPIAddress seems to be used as apiURL param value in all calls, does it make sense to parametrize it?

This comment has been minimized.

Copy link
@zolia

zolia Feb 26, 2018

Author Member

not sure about this place, since its functions and not member methods we should not depend on internal state. Should we hardcoded in apiURI, can it be considered a state or never changing constant..

sudo ./build/client/mysterium_client \
--config-dir=bin/client_package/config \
--runtime-dir=build/client \
$DISCOVERY \

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

Shouldn't this param be passed in docker-entrypoints as well?

This comment has been minimized.

Copy link
@zolia

zolia Feb 26, 2018

Author Member

it depends from environment we want to build for.. we should compile in for testnet and only for development to show our own discovery.

This comment has been minimized.

Copy link
@zolia

zolia Feb 26, 2018

Author Member

is is passed now

@@ -17,6 +17,8 @@ type mockedSigner struct {
signatureToReturn identity.Signature
}

var testRequestApiUrl = "http://testUrl"

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

testRequestAPIURL

@@ -1,4 +1,4 @@
package server
package requestor

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

requestor name is confusing, since there is no object named requestor - this package is just a bunch of functions.
What about package request or requests?
I.e. requests.NewGetRequest looks quite good.

This comment has been minimized.

Copy link
@tadovas

tadovas Feb 26, 2018

Member

requests.Get looks even better :)

zolia added some commits Feb 26, 2018

introduce MYSTERIUM_DISCOVERY_ADDRESS and MYSTERIUM_BROKER_ADDRESS en…
…v variables to control respective cmd line options
@@ -7,15 +7,25 @@ import (
"net/http"
)

type ErrorResponse struct {

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

This should be private.

@@ -5,3 +5,6 @@ MYSTERIUM_CLIENT_TEQUILAPI_ADDRESS="localhost"

# Port for listening incoming api requests
MYSTERIUM_CLIENT_TEQUILAPI_PORT="4050"

# Mysterium discovery address (default: http://testnet.mysterium.network:5000/v1)

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

How http://testnet.mysterium.network:5000/v1 is default? Isn't value in .env the default?

source .env

[ -n "$MYSTERIUM_API_URL" ] && DISCOVERY="--discovery-address=$MYSTERIUM_API_URL"
[ -n "$NATS_SERVER_IP" ] && BROKER="--broker-address=$NATS_SERVER_IP"

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

DISCOVERY and BROKER are too common - perhaps DISCOVERY_OPTION and BROKER_OPTION are more explicit?

}

const defaultLocationDatabase = "GeoLite2-Country.mmdb"

var natsServerIP string

This comment has been minimized.

Copy link
@donce

donce Feb 26, 2018

Contributor

brokerIP?

@donce

donce approved these changes Feb 26, 2018

@zolia zolia merged commit afee6ae into master Feb 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@zolia zolia deleted the feature/MYST-361-pass-api-nats-ips-via-cmdline branch Feb 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.