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

Dummy connection which does Noop tunnel #446

Merged
merged 10 commits into from Oct 24, 2018

Conversation

Projects
None yet
5 participants
@Waldz
Copy link
Member

commented Oct 14, 2018

Closes #480
Updates #382

@Waldz Waldz requested review from tadovas, soffokl, zolia and vkuznecovas Oct 14, 2018

@Waldz Waldz changed the title Dummy transport which does Noop encryption Dummy modular transport which does Noop encryption Oct 14, 2018

@Waldz Waldz force-pushed the abstract-connection branch from 369aa37 to 287f010 Oct 14, 2018

@Waldz Waldz force-pushed the feature/noop-connection branch from a687995 to b227287 Oct 14, 2018

@soffokl
Copy link
Member

left a comment

Looks good to me.

@vkuznecovas vkuznecovas force-pushed the abstract-connection branch from 287f010 to 0439ff3 Oct 15, 2018

@vkuznecovas

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

misslick

@vkuznecovas vkuznecovas reopened this Oct 15, 2018

@Waldz Waldz changed the base branch from abstract-connection to master Oct 15, 2018

return cf.location, cf.err
}

var _ connection.ConnectionCreator = &ProcessBasedConnectionFactory{}

This comment has been minimized.

Copy link
@zolia

zolia Oct 17, 2018

Member

why we need this one?

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 23, 2018

Author Member

It's compilation time check if ProcessBasedConnectionFactory complies interface ConnectionCreator

@zolia
Copy link
Member

left a comment

lgtm

@Waldz Waldz dismissed stale reviews from zolia and soffokl via 8b087a5 Oct 21, 2018

@Waldz Waldz force-pushed the feature/noop-connection branch from b227287 to 8b087a5 Oct 21, 2018

Waldz added some commits Oct 12, 2018

@Waldz Waldz force-pushed the feature/noop-connection branch from 8b087a5 to 320f4a4 Oct 23, 2018

@@ -122,6 +129,11 @@ func (manager *connectionManager) startConnection(consumerID, providerID identit
return err
}

connectionCreator, exists := (*manager.connectionCreators)[proposal.ServiceType]

This comment has been minimized.

Copy link
@tadovas

tadovas Oct 23, 2018

Member

Maybe an interface like ConnectionFactoryRegistry can be more clear instead of using a map directly?
I. E. Registry.CreateConnection(proposal) -> connection,err. this way all the details like concrete factory lookup can be hidden. From other side - different connection plugins can register themselves too without direct map asignation

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 24, 2018

Author Member

Improved by suggestion

@Waldz Waldz requested review from soffokl, vkuznecovas and tadovas Oct 23, 2018

@zolia
Copy link
Member

left a comment

lgtm

Waldz added some commits Oct 12, 2018

@Waldz Waldz force-pushed the feature/noop-connection branch from ae12719 to 5bed552 Oct 24, 2018

@tadovas
Copy link
Member

left a comment

LGTM

Waldz added some commits Oct 23, 2018

@Waldz Waldz dismissed stale reviews from soffokl and tadovas via 8cce982 Oct 24, 2018

@Waldz Waldz force-pushed the feature/noop-connection branch from 5bed552 to 8cce982 Oct 24, 2018

Waldz added some commits Oct 23, 2018

@Waldz Waldz force-pushed the feature/noop-connection branch from 8cce982 to 34057d0 Oct 24, 2018

@Waldz Waldz changed the title Dummy modular transport which does Noop encryption Dummy connection which does Noop encryption Oct 24, 2018

@Waldz Waldz requested review from soffokl, tadovas and zolia Oct 24, 2018

@Waldz Waldz added this to the Keliukis (0.5) milestone Oct 24, 2018


// CreateConnection create plugable connection
func (registry *Registry) CreateConnection(options ConnectOptions, stateChannel StateChannel) (Connection, error) {
creator, exists := registry.creators[options.Proposal.ServiceType]

This comment has been minimized.

Copy link
@zolia

zolia Oct 24, 2018

Member

Most likely we need to distinguish between OpenVPN UDP and OpenVPN TCP. If that would be only OpenVPN service type, then it would be ambiguous ConnectionCreator. Maybe we could have not OpenVPN service type, but OpenvpnTCP and OpenvpnUDP service types?

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 24, 2018

Author Member

Proposal defines service and it's details (protocol, speed, price, contact, etc.)
Consumer is interested if any of details change, not only udp

@tadovas
Copy link
Member

left a comment

reLGTMed

@zolia

zolia approved these changes Oct 24, 2018

Copy link
Member

left a comment

my comment can be addressed later on

@Waldz Waldz merged commit 4fc6c84 into master Oct 24, 2018

2 checks passed

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

@Waldz Waldz changed the title Dummy connection which does Noop encryption Dummy connection which does Noop tunnel Oct 24, 2018

@Waldz Waldz deleted the feature/noop-connection branch Oct 24, 2018

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.