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-143 tequila disconnect #71

Merged
merged 5 commits into from Dec 27, 2017

Conversation

Projects
None yet
4 participants
@shroomist
Copy link
Contributor

commented Dec 26, 2017

No description provided.

@shroomist shroomist requested review from tadovas and Waldz Dec 26, 2017

@@ -100,6 +102,10 @@ func statusNotConnected() ConnectionStatus {
return ConnectionStatus{NotConnected, "", nil}
}

func statusDisconnecting() ConnectionStatus {
return ConnectionStatus{Disconnecting, "", nil}

This comment has been minimized.

Copy link
@donce

donce Dec 27, 2017

Contributor

Why are we using empty strings and not nil in session ids?

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 27, 2017

Member

That's go conventions. But on DTO layer I suggest to have mapping to null in JSON value

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 27, 2017

Member

Json encoder can automatically handle empty strings by not rendering fields at all. It's already done by the way.

assert.JSONEq(
t,
`{
"status" : "Disconnecting"

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 27, 2017

Member

Formatting again :) mixing spaces with tabs perhaps?

connManager *connectionManager
fakeDiscoveryClient *server.ClientFake
fakeOpenVpn *fake_openvpn_client
fakeDialogResumeDisconnect chan int

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 27, 2017

Member

Why can't we move this channel to a field inside fake_dialog struct ?
And define two specials operations on fake_dialog itself ?

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 27, 2017

Author Contributor

because fake_dialog is not accessible from this context and is private to connection manager.

@@ -100,6 +102,10 @@ func statusNotConnected() ConnectionStatus {
return ConnectionStatus{NotConnected, "", nil}
}

func statusDisconnecting() ConnectionStatus {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 27, 2017

Member

Strange to nullify sessions ID, while session still exists

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 27, 2017

Member

Does session have any meaning on disconnecting state?

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 27, 2017

Member

Probably we dont have real case

@Waldz

This comment has been minimized.

Copy link
Member

commented Dec 27, 2017

Looks good to me

@shroomist shroomist merged commit 9c71205 into master Dec 27, 2017

@shroomist shroomist deleted the feature/MYST-143-tequila-disconnect branch Dec 28, 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.