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-703 Refactor CLI flag bootstraping #329

Merged
merged 28 commits into from Sep 7, 2018

Conversation

Projects
None yet
4 participants
@Waldz
Copy link
Member

commented Sep 5, 2018

No description provided.

@Waldz Waldz requested review from tadovas, interro, soffokl, zolia and mysteriumnetwork/core Sep 5, 2018

soffokl added some commits Sep 4, 2018

cmd/di.go Outdated
)
}

// function decides on etwork definition combined from testnet/localnet flags and possible overrides

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 6, 2018

Member

etwork -> network

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 6, 2018

Author Member

Fixed typo

@@ -27,14 +27,19 @@ import (
"strconv"
"syscall"

log "github.com/cihub/seelog"
"github.com/ethereum/go-ethereum/log"

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 6, 2018

Member

Looks like wrong logging package

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 6, 2018

Author Member

Fixed in all places

)

// IdentityRegistry checks whenever given identity is registered
type IdentityRegistry interface {
// Registry checks whenever given identity is registered

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 6, 2018

Member

Registry of what? It either should be in identity package or old name should be returned. Current import ant usage doesn't sound: import "...../registry" registry.Registry

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 6, 2018

Author Member

This is not package registry, but rather parent/child package identity/registry. This is why I allias those package then importing:

identity_loading "github.com/mysteriumnetwork/node/identity/loading"
identity_registry "github.com/mysteriumnetwork/node/identity/registry"

So I thought there is not point to repeat name identity_loading.IdentityRegistry. But aliasing each time is easy to forget, so I dont know perfect pattern in Golang :/

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 6, 2018

Author Member

I reverted this rename identity_registry.IdentityRegistry -> identity_registry.Registry until we know how to proceed:

  • A. Keep interface in "parent" package identity.Registry, implementation in identity/registry
  • B. Put interfaces on caller side dialog.IdentityRegistry, instead registry.IdentityRegistry. Cons: what if we have 2 callers
  • C. Name "parent/child" packages identityregistry instead of identity/registry
@@ -0,0 +1,42 @@
/*

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 6, 2018

Member

openvpn/core ? These flags are not related to core of openvpn. We tried so hard to move out all code not directly related to openvpn functionality itself.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 6, 2018

Author Member

It should belong to Openvpn Adapter in future, we will move it there later

@Waldz Waldz force-pushed the Waldz:feature/MYST-703-refactor-flags branch from 5e1405e to 64415a1 Sep 6, 2018

@Waldz

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

@tadovas @soffokl All comments fixed

Waldz added some commits Sep 1, 2018

cmd/di.go Outdated
return nil
}

func (di *Dependencies) bootstrapIdentity(directories node.DirectoryOptions) {

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

Bootstrap identity components? (it's also keystore, identity manager, singer factory etc.)

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

Rebamed all functions

cmd/di.go Outdated
}
}

func (di *Dependencies) bootstrapLocation(options node.LocationOptions, configDirectory string) {

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

maybe bootstrapLocationResolver ?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

I bootstrap all location configs here, it's like "location" module

@@ -0,0 +1,55 @@
/*

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

flags_location -> location_flags.go ?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

File name is not so important flags_network.go contains logic related to network flags only: flag constants, registering and parsing

IpifyUrl string
LocationDatabase string

Openvpn openvpn_core.NodeOptions

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

NodeOptions in openvpn core package? Is it really part of openvpn?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

Working on that in another PR

@@ -1,5 +1,5 @@
/*
* Copyright (C) 2017 The "MysteriumNetwork/node" Authors.
* Copyright (C) 2018 The "MysteriumNetwork/node" Authors.

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

location_options.go ?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

I splited file options.go to smaller pieces, this is why
screen shot 2018-09-07 at 14 07 07

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

OK. Lets keep that

@@ -17,11 +17,6 @@

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

Network_options.go ?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

Made consistent

@@ -15,22 +15,24 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package identity
package loading

This comment has been minimized.

Copy link
@tadovas

tadovas Sep 7, 2018

Member

Package is loading....... 15%, 20%. Naming :)

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

Renamed to identity/selector

@Waldz Waldz force-pushed the Waldz:feature/MYST-703-refactor-flags branch from 64415a1 to efc114d Sep 7, 2018

cmd/di.go Outdated
}

normalizedAddress := common.HexToAddress(options.EtherPaymentsAddress)
if normalizedAddress.String() != metadata.DefaultNetwork.PaymentsContractAddress.String() {

This comment has been minimized.

Copy link
@zolia

zolia Sep 7, 2018

Member

Do we really need to convert both types to String? Structs should be comparable here.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

Remade

cmd/di.go Outdated
func (di *Dependencies) bootstrapLocation(options node.OptionsLocation, configDirectory string) {
di.IPResolver = ip.NewResolver(options.IpifyUrl)

switch {

This comment has been minimized.

Copy link
@zolia

zolia Sep 7, 2018

Member

Two options here, simple if / else should be enough.

@@ -52,13 +52,17 @@ func NewResolverWithTimeout(ipifyUrl string, timeout time.Duration) Resolver {
}
}

type ipResponse struct {

This comment has been minimized.

Copy link
@zolia

zolia Sep 7, 2018

Member

imho bad name here, should be resultingIP or returnedIP

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

Not my scope. And all structures in tequilapi endpoints has *Response and *Request naming

This comment has been minimized.

Copy link
@zolia

zolia Sep 7, 2018

Member

but it is not in tequilapi module / submodule and it is not generic but concrete type.

networkDefinition := GetNetworkDefinition(options.NetworkOptions)
mysteriumClient := server.NewClient(networkDefinition.DiscoveryAPIAddress)

func NewNode(

This comment has been minimized.

Copy link
@zolia

zolia Sep 7, 2018

Member

We could use NodeOption or NodeFlags structure here, not multi parameter factory.. as per: https://hackmongo.com/page/golang-antipatterns/#runaway-arguments

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

Yeah, that's what i'm moving towards. But not so easy while we have 3 executables. Would like to do it in upcoming PR

// NewIdentityRegistry creates identity registry service which uses blockchain for information
func NewIdentityRegistry(contractCaller bind.ContractCaller, registryAddress common.Address) (IdentityRegistry, error) {
func NewIdentityRegistry(contractCaller bind.ContractCaller, registryAddress common.Address) (*contractRegistry, error) {

This comment has been minimized.

Copy link
@zolia

zolia Sep 7, 2018

Member

I would leave IdentityRegistry as is, since the only way identities can be registered is through contract.
Factory method also is called NewIdentityRegistry not NewContractRegistry.

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

We dont return interface type, but exact type of returned struct. This is why I renamed

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

Renamed to NewIdentityRegistryContract()

@@ -30,11 +30,16 @@ import (
log "github.com/cihub/seelog"
)

// NodeOptions describes possible parameters of Openvpn configuration
type NodeOptions struct {
Binary string

This comment has been minimized.

Copy link
@zolia

zolia Sep 7, 2018

Member

Would rename it to BinaryPath

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 7, 2018

Author Member

Renamed

Waldz added some commits Sep 6, 2018

@Waldz Waldz force-pushed the Waldz:feature/MYST-703-refactor-flags branch from 65f0a05 to 48499e4 Sep 7, 2018

@Waldz

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2018

@tadovas @zolia all comments fixed

@zolia

zolia approved these changes Sep 7, 2018

@tadovas

tadovas approved these changes Sep 7, 2018

Copy link
Member

left a comment

I don't know where we gonna land with these huge refactoring, but keeping my fingers crossed

@soffokl

soffokl approved these changes Sep 7, 2018

@Waldz Waldz merged commit 3216981 into mysteriumnetwork:master Sep 7, 2018

1 check passed

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

@Waldz Waldz deleted the Waldz:feature/MYST-703-refactor-flags branch Sep 7, 2018

@Waldz Waldz referenced this pull request Sep 11, 2018

Closed

Tequilapi. Add endpoints for service management #339

5 of 5 tasks complete
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.