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

Dont expose session transport DTOs to external world #405

Merged
merged 10 commits into from Oct 1, 2018

Conversation

Projects
None yet
4 participants
@Waldz
Copy link
Member

commented Sep 25, 2018

Updates #382

@Waldz Waldz requested a review from mysteriumnetwork/core Sep 25, 2018

@Waldz Waldz requested a review from tadovas as a code owner Sep 25, 2018

@vkuznecovas

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

but your e2e are failing :(

Show resolved Hide resolved core/connection/manager.go
Show resolved Hide resolved core/connection/manager.go Outdated
service Service,
discoveryService *discovery.Discovery,
) *Manager {
logconfig.Bootstrap()

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 25, 2018

Member

We are doing the same logconfig.Bootstrap() at the NewNode function.
What is the reason for doing it here too?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 25, 2018

Author Member

Fixed

@@ -97,7 +97,7 @@ func (client *ClientFake) FindProposals(providerID string) (proposals []dto_disc

// SendSessionStats heartbeats that session is still active + session upload and download amounts
func (client *ClientFake) SendSessionStats(sessionId session.SessionID, sessionStats dto.SessionStats, signer identity.Signer) (err error) {
log.Info(mysteriumAPILogPrefix, "Session stats sent: ", sessionId)
log.Info(mysteriumAPILogPrefix, "SessionDto stats sent: ", sessionId)

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 25, 2018

Member

Is it correct that SessionDto should be here?

This comment has been minimized.

Copy link
@Waldz

Waldz Sep 26, 2018

Author Member

Fixed

@@ -48,6 +50,14 @@ type ServiceProposal struct {
ProviderContacts []Contact `json:"provider_contacts"`
}

// SetProviderContact updates service proposal description with general data
func (proposal *ServiceProposal) SetProviderContact(providerID identity.Identity, providerContact Contact) {
proposal.Format = "service-proposal/v1"

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 25, 2018

Member

Were we talking about moving it into the constant in some other PR?
There are 8 occurrences of this string in the project.


const logPrefix = "[service-openvpn] "

// ServerFactory initiates Openvon server instance during runtime

This comment has been minimized.

Copy link
@soffokl

soffokl Sep 25, 2018

Member

Openvon -> OpenVPN

Show resolved Hide resolved session/create_producer.go

@Waldz Waldz self-assigned this Sep 25, 2018

@Waldz Waldz force-pushed the Waldz:feature/refactor-session-dto branch 5 times, most recently from baa310a to f5f70e7 Sep 25, 2018

@Waldz Waldz changed the title Refactor session dto Dont expose session transport DTOs to external world Sep 26, 2018

@Waldz Waldz force-pushed the Waldz:feature/refactor-session-dto branch from caaebbd to 5d14b91 Sep 26, 2018

@Waldz Waldz removed the work in progress label Sep 26, 2018

@Waldz
Copy link
Member Author

left a comment

All comments fixed

@soffokl
Copy link
Member

left a comment

Looks good to me. Only e2e-test should be fixed.

@Waldz Waldz force-pushed the Waldz:feature/refactor-session-dto branch from 7bcb44a to 9c0eb7d Sep 28, 2018

@Waldz

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2018

Fixed E2E problem

@Waldz Waldz force-pushed the Waldz:feature/refactor-session-dto branch from 9c0eb7d to abd5d11 Sep 29, 2018

Message: fmt.Sprintf("Proposal doesn't exist: %d", request.ProposalId),
}
return
return responWithError(fmt.Sprintf("Proposal doesn't exist: %d", request.ProposalId)), nil

This comment has been minimized.

Copy link
@zolia

zolia Oct 1, 2018

Member

responWithError typo.

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 1, 2018

Author Member

Fixed

@vkuznecovas
Copy link
Contributor

left a comment

Fix the typo @zolia mentioned, and I'm happy

@@ -29,36 +29,42 @@ import (
"github.com/mysteriumnetwork/node/identity"
"github.com/mysteriumnetwork/node/nat"
dto_discovery "github.com/mysteriumnetwork/node/service_discovery/dto"
service_openvpn "github.com/mysteriumnetwork/node/services/openvpn"

This comment has been minimized.

Copy link
@zolia

zolia Oct 1, 2018

Member

lets keep it the same as above 'openvpn_service'

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 1, 2018

Author Member

Fixed

@@ -32,25 +33,31 @@ func (producer *createProducer) GetRequestEndpoint() communication.RequestEndpoi
}

func (producer *createProducer) NewResponse() (responsePtr interface{}) {

This comment has been minimized.

Copy link
@zolia

zolia Oct 1, 2018

Member

We prob don't need it at all. All structure members are public.. and no need to test then too.

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 1, 2018

Author Member

Did not get this comment.
But we will need to simplify and refactor communication endpoints somehow

This comment has been minimized.

Copy link
@zolia

zolia Oct 1, 2018

Member

We could return structure directly, no need for factory.

@zolia
Copy link
Member

left a comment

small issues

Waldz added some commits Sep 21, 2018

Split service.Manager factory to several factory functions
(cherry picked from commit 2ab7d8514048abc2d704105f3e63aa08db6e8a65)

@Waldz Waldz force-pushed the Waldz:feature/refactor-session-dto branch from abd5d11 to f5b0068 Oct 1, 2018

@zolia

zolia approved these changes Oct 1, 2018

@soffokl

soffokl approved these changes Oct 1, 2018

@Waldz Waldz merged commit af2eff2 into mysteriumnetwork:master Oct 1, 2018

1 check passed

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

@Waldz Waldz deleted the Waldz:feature/refactor-session-dto branch Oct 1, 2018

@Waldz Waldz referenced this pull request Oct 1, 2018

Closed

Make Openvpn plugable transport #382

10 of 10 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.