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-682 Move mysterium_server logic to "mysterium_node service" subcommand #324

Merged

Conversation

Projects
None yet
4 participants
@Waldz
Copy link
Member

commented Aug 28, 2018

Output of `bin/build && bin/run help service`


NAME:
   mysterium_node service - Starts and publishes service on Mysterium Network

USAGE:
   mysterium_node service [command options]

OPTIONS:
   --identity value             Keystore's identity used to provide service. If not given identity will be created automatically
   --identity.passphrase value  Used to unlock keystore's identity
   --openvpn.protocol value     Openvpn protocol to use. Options: { udp, tcp } (default: "udp")
   --openvpn.port value         Openvpn port to use. Default 1194 (default: 1194)
   --location.country value     Service location country. If not given country is autodetected

@Waldz Waldz requested review from tadovas, interro, soffokl, zolia and mysteriumnetwork/core Aug 28, 2018

@Waldz Waldz force-pushed the Waldz:feature/MYST-682-service-subcommand branch from a77d432 to ed63649 Aug 29, 2018

Waldz added some commits Aug 23, 2018

@Waldz Waldz force-pushed the Waldz:feature/MYST-682-service-subcommand branch from ed63649 to c0aae50 Aug 29, 2018

Waldz added some commits Aug 26, 2018

@Waldz Waldz force-pushed the Waldz:feature/MYST-682-service-subcommand branch from c0aae50 to d2e38ed Aug 29, 2018


cmd.RegisterSignalCallback(utils.SoftKiller(stopCommand))

go nodeInstance.Start()

This comment has been minimized.

Copy link
@soffokl

soffokl Aug 29, 2018

Member

nodeInstance.Start() returns an error, should we at least log it somehow?

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 30, 2018

Author Member

Improoved

@@ -95,6 +95,20 @@ func TestNewAddressForContact_UnknownDefinition(t *testing.T) {
assert.Nil(t, address)
}

func TestAddress_Disconnect_EmptyAddress(t *testing.T) {
address := &AddressNATS{}
address.Disconnect()

This comment has been minimized.

Copy link
@soffokl

soffokl Aug 29, 2018

Member

Do we need any assertion here, or it checks "no panic" case?

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 30, 2018

Author Member

Exactly, I fixed problem with panic and covered with test

openvpnServiceAddress: func(outboundIP, publicIP string) string {
//TODO public ip could be overriden by arg options if needed
//TODO public ip could be overriden by arg nodeOptions if needed

This comment has been minimized.

Copy link
@soffokl

soffokl Aug 29, 2018

Member

overriden -> overridden

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 30, 2018

Author Member

Fixed typo

if publicIP != outboundIP {
forwardInfo := fmt.Sprintf("%s:%v -> %s:%v", publicIP, options.OpenvpnPort, outboundIP, options.OpenvpnPort)
log.Warnf(
`WARNING: It seems that publicaly visible ip: [%s] does not match your local machines ip: [%s].

This comment has been minimized.

Copy link
@soffokl

soffokl Aug 29, 2018

Member

publicaly -> publicly

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 30, 2018

Author Member

Fixed typo

log.Warnf(
`WARNING: It seems that publicaly visible ip: [%s] does not match your local machines ip: [%s].
You should probaly need to do port forwarding on your router: %s.`,
You should probaly need to do port forwarding on your router: %s:%v -> %s:%v.`,

This comment has been minimized.

Copy link
@soffokl

soffokl Aug 29, 2018

Member

probaly -> probably

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 30, 2018

Author Member

Fixed typo


stopCommand := func() error {
if err := serviceManager.Kill(); err != nil {
return err

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 29, 2018

Member

Maybe all errors on stop should be logged as warnings, and stop should proceed anyway? Now return in the middle of stop leaves command in interesting state

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 30, 2018

Author Member

Improved


flags.StringVar(
&options.NodeOptions.OpenvpnBinary,
"openvpn.binary",

This comment has been minimized.

Copy link
@interro

interro Aug 29, 2018

Contributor

Does mysterium_node service accept --openvpn.binary parameter?

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 30, 2018

Author Member

Yes, it's passed in global variables:

mysterium_node --openvpn.binary=openvpn service --openvpn.protocol=tcp --openvpn.port=1111

Waldz added some commits Aug 30, 2018

@soffokl
Copy link
Member

left a comment

Looks good to me.

@tadovas
Copy link
Member

left a comment

Alright lets do it

@Waldz Waldz merged commit ae63fa8 into mysteriumnetwork:master Aug 30, 2018

1 check passed

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

@Waldz Waldz deleted the Waldz:feature/MYST-682-service-subcommand branch Aug 30, 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.