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-181 Use proposals endpoint instead of create-session #69

Merged
merged 6 commits into from Dec 27, 2017

Conversation

Projects
None yet
4 participants
@donce
Copy link
Contributor

commented Dec 22, 2017

For client, proposals are fetched from a dedicated endpoint instead /client-create-session.

This should only be merged after updating discovery api DEV env from release/v1.

@@ -70,6 +71,27 @@ func (client *clientRest) NodeSendStats(nodeKey string, sessionList []dto.Sessio
return nil
}

func (client *clientRest) Proposals(nodeKey string) (proposals []dto_discovery.ServiceProposal, err error) {
response, err := client.doRequest("GET", "proposals", dto.ProposalsRequest{

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 22, 2017

Member

Shouldn't it be like "/proposals?byNodeKey=" ? Get cannot have body

This comment has been minimized.

Copy link
@donce

donce Dec 22, 2017

Author Contributor

Yes, will fix.

This comment has been minimized.

Copy link
@donce

donce Dec 22, 2017

Author Contributor

Fixed.

NodeKey: nodeKey,
})

if err == nil {

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 22, 2017

Contributor

Let's do the following way:

if err != nil {
    return
}
noErrorLogic()

This comment has been minimized.

Copy link
@donce

donce Dec 22, 2017

Author Contributor

This looked strange for me as well, but there are a lot such cases in this file!
Will fix this instance, but other instances should be refactored in a separate PR not to explode this one.

This comment has been minimized.

Copy link
@donce

donce Dec 22, 2017

Author Contributor

Done.


proposal := clientSession.ServiceProposal
// TODO: pass in selected proposal to session creation
clientSession, err := manager.mysteriumClient.SessionCreate(nodeKey)

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 22, 2017

Member

This is not needed and should go away. Client gets session id from node with last dialog message (VpnSession at the moment)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 22, 2017

Member

he can do it in separate request

This comment has been minimized.

Copy link
@donce

donce Dec 22, 2017

Author Contributor

Done (although separate request would have been better..)

return
proposals = []dto_discovery.ServiceProposal{proposal}
} else {
proposals = []dto_discovery.ServiceProposal{}

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 22, 2017

Contributor

shouldn't we return an error here instead of new empty proposal?

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 22, 2017

Member

If client by contract returns zero or more proposals then it's ok to return zero propsals, caller will decide if it is error (connection manager) or its normal( /proposals tequilapi endpoint)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 22, 2017

Member

he decided to push this error to upper logic level

This comment has been minimized.

Copy link
@donce

donce Dec 22, 2017

Author Contributor

Yeah - /v1/proposals endpoint works this way:

  • By default, it returns all proposals.
  • It allows filtering by some values. If filtering doesn't return any matches, it doesn't mean that it's an error - it means that there are no objects by given filters.

This comment has been minimized.

Copy link
@Waldz
manager.status = statusConnecting()
clientSession, err := manager.mysteriumClient.SessionCreate(NodeKey)

proposals, err := manager.mysteriumClient.Proposals(nodeKey)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 22, 2017

Member

Proposals() -> FindProposals()

This comment has been minimized.

Copy link
@donce

donce Dec 22, 2017

Author Contributor

Done.

@donce donce changed the title MYST-181 Use proposals endpoint MYST-181 Use proposals endpoint instead of create-session Dec 22, 2017

@Waldz

Waldz approved these changes Dec 22, 2017

@Waldz

This comment has been minimized.

Copy link
Member

commented Dec 22, 2017

Looks Good To Me

@donce donce merged commit 9679c09 into master Dec 27, 2017

@donce donce deleted the feature/MYST-181-use-proposals-endpoint branch Dec 27, 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.