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-498 Cancel connection returns 499 #222

Merged
merged 4 commits into from Apr 11, 2018

Conversation

Projects
None yet
4 participants
@donce
Copy link
Contributor

commented Apr 11, 2018

No description provided.

@donce donce requested review from ignasbernotas, tadovas and zolia Apr 11, 2018

@@ -62,9 +62,17 @@ func (manager *connectionManager) Connect(consumerID, providerID identity.Identi
}
}()

err = manager.startConnection(consumerID, providerID)
if err == utils.ErrRequestCancelled {
return ErrConnectionCancelled

This comment has been minimized.

Copy link
@tadovas

tadovas Apr 11, 2018

Member

Do we really need separate error in manager if utils.ErrRequestCancelled is the only error which indicates cancelation ?

This comment has been minimized.

Copy link
@donce

donce Apr 11, 2018

Author Contributor

Yes - we specify more concrete custom error message, which we use in our api response body.

This comment has been minimized.

Copy link
@donce

donce Apr 11, 2018

Author Contributor

I was wondering, maybe this mapping can be done in api level - if manager.Connect returns ErrRequestCancelled, then we return custom api response.

This comment has been minimized.

Copy link
@donce

donce Apr 11, 2018

Author Contributor

@tadovas pushed new commit of what I've meant.

@tadovas
Copy link
Member

left a comment

LGTM

@zolia

zolia approved these changes Apr 11, 2018

@donce donce merged commit cabd5b5 into master Apr 11, 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

@donce donce deleted the fix/MYST-498-cancel-connection-returns-499 branch Apr 11, 2018

@@ -20,8 +20,6 @@ var (
ErrNoConnection = errors.New("no connection exists")
// ErrAlreadyExists error indicates that aciton applieto to manager expects no active connection (i.e. connect)
ErrAlreadyExists = errors.New("connection already exists")
// ErrConnectionCancelled indicates that connection in progress was cancelled by request of api user
ErrConnectionCancelled = errors.New("connection was cancelled")

This comment has been minimized.

Copy link
@Waldz

Waldz Apr 11, 2018

Member

What was wrong with more concrete error?

@@ -214,7 +216,7 @@ func (manager *connectionManager) waitForConnectedState(stateChannel <-chan open
manager.onStateChanged(state, sessionID)
}
case <-cancelRequest:
return ErrConnectionCancelled
return utils.ErrRequestCancelled

This comment has been minimized.

Copy link
@Waldz

Waldz Apr 11, 2018

Member

Dont like that internally used library (cancellable) is exposed to external world

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.