MYST-688 Start having subcommands of "mysterium_node" #316

Waldz · 2018-08-23T01:30:13Z

Output of bin/build && bin/run --help

NAME:
   mysterium_node - VPN server and client for Mysterium Network https://mysterium.network/

USAGE:
   mysterium_node [global options] command [command options] [arguments...]

VERSION:
   source.dev-build

AUTHOR:
   The "MysteriumNetwork/node" Authors <mysterium-dev@mysterium.network>

COMMANDS:
     version  Show version
     license  Show license
     help, h  Shows a list of commands or help for one command

GLOBAL OPTIONS:
   --data-dir value                 Data directory containing keystore & other persistent files (default: "/Users/waldz/.mysterium")
   --config-dir value               Configs directory containing all configuration, script and helper files (default: "/Users/waldz/workspace/go/src/github.com/mysterium/node/build/node/config")
   --runtime-dir value              Runtime writable directory for temp files (default: "/Users/waldz/workspace/go/src/github.com/mysterium/node/build/node")
   --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)
   --testnet                        Defines test network configuration
   --localnet                       Defines network configuration which expects localy deployed broker and discovery services
   --discovery-address value        Address (URL form) of discovery service (default: "https://testnet-api.mysterium.network/v1")
   --broker-address value           Address (IP or domain name) of message broker (default: "testnet-broker.mysterium.network")
   --ether.client.rpc value         Url or IPC socket to connect to ethereum node, anything what ethereum client accepts - works (default: "https://ropsten.infura.io")
   --ether.contract.payments value  Address of payments contract (default: "0xbe5F9CCea12Df756bF4a5Baf4c29A10c3ee7C83B")
   --openvpn.binary value           openvpn binary to use for Open VPN connections (default: "openvpn")
   --ipify-url value                Address (URL form) of ipify service (default: "https://api.ipify.org/")
   --location.database value        Service location autodetect database of GeoLite2 format e.g. http://dev.maxmind.com/geoip/geoip2/geolite2/ (default: "GeoLite2-Country.mmdb")
   --cli                            Run an interactive CLI based Mysterium UI
   --help, -h                       show help
   --version, -v                    print the version

COPYRIGHT:
   Mysterium Node Copyright (C) 2017 The "MysteriumNetwork/node" Authors.
This program comes with ABSOLUTELY NO WARRANTY; for details run command 'license --warranty'.
This is free software, and you are welcome to redistribute it
under certain conditions; run command 'license --conditions' for details.

soffokl · 2018-08-23T01:57:52Z

cmd/network.go

@@ -109,3 +69,46 @@ func ParseNetworkOptions(flags *flag.FlagSet, options *NetworkOptions) {
 		"Address of payments contract",
 	)
 }
+
+// RegisterNetworkFlags function register directory options to flag set
+func RegisterNetworkFlags(flags *[]cli.Flag, options *node.NodeOptions) error {


No error can be returned from the function, we can remove it.

soffokl · 2018-08-23T01:59:35Z

cmd/network.go

+		},
+		cli.BoolFlag{
+			Name:        "localnet",
+			Usage:       "Defines network configuration which expects localy deployed broker and discovery services",


typo: locally

soffokl · 2018-08-23T02:06:44Z

cmd/commands/application/command.go

+	if err := cmd.RegisterDirectoryFlags(&app.Flags, &options); err != nil {
+		return app, err
+	}
+	app.Flags = append(app.Flags, tequilapiAddressFlag, tequilapiPortFlag)


I'd moved this append after the if block, to make it more readable.

Improved flag registration

soffokl · 2018-08-23T02:12:47Z

cmd/commands/license/command_test.go

+
+import (
+	"testing"
+


nitpicking: extra spaces in imports.

soffokl · 2018-08-23T02:20:28Z

cmd/commands/version/command_test.go

+
+import (
+	"testing"
+


nitpicking: extra spaces in imports.

soffokl · 2018-08-23T02:23:17Z

cmd/directories.go

-func (options *DirectoryOptions) Check() error {
-	err := ensureDirExists(options.Config)
+// RegisterDirectoryFlags function register directory options to flag set
+func RegisterDirectoryFlags(flags *[]cli.Flag, options *node.NodeOptions) error {


* for flags can be omitted here, slice already a reference type.

Tried, but did not succeed pass wo reference sign &

just removing all & and * solves the problem.
like following:

Yes I tried, but in that cases, values is appended to copies :/

OK, got it. Sorry for confusing.

soffokl · 2018-08-23T02:26:49Z

cmd/mysterium_client/mysterium_client.go

@@ -19,20 +19,25 @@ package main

 import (
 	"fmt"
+	"os"
+	"path/filepath"
+


nitpicking: extra spaces in imports.

soffokl · 2018-08-23T02:38:47Z

core/node/options.go

+package node
+
+// NodeOptions describes options which are required to start Command
+type NodeOptions struct {


type name will be used as node.NodeOptions by other packages, and that stutters; consider calling this Options (golint)

tadovas · 2018-08-23T06:06:54Z

cmd/commands/application/command.go

+	app.Copyright = licenseCopyright
+
+	if err := cmd.RegisterDirectoryFlags(&app.Flags, &options); err != nil {
+		return app, err


Is it intentional to return app object and not-nil error? Normal go convensions say that in case of error - returned object contains undefined state and should not be used (from caller perspective)

fixed to return nil, err

tadovas · 2018-08-23T06:17:38Z

logconfig/config.go

@@ -30,7 +30,8 @@ const seewayLogXmlConfig = `
 </seelog>
 `

-func init() {
+// Bootstrap loads seelog package into the overall system
+func Bootstrap() {


why explicit Bootstrap calling is better when previous init() way? Init is guaranteed to be called only once per whole program. Bootstrap can be called in multiple places by different packages - and introduce very interesting side effects if not carefully used.

Longer is very angry of such hack then you include package with _

That practice is allowed only in tests.go or main.go

Ok I see. In that case - let's Bootstrap instead of init

tadovas · 2018-08-23T06:18:39Z

All in all - nice 👍

tadovas

Please address @soffokl comments too

soffokl · 2018-08-23T15:01:20Z

cmd/commands/application/command.go

+		return err
+	}
+
+	*flags = append(*flags, tequilapiAddressFlag, tequilapiPortFlag)


I was talking about moving all appends together:

cmd.RegisterNetworkFlags(flags, &options) *flags = append(*flags, tequilapiAddressFlag, tequilapiPortFlag) *flags = append(*flags, openvpnBinaryFlag, ipifyUrlFlag, locationDatabaseFlag) *flags = append(*flags, cliFlag)

But I want to keep specific order of flags

soffokl · 2018-08-23T15:34:40Z

cmd/commands/application/command.go

+	return app, nil
+}
+
+func registerFlags(flags *[]cli.Flag) error {


Please, take a look on comment here.

soffokl · 2018-08-23T15:34:53Z

cmd/directories.go

-func (options *DirectoryOptions) Check() error {
-	err := ensureDirExists(options.Config)
+// RegisterDirectoryFlags function register directory options to flag set
+func RegisterDirectoryFlags(flags *[]cli.Flag, options *node.Options) error {


Please, take a look on comment here.

soffokl · 2018-08-23T15:35:03Z

cmd/network.go

@@ -109,3 +69,46 @@ func ParseNetworkOptions(flags *flag.FlagSet, options *NetworkOptions) {
 		"Address of payments contract",
 	)
 }
+
+// RegisterNetworkFlags function register directory options to flag set
+func RegisterNetworkFlags(flags *[]cli.Flag, options *node.Options) {


Please, take a look on comment here.

soffokl

Looks good to me.

Waldz requested a review from a team August 23, 2018 01:30

Waldz requested a review from tadovas as a code owner August 23, 2018 01:30

Waldz changed the title ~~MYST-688 Integrate cli subcommands tool~~ MYST-688 Start having subcommands of executable Aug 23, 2018

Waldz changed the title ~~MYST-688 Start having subcommands of executable~~ MYST-688 Start having subcommands of "mysterium_node" Aug 23, 2018

Waldz force-pushed the feature/MYST-688-executable-subcommands branch 2 times, most recently from d0ea62b to 05d6d1f Compare August 23, 2018 01:48

soffokl reviewed Aug 23, 2018

View reviewed changes

tadovas reviewed Aug 23, 2018

View reviewed changes

tadovas suggested changes Aug 23, 2018

View reviewed changes

Waldz force-pushed the feature/MYST-688-executable-subcommands branch from 05d6d1f to c6b6094 Compare August 23, 2018 13:15

soffokl reviewed Aug 23, 2018

View reviewed changes

Waldz force-pushed the feature/MYST-688-executable-subcommands branch 2 times, most recently from ae62894 to 4eee4b0 Compare August 24, 2018 18:56

soffokl previously approved these changes Aug 27, 2018

View reviewed changes

Waldz added 10 commits August 28, 2018 13:37

Add "cli" package

48bb158

Rename "myusterium_client" -> to "mysterium_node"

d1c894d

Start having node application

6b800ad

Start having version command

2c82ab3

Remove suffix from command.go files

04e9623

Extract NodeOptions internal structure to core package

206d2c6

Extract DirectoryOptions internal structure to core package

b52216a

Extract NetworkOptions internal structure to core package

d549a69

Calculate network from NetworkOptions in core package

76f8dfb

Move stopper to utils

afd6d9b

Waldz dismissed soffokl’s stale review via 298d167 August 28, 2018 10:38

Waldz force-pushed the feature/MYST-688-executable-subcommands branch from 4eee4b0 to 298d167 Compare August 28, 2018 10:38

Extract Node internal structure to core package

a142549

Waldz added 6 commits August 28, 2018 14:05

Start having directory flags

00e75f5

Start having network flags

8268d30

Start having lisense command

595b8ee

Improve application flags registration

06bf4c1

nitpicking: extra spaces in imports

5d87234

Rename node.NodeOptions -> node.Options

c028b32

Waldz force-pushed the feature/MYST-688-executable-subcommands branch from 298d167 to c028b32 Compare August 28, 2018 11:07

soffokl approved these changes Aug 28, 2018

View reviewed changes

tadovas approved these changes Aug 28, 2018

View reviewed changes

Waldz merged commit de4a89a into mysteriumnetwork:master Aug 28, 2018

Waldz deleted the feature/MYST-688-executable-subcommands branch August 28, 2018 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MYST-688 Start having subcommands of "mysterium_node" #316

MYST-688 Start having subcommands of "mysterium_node" #316

Waldz commented Aug 23, 2018

soffokl Aug 23, 2018

Waldz Aug 23, 2018

soffokl Aug 23, 2018

Waldz Aug 23, 2018

soffokl Aug 23, 2018

Waldz Aug 23, 2018

soffokl Aug 23, 2018

Waldz Aug 23, 2018

soffokl Aug 23, 2018

Waldz Aug 23, 2018

soffokl Aug 23, 2018

Waldz Aug 23, 2018

soffokl Aug 23, 2018

Waldz Aug 24, 2018

soffokl Aug 24, 2018

soffokl Aug 23, 2018

Waldz Aug 23, 2018

soffokl Aug 23, 2018

Waldz Aug 23, 2018

tadovas Aug 23, 2018 •

edited

Waldz Aug 23, 2018

tadovas Aug 23, 2018 •

edited

Waldz Aug 23, 2018

Waldz Aug 23, 2018 •

edited

tadovas Aug 23, 2018

tadovas commented Aug 23, 2018

tadovas left a comment

soffokl Aug 23, 2018

Waldz Aug 23, 2018

soffokl Aug 23, 2018

soffokl Aug 23, 2018

soffokl Aug 23, 2018

soffokl Aug 23, 2018

soffokl left a comment

MYST-688 Start having subcommands of "mysterium_node" #316

MYST-688 Start having subcommands of "mysterium_node" #316

Conversation

Waldz commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadovas Aug 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadovas Aug 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Waldz Aug 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadovas commented Aug 23, 2018

tadovas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soffokl left a comment

Choose a reason for hiding this comment

tadovas Aug 23, 2018 •

edited

tadovas Aug 23, 2018 •

edited

Waldz Aug 23, 2018 •

edited