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

Node registration sends service proposal #38

Merged
merged 3 commits into from Oct 18, 2017

Conversation

Projects
None yet
2 participants
@ignasbernotas
Copy link
Member

commented Oct 18, 2017

No description provided.

@ignasbernotas ignasbernotas requested review from interro and Waldz Oct 18, 2017

@@ -74,7 +77,14 @@ func (cmd *CommandRun) Run(options CommandOptions) (err error) {
return err
}

if err := cmd.MysteriumClient.NodeRegister(options.NodeKey, vpnClientConfigString); err != nil {
proposal := service_discovery.NewServiceProposal(
dto_discovery.Identity(options.NodeKey),

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 18, 2017

Member

Same identity object created 2 times

@@ -5,21 +5,22 @@ import (

"fmt"
log "github.com/cihub/seelog"
dto2 "github.com/mysterium/node/service_discovery/dto"

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 18, 2017

Member

dto_discovery

client.connectionConfigByNode[nodeKey] = connectionConfig
log.Info(MYSTERIUM_API_LOG_PREFIX, "Fake node registered: ", nodeKey)
func (client *clientFake) NodeRegister(proposal dto2.ServiceProposal) (err error) {
client.connectionConfigByNode[string(proposal.ProviderId)] = proposal

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 18, 2017

Member

connectionConfigByNode -> proposalsByProvider

@@ -10,6 +10,7 @@ import (

log "github.com/cihub/seelog"
"github.com/mysterium/node/server/dto"
dto2 "github.com/mysterium/node/service_discovery/dto"

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 18, 2017

Member

dto_discovery

import "github.com/mysterium/node/server/dto"
import (
"github.com/mysterium/node/server/dto"
dto2 "github.com/mysterium/node/service_discovery/dto"

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 18, 2017

Member

dto_discovery

@@ -1,6 +1,7 @@
package dto

import "github.com/mysterium/node/service_discovery/dto"

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 18, 2017

Member

dto_discovery

return
}

// this is a temporary solution for pulling the VPN config

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 18, 2017

Member
  1. Why not to add to Session DTO?
  2. I think endpoint /client_create_session is still unchanged. Why to change it?
// this is a temporary solution for pulling the VPN config
temp := struct {
ConnectionConfig string `json:"connection_config"`
}{}

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 18, 2017

Member

initialization {} unnecessarily:

var temp struct {
     ConnectionConfig string `json:"connection_config"`
 }

@ignasbernotas ignasbernotas force-pushed the feature/MYST-38-proposal-register branch from 49ee8cc to c5a1dae Oct 18, 2017

@Waldz

Waldz approved these changes Oct 18, 2017

@Waldz Waldz merged commit 0147037 into master Oct 18, 2017

@Waldz Waldz deleted the feature/MYST-38-proposal-register branch Oct 18, 2017

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.