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-102 client api connect validation #58

Merged
merged 15 commits into from Dec 15, 2017

Conversation

Projects
None yet
3 participants
@tadovas
Copy link
Member

commented Dec 15, 2017

/connection endpoint implemented, connection manager still in progress

tadovas and others added some commits Dec 7, 2017

Merge branch 'master' of github.com:MysteriumNetwork/node into featur…
…e/MYST-102-client-api-connect

* 'master' of github.com:MysteriumNetwork/node:
  Fixed parameter description
  Bind local api on localhost interface by default - slight security improvement
  fixed tests asking for network permission
Merge branch 'feature/MYST-102-client-api-connect' of github.com:Myst…
…eriumNetwork/node into feature/MYST-102-client-api-connect

* 'feature/MYST-102-client-api-connect' of github.com:MysteriumNetwork/node:

@tadovas tadovas self-assigned this Dec 15, 2017

SessionId string
}

type Manager interface {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 15, 2017

Member

Looks like filename should be client_connection/client_connection.go -> client_connection/manager.go

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

Noted and fixed.

)

func NewCommand(vpnMiddlewares ...openvpn.ManagementMiddleware) *CommandRun {
nats_discovery.Bootstrap()
openvpn.Bootstrap()

return &CommandRun{
Output: os.Stdout,

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 15, 2017

Member

Why? :/

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

Because its not used anywhere

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

Why to keep unused variables anyway? Boy scout rule -> keep a place a bit cleaner than found.

@@ -1,5 +1,5 @@
hash: 43125bb122714ffb721e8349d72a4a1b3ec1b0ed0707e2a6ea18f7553a6ed11d

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 15, 2017

Member

We did not change any dependencies, why update needed?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

As per discussion - previous merge was not handled correctly in other pull request

Wait() error
}

type nullManager struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 15, 2017

Member

Wer kinda had pattern to call such dummy stubs: nullManager -> ManagerFake

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

This component is still in progress and will be eventually renamed in other branch

@@ -16,7 +16,7 @@ type tequilapiTestSuite struct {
func (testSuite *tequilapiTestSuite) SetupSuite() {
var err error
idmFake := identity.NewIdentityManagerFake()
testSuite.server, err = StartNewServer("localhost", 0, NewApiEndpoints(idmFake))
testSuite.server, err = StartNewServer("localhost", 0, NewApiEndpoints(idmFake, nil))

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 15, 2017

Member

nil, You cat get segmentation error like that

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

Noted. It also points out that in order just to check simple endpointer we need a lot of fake depencies because all endpoints are registered. Will refactor to separate functions for endpoint registering:
router :=NewApiRouter()
registerIdentitiesEndpoint(router ,
registerConnectionEndpoint(router,


func toStatusResponse(status client_connection.Status) statusResponse {
return statusResponse{
Status: fmt.Sprint(status.State),

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 15, 2017

Member

Looks weird, could be string(status.State) or status.State.toString()

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

Noted.

}

func validateConnectionRequest(cr *connectionRequest) *validation.FieldErrorMap {
errors := validation.NewErrorMap()

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 15, 2017

Member

+100 simple and no deps

ALREADY_CONNECTED = errors.New("already connected")
)

type Status struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 15, 2017

Member

Status and State looks confusing. What about ranaming Status -> Connection?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

Makes sense. But Status is object which represents connection state and possible other information like bytes sent/received last error etc. My suggestion Status -> ConnectionStatus

{ "code" : "invalid" , "message" : "field invalid" }
]
}`,
string(v[:]))

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 15, 2017

Member

string(v[:])) -> string(v) would be the same

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

Noted.

@tadovas
Copy link
Member Author

left a comment

Fixed/commented according to feedback

`{
"identity" : "my-identity",
"nodeKey" : "required-node"
}`))

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 15, 2017

Member

Argument alignment seems to be messed up within most tests. Shouldn't the trailing ")" be on a separate line?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

It's not java, end of line has implicit ; which breaks golang code :)

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 15, 2017

Author Member

It works when you do like ), and ) on separate line. Thanks @Waldz for pointing out.

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 15, 2017

Member

It was @ignasbernotas +1 who pointed out

@Waldz Waldz changed the title Feature/myst 102 client api connect MYST-102 client api connect validation Dec 15, 2017

@tadovas tadovas merged commit d30cda4 into master Dec 15, 2017

@tadovas tadovas deleted the feature/MYST-102-client-api-connect branch Dec 15, 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.