Skip to content

Fix for #670#680

Merged
KostyaSha merged 1 commit intodocker-java:masterfrom
tejksat:fixLoadImageFromTarFails
Aug 28, 2016
Merged

Fix for #670#680
KostyaSha merged 1 commit intodocker-java:masterfrom
tejksat:fixLoadImageFromTarFails

Conversation

@tejksat
Copy link
Contributor

@tejksat tejksat commented Aug 25, 2016 •
edited by marcuslinke
Loading

JSON decoding disabled when response body is not expected to be parsed as a JSON.

This change is Reviewable

@tejksat tejksat mentioned this pull request Aug 25, 2016
Closed
@tejksat
Copy link
Contributor Author

tejksat commented Aug 25, 2016

It could be fixed with this check or it could be a new post() method of InvocationBuilder, f.e. without TypeReference parameter.

KostyaSha
KostyaSha reviewed Aug 25, 2016
View reviewed changes
src/main/java/com/github/dockerjava/netty/InvocationBuilder.java Outdated
channel.pipeline().addLast(new JsonObjectDecoder());
channel.pipeline().addLast(jsonResponseHandler);

if (!Void.class.equals(typeReference.getType())) {
Copy link
Member

@KostyaSha KostyaSha Aug 25, 2016

Choose a reason for hiding this comment

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

maybe instanceof check?

Copy link
Contributor Author

@tejksat tejksat Aug 27, 2016

Choose a reason for hiding this comment

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

I would rather refractor and extract a specific method without JSON mapping. I'll do it in the nearest days.

Copy link
Member

@KostyaSha KostyaSha Aug 27, 2016

Choose a reason for hiding this comment

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

So merge it or better wait?

Copy link
Contributor Author

@tejksat tejksat Aug 27, 2016

Choose a reason for hiding this comment

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

@KostyaSha done! Pls check.

@KostyaSha
Copy link
Member

KostyaSha commented Aug 25, 2016

hello world still fails on first run :E but seems loadimage test fixed.
LGTM

@tejksat tejksat force-pushed the fixLoadImageFromTarFails branch from 3b154a9 to 548b247 Compare August 27, 2016 21:25
@tejksat
Fix for docker-java#670
cc8dbea 
JSON decoding disabled on /images/load command execution.
@tejksat tejksat force-pushed the fixLoadImageFromTarFails branch from 548b247 to cc8dbea Compare August 27, 2016 22:40
@KostyaSha
Copy link
Member

KostyaSha commented Aug 28, 2016 •
edited
Loading

@tejksat mmm... maybe it could be combined with #664 ?
The problem is that for some arguments loadImage may return json and for some plain text.
Now we have only:

 public class LoadImageCmdExec extends AbstrSyncDockerCmdExec<LoadImageCmd, Void> implements
         LoadImageCmd.Exec {

But i tried to make #664 with JsonParsed content and broke current quiet call.

So probably we can offer 2 different callbacks for end-users so they can choose them self what to use for what mix of arguments?

@KostyaSha
Copy link
Member

KostyaSha commented Aug 28, 2016

Merging this as it fixes test for 1.12 engine

@KostyaSha KostyaSha added this to the 3.0.6 milestone Aug 28, 2016
@KostyaSha KostyaSha merged commit c8ab23f into docker-java:master Aug 28, 2016
@tejksat tejksat deleted the fixLoadImageFromTarFails branch September 12, 2016 13:12
panuse pushed a commit to TuKangTech/docker-java that referenced this pull request Aug 20, 2017
@tejksat @KostyaSha
Fix for docker-java#670 (docker-java#680)
64574ba 
JSON decoding disabled on /images/load command execution.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

transport/netty type/bug

Projects

None yet

Milestone

3.0.6

Development

Successfully merging this pull request may close these issues.

2 participants

@tejksat @KostyaSha