Skip to content
This repository has been archived by the owner on Mar 18, 2024. It is now read-only.

Add typed error on connection failure #194

Merged
merged 1 commit into from
Dec 7, 2015

Conversation

abronan
Copy link
Collaborator

@abronan abronan commented Dec 5, 2015

Add typed error on connection failure. This is convenient to differentiate between legitimate container operation failures and a connection failure to mark a Swarm node as Healthy or Unhealthy.

/cc @vieux @dongluochen

Signed-off-by: Alexandre Beslic abronan@docker.com

@dongluochen
Copy link
Contributor

LGTM.

@vieux
Copy link
Contributor

vieux commented Dec 7, 2015

The if is different now. It's a change of behaviour, not sur we want that.

  • With a connection error and no TLS -> no more "Are you trying to connect to a TLS-enabled daemon without TLS"
  • With an error different than connection error and no TLS -> "Are you trying to connect to a TLS-enabled daemon without TLS"

@abronan
Copy link
Collaborator Author

abronan commented Dec 7, 2015

Good point, this breaks the error flow for the TLS error. Let me change that.

Signed-off-by: Alexandre Beslic <abronan@docker.com>
@abronan abronan force-pushed the add_connection_refused_error branch from 48637cc to 99bbdcd Compare December 7, 2015 19:03
@abronan
Copy link
Collaborator Author

abronan commented Dec 7, 2015

@vieux PTAL

@vieux
Copy link
Contributor

vieux commented Dec 7, 2015

@abronan so you will the the new error only when you use TLS, is it ok ?

@abronan
Copy link
Collaborator Author

abronan commented Dec 7, 2015

@vieux I think this way we don't break the current behavior (which is also disputable as we throw a TLS related message on something that might be completely unrelated, ie. remote endpoint unprotected but we throw the TLS message anyway because it is not a connection issue).

I added the test right after this one to not break existing clients and change the behavior, but ultimately we should only check for a connection issue here and not try to guess a TLS related error (I mimic fsouza/dockerclient by doing this 😄).

I might be missing something though, is there a better alternative?

@vieux
Copy link
Contributor

vieux commented Dec 7, 2015

I don't see anything better, I'm just not sure this will be helpful

@vieux
Copy link
Contributor

vieux commented Dec 7, 2015

LGTM

vieux added a commit that referenced this pull request Dec 7, 2015
@vieux vieux merged commit dcacc6c into samalba:master Dec 7, 2015
@vieux vieux deleted the add_connection_refused_error branch December 7, 2015 20:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants