Implementation of copy archive to/from container commands by vuminhkh · Pull Request #347 · docker-java/docker-java

vuminhkh · 2015-10-17T17:07:52Z

Hello,

I've implemented "Copy file to container command", but it seems that migrating to 1.20 API breaks other part (8 tests failure). The commands that I modified, the test works well.

As I don't have enough knowledge of the project to fix them all, I still perform the pull request, hoping that it will be useful.

Thanks,

Minh Khang VU

Modification of Copy file from container command to follow 1.20 API

marcuslinke · 2015-10-19T07:08:23Z

@vuminhkh Thanks for your contribution. I will merge ASAP.

KostyaSha · 2015-10-19T12:25:08Z

src/main/java/com/github/dockerjava/core/CompressArchiveUtil.java

Place in try-with-resources?

Thank you. It's better to go with new Java 7 functionality. It's cleaner.

KostyaSha · 2015-10-19T12:29:17Z

I see many unclosed streams, @vuminhkh could you check that you have no resource leaks https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html ?

vuminhkh · 2015-10-19T20:20:37Z

Hello,

For streams unclosed, if i'm not mistaken it's about the tar method in CompressArchiveUtil, as it's stream which wrap stream, closing the most outer will delegate to inner and the whole stream chain will be closed.

For formatting rules, i'm using Intellij, please feel free to re-organize based on your IDE settings, or give me some hints to have the same format rules as yours.

Thanks,

Minh Khang VU

* master: Update CHANGELOG.md issue #349

@checkfornull

* master: (21 commits) Update CHANGELOG.md Format sources Update CHANGELOG.md Update CHANGELOG.md Added missing withXXX(List list) methods Update README.md Bump to version 3.0.0-SNAPSHOT according to semver Update CHANGELOG.md Added override for bulk-read variant of InputStream.read() in anonymous inner class created by Dockerfile.ScannedResult.buildDockerFolderTar(). This fixes an IO performance issue that occurs when only the single-byte variant of the read() method on InputStream is overriden. Added @checkfornull annotation Update README.md Update CHANGELOG.md [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release docker-java-2.1.2 Fix issue #348 Small doc fixes Removed import Added additional withX(List<Y> x) methods Added null check annotations and removed withX() methods WIP ... Conflicts: src/main/java/com/github/dockerjava/core/command/CopyFileFromContainerCmdImpl.java

marcuslinke · 2015-11-17T19:39:18Z

@vuminhkh After thinking about again we should stay backward compatible. In fact the CopyFileFromContainerCmd should target to /containers/{id}/copy and then
a new CopyArchiveFromContainerCmd as well as a CopyArchiveToContainerCmd should target to /containers/{id}/archive. Would it be possible for you to change your PR accordingly please?

vuminhkh · 2015-11-21T14:09:38Z

Hi Marcus,

I'll find my-self some times to do this asap.

Minh Khang VU

* master: Fix DockerClientConfig serialization. Move findbugs into profile. Add checkstyle with unused imports check Move utils into util package Remove unused imports Make util contstructors private Move exceptions into separate package Conflicts: src/main/java/com/github/dockerjava/core/command/CopyFileFromContainerCmdImpl.java src/main/java/com/github/dockerjava/jaxrs/DockerCmdExecFactoryImpl.java

…hiveFromContainerCmd and CopyArchiveToContainerCmd

vuminhkh · 2015-11-23T23:39:07Z

Hi Marcus,

Please review to see if it's ok. There are always 8 test failures (the same as before) but they don't concern the commands that I added/modified.

Regards,

Minh Khang VU
TestSuite.txt

KostyaSha · 2015-11-24T09:33:40Z

src/main/java/com/github/dockerjava/api/DockerClient.java

please add javadoc and mention from what docker-api version it works.

KostyaSha · 2015-11-24T09:36:50Z

@vuminhkh please exclude all unrelated to your change formatting and import changes

@checkfornull

* master: Fix potential NullpointerExceptions Fix changelog Renamed networStats to network Remove "Stats" from "networks" field in statistics Improve JavaDoc and include @checkfornull Deprecate "network" and enable "networks" stats (remote Docker API 1.21)

vuminhkh · 2015-11-27T21:56:54Z

@KostyaSha I tried to use your formatting rules at docker-java/etc/code-style.epf and eclipse plugin for intellij, it seems that i's not your current formatting rules. So I made all the changes "manually", what do you advice about formatting rules in order to contribute to your project ?

marcuslinke · 2015-12-02T06:25:40Z

@vuminhkh Thanks! Hopefully I'm able to merge this evening...

marcuslinke · 2015-12-02T20:37:20Z

@vuminhkh After looking deeper into this I've just found that the API of CopyArchiveToContainerCmd prevents from copying an already compressed tar archive to the container. This is useful if someone wants to copy an archive from one container to another one.

The command should be able to transport a InputStream to the CopyArchiveToContainerCmdExec just like the BuildImageCmd does. The tar compression must be done in the command and not in the exec class then. Would it be possible for you to adopt your PR accordingly please?

Beside this, the NullpointerExceptions that you got are caused by #388

KostyaSha · 2015-12-02T22:23:53Z

src/main/java/com/github/dockerjava/api/DockerClient.java

@deprecated javadoc

KostyaSha · 2015-12-02T22:25:54Z

src/test/java/com/github/dockerjava/core/command/CopyFileFromContainerCmdImplTest.java

unrelated change

@nonnull

* master: Added @nonnull annotations Pass mandatory container id into stats command constructor Update CHANGELOG.md Remove public from members as getters are public already Allow legacy registry message to indicate pull success Allow legacy registry message to indicate pull success Make ProgressDetails attributes public Update CHANGELOG.md Update CHANGELOG.md Add equals and hashCode for State Add test case Add missing properties for InspectContainerResponse.Status Add docker versions for easier javadoc reference Conflicts: src/main/java/com/github/dockerjava/api/DockerClient.java

vuminhkh · 2015-12-03T22:24:30Z

@marcuslinke I agree with you, It's not much modification but the API is more generic. I updated the API according to your suggestion.
@KostyaSha API doc and deprecated annotation updated to Java standard. For indentation issues, I insist that you commit your formatting rules and I'll use it to reformat files that I modified.

Thanks,

Minh Khang VU

KostyaSha · 2015-12-03T22:35:36Z

@vuminhkh i slowly adding checkstyle that matches to what marcus did. The standard rule for projects that has not yet defined styles is not mix styles and follow existed. And exclude unrelated to change changes.

KostyaSha · 2015-12-03T22:42:32Z

src/main/java/com/github/dockerjava/api/DockerClient.java

(Not critical, but for simplification there are API versions for linking javadocs. Example

docker-java/src/main/java/com/github/dockerjava/api/command/InspectContainerResponse.java

Line 234 in 58d9407

* @since {@link RemoteApiVersion#VERSION_1_20}

)

Try change link to version here also

vuminhkh · 2015-12-05T18:23:16Z

@KostyaSha updated javadoc following your hint.
@marcuslinke and @KostyaSha I began to implement new network API, you can preview at https://github.com/vuminhkh/docker-java/tree/network . I'll perform another pull request once this one merged.

Thanks,

Minh Khang VU

KostyaSha · 2015-12-05T18:26:47Z

@vuminhkh implement classes is pretty simple, problem is wrote tests and verify this changes.

vuminhkh · 2015-12-05T18:42:53Z

@KostyaSha For commands that I implemented, if i'm not mistaken, I've written tests for them all. Normally what I wrote did not break any existing feature, tests that are broken were APIs which are not compatible anymore with newer Docker API. I have not fixed those tests yet, I'll do it once I have more time. If you guy could help me also on this it will be great as I don't have knowledge about those existing command.

vuminhkh · 2015-12-05T18:45:11Z

@marcuslinke @KostyaSha For breaking changes in docker API, what will be your strategy to be compatible with multiple docker api version ?

KostyaSha · 2015-12-05T18:49:22Z

Depends on cases.

2015-12-05 21:45 GMT+03:00 vuminhkh notifications@github.com:

@marcuslinke https://github.com/marcuslinke @KostyaSha
https://github.com/KostyaSha For breaking changes in docker API, what
will be your strategy to be compatible with multiple docker api version ?

—
Reply to this email directly or view it on GitHub
#347 (comment)
.

vuminhkh · 2015-12-05T19:17:13Z

@KostyaSha What is the configuration of your test ? Which version of docker do you target to assure that there's not regression ? If i'm not mistaken it's here https://github.com/docker-java/docker-java/blob/master/src/test/java/com/github/dockerjava/client/AbstractDockerClientTest.java#L41 that you define which version of API the test target. Is it ok if I change it to 1.21 ?

KostyaSha · 2015-12-06T22:19:20Z

I'm bit newbie with current tests design. @marcuslinke please suggest.

marcuslinke · 2015-12-07T19:26:07Z

@vuminhkh, @KostyaSha Currently we can only test against one specific version of the remote API. This is also the reason for the compatibility statement here: https://github.com/docker-java/docker-java#docker-java

As you already found this is hard-coded in the AbstractDockerClientTest class. I will push some changes related to issue #388 soon that will change the tested API version.

marcuslinke · 2015-12-07T21:43:32Z

After merging #392 I will take a look at your latest changes @vuminhkh.

marcuslinke · 2015-12-08T21:13:41Z

@vuminhkh Thanks for contributing!

Implementations of Copy file to container command

cae2924

Modification of Copy file from container command to follow 1.20 API

KostyaSha reviewed Oct 19, 2015
View reviewed changes

Use new try syntax for Java to free resources

89bb406

vuminhkh added 3 commits October 30, 2015 18:12

Use java 7 to close stream

90f4aba

Merge branch 'master' into master-vuminhkh

28c6f94

* master: Update CHANGELOG.md issue #349

marcuslinke added the API_1.20 label Nov 10, 2015

vuminhkh added 3 commits November 23, 2015 23:22

Rename CopyFileFromContainerCmd and CopyFileToContainerCmd to CopyArc…

f44ee8f

…hiveFromContainerCmd and CopyArchiveToContainerCmd

Reset CopyFileFromContainerCmd for compatibility purpose

6124775

KostyaSha reviewed Nov 24, 2015
View reviewed changes

vuminhkh added 2 commits November 27, 2015 22:52

Fix formatting rules

0f65469

KostyaSha reviewed Dec 2, 2015
View reviewed changes

src/main/java/com/github/dockerjava/api/DockerClient.java Outdated

Copy link
Copy Markdown

Member

KostyaSha Dec 2, 2015

Choose a reason for hiding this comment

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

@deprecated javadoc

KostyaSha reviewed Dec 2, 2015
View reviewed changes

src/test/java/com/github/dockerjava/core/command/CopyFileFromContainerCmdImplTest.java Outdated

Copy link
Copy Markdown

Member

KostyaSha Dec 2, 2015

Choose a reason for hiding this comment

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

unrelated change

vuminhkh added 2 commits December 3, 2015 21:50

Re-design API to accept both a path and a tar input stream

fca2ea5

Minor fix API doc

0871b9d

KostyaSha reviewed Dec 3, 2015
View reviewed changes

vuminhkh added 2 commits December 5, 2015 19:15

Rollback unrelated changes due to code formatter

a72355f

Use remote api version constant

477f4a1

marcuslinke merged commit 477f4a1 into docker-java:master Dec 8, 2015

marcuslinke changed the title ~~Implementations of Copy file to container command~~ Implementation of copy archive to/from container commands Dec 8, 2015

marcuslinke mentioned this pull request Dec 17, 2015

Updates to Remote API 1.20 copy operations #292

Closed

Conversation

vuminhkh commented Oct 17, 2015

Uh oh!

marcuslinke commented Oct 19, 2015

Uh oh!

KostyaSha Oct 19, 2015

Choose a reason for hiding this comment

Uh oh!

vuminhkh Oct 19, 2015

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Oct 19, 2015

Uh oh!

vuminhkh commented Oct 19, 2015

Uh oh!

marcuslinke commented Nov 17, 2015

Uh oh!

vuminhkh commented Nov 21, 2015

Uh oh!

vuminhkh commented Nov 23, 2015

Uh oh!

KostyaSha Nov 24, 2015

Choose a reason for hiding this comment

Uh oh!

KostyaSha commented Nov 24, 2015

Uh oh!

vuminhkh commented Nov 27, 2015

Uh oh!

marcuslinke commented Dec 2, 2015

Uh oh!

marcuslinke commented Dec 2, 2015

Uh oh!

KostyaSha Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

KostyaSha Dec 2, 2015

Choose a reason for hiding this comment

Uh oh!

vuminhkh commented Dec 3, 2015

Uh oh!

KostyaSha commented Dec 3, 2015

Uh oh!

KostyaSha Dec 3, 2015

Choose a reason for hiding this comment

Uh oh!

KostyaSha Dec 5, 2015

Choose a reason for hiding this comment

Uh oh!

vuminhkh commented Dec 5, 2015

Uh oh!

KostyaSha commented Dec 5, 2015

Uh oh!

vuminhkh commented Dec 5, 2015

Uh oh!

vuminhkh commented Dec 5, 2015

Uh oh!

KostyaSha commented Dec 5, 2015

Uh oh!

vuminhkh commented Dec 5, 2015

Uh oh!

KostyaSha commented Dec 6, 2015

Uh oh!

marcuslinke commented Dec 7, 2015

Uh oh!

marcuslinke commented Dec 7, 2015

Uh oh!

marcuslinke commented Dec 8, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants