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

Add the boolean parameter 'Force' to DockerClient#disconnectFromNetwork and fix issue on exceptions being swalled #839

Merged
merged 2 commits into from
Jul 29, 2017

Conversation

hgontijo
Copy link
Contributor

Add the boolean parameter 'Force' to DockerClient#disconnectFromNetwork at Docker Networks.

Refactored accordingly due to the introduction of the new parameter.

Fix issue at DockerClient#disconnectFromNetwork and DockerClient#connectToNetwork that led to an request error (HTTP 4xx, 5xx) to be swalled and no exception has been thrown.
This was happening since the request was using Response.class and JerseyInvocation#translate would just report the error via Response.class whenever this class is used to carry the response data. If a different class other then Response.class is given, then Jersey will thrown an exception.

…rk as it's defined at [Docker Networks](https://docs.docker.com/engine/api/v1.30/#operation/NetworkDisconnect).

Refactored accordingly due to the introduction of the new parameter.

Fix issue at DockerClient#disconnectFromNetwork and DockerClient#connectToNetwork that led to an request error (HTTP 4xx, 5xx) to be swalled and no exception has been thrown.
This was happening since the request was using Response.class and JerseyInvocation#translate would just report the error via Response.class whenever this class is used to carry the response data. If a different class other then Response.class is given, then Jersey will thrown an exception.
@hgontijo
Copy link
Contributor Author

Here's the JerseyInvocation#translate (jersey-client-2.22.2) code I mentioned above:

    private <T> T translate(final ClientResponse response, final RequestScope scope, final Class<T> responseType)
            throws ProcessingException {
        if (responseType == Response.class) {
            return responseType.cast(new InboundJaxrsResponse(response, scope));
        }

        if (response.getStatusInfo().getFamily() == Response.Status.Family.SUCCESSFUL) {
            try {
                return response.readEntity(responseType);
            } catch (final ProcessingException ex) {
                if (ex.getClass() == ProcessingException.class) {
                    throw new ResponseProcessingException(new InboundJaxrsResponse(response, scope), ex.getCause());
                }
                throw new ResponseProcessingException(new InboundJaxrsResponse(response, scope), ex);
            } catch (final WebApplicationException ex) {
                throw new ResponseProcessingException(new InboundJaxrsResponse(response, scope), ex);
            } catch (final Exception ex) {
                throw new ResponseProcessingException(new InboundJaxrsResponse(response, scope),
                        LocalizationMessages.UNEXPECTED_ERROR_RESPONSE_PROCESSING(), ex);
            }
        } else {
            throw convertToException(new InboundJaxrsResponse(response, scope));
        }
    }

@codecov-io
Copy link

codecov-io commented Jul 25, 2017

Codecov Report

Merging #839 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #839      +/-   ##
============================================
+ Coverage     66.37%   66.39%   +0.01%     
  Complexity      742      742              
============================================
  Files           166      166              
  Lines          3138     3139       +1     
  Branches        357      357              
============================================
+ Hits           2083     2084       +1     
  Misses          898      898              
  Partials        157      157

@hgontijo
Copy link
Contributor Author

Thoughts on this PR?

Copy link
Contributor

@davidxia davidxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

@@ -4062,6 +4062,62 @@ public void testNetworksConnectContainer() throws Exception {
}

@Test
public void testNetworksConnectContainerWithoutForcefulDisconnectNetwork() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can leave out this test as it's almost the same as the one above and we're not testing the difference between forceful and not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to me, I left the test out. Thanks.

}

private void manageNetworkConnection(String containerId, String methodname, String networkId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up.

@davidxia davidxia merged commit 41490ff into spotify:master Jul 29, 2017
@hgontijo hgontijo deleted the add-force-network-disconnect branch August 1, 2017 03:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants