Support remote API 1.22 subset by KostyaSha · Pull Request #488 · docker-java/docker-java

KostyaSha · 2016-03-01T22:18:59Z

squashed and rordered variant of #469

KostyaSha · 2016-03-01T22:26:26Z

Merged completion fix and rebased.

codecov-io · 2016-03-01T22:50:23Z

Current coverage is `21.61%`

Merging #488 into master will increase coverage by +2.60% as of 40b0317

@@            master    #488   diff @@
======================================
  Files          281     290     +9
  Stmts         5563    5944   +381
  Branches       525     527     +2
  Methods          0       0       
======================================
+ Hit           1058    1285   +227
- Partial         82      83     +1
- Missed        4423    4576   +153

Review entire Coverage Diff as of 40b0317

Powered by Codecov. Updated on successful CI builds.

KostyaSha · 2016-03-02T07:38:14Z

@marcuslinke please check your comments on this pr.

marcuslinke · 2016-03-02T07:41:06Z

@KostyaSha Thanks!

marcuslinke · 2016-03-02T20:28:33Z

Tested successfully against 1.22.

Tested against 1.21:

Failed tests: 
  InspectExecCmdImplTest.inspectExecNetworkSettings:122 
Expected: is null
     but: was <0>
  StartContainerCmdImplTest.startContainerWithVolumes:97 
Expected: </opt/webapp1>
     but: was </opt/webapp2>
  UpdateContainerCmdImplTest.updateContainer:67 ? NotFound 404 page not found

  InspectExecCmdExecTest.inspectExecNetworkSettings:123 
Expected: is null
     but: was <0>
  UpdateContainerCmdExecTest.updateContainer ? NotFound 404 page not found

marcuslinke · 2016-03-02T20:35:45Z

src/main/java/com/github/dockerjava/api/command/UpdateContainerCmd.java

+
+    public UpdateContainerCmd withBlkioWeight(Integer blkioWeight);
+
+    public UpdateContainerCmd withContainerId(String containerId);


It would be better to deny null here (by adding a non-null check in impl and annotate with @nonnull). It doesn't make sense to pass null. What should be done when container id is null in exec?

as low level api just send to null container id (btw, can container be reached via null name?). Buy yeah, while it ~CLI interface for library users, then makes sense.

marcuslinke · 2016-03-02T20:43:11Z

@KostyaSha Beside the comments above it looks good to me. What about #484 from @fbuecklers now?

KostyaSha · 2016-03-02T21:35:06Z

@marcuslinke this pr is part 1 and everything should be updated later. His pr has no tests and need to be reviewed/updated/etc

KostyaSha · 2016-03-02T22:16:30Z

  StartContainerCmdImplTest.startContainerWithVolumes:97 
Expected: </opt/webapp1>
     but: was </opt/webapp2>

Can't reproduce.

marcuslinke · 2016-03-02T22:25:05Z

StartContainerCmdImplTest.startContainerWithVolumes:97 
Expected: </opt/webapp1>
     but: was </opt/webapp2>

Can't reproduce from IDE too. So it seems unrelated to 1.21. Maybe returned order is unpredictable sometimes... I think we can ignore this for now.

marcuslinke · 2016-03-02T22:43:10Z

Reviewed 59 of 59 files at r1, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

KostyaSha · 2016-03-02T22:48:13Z

My results against 1.9.1

Results :

Failed tests:
  AttachContainerCmdImplTest.attachContainerWithTTY:108
Expected: a string containing "stdout\r\nstderr"
     but: was ""
  AuthCmdImplTest.testAuth:42 » InternalServerError Registration: "Wrong email f...
  BuildImageCmdImplTest.testBuildFromPrivateRegistry:299 » DockerClient Could no...
  PushImageCmdImplTest.pushLatest:63 » DockerClient Could not push image: unauth...
  AuthCmdExecTest.testAuth » InternalServerError Registration: "Wrong email form...
  AuthCmdExecTest.testAuthInvalid:51 » InternalServerError Registration: "Wrong ...
  BuildImageCmdExecTest.testBuildFromPrivateRegistry:291 » DockerClient Could no...
  InspectExecCmdExecTest.inspectExecNetworkSettings:123
Expected: is null
     but: was <0>
  PushImageCmdExecTest.pushLatest:65 » DockerClient Could not push image: unauth...
  StartContainerCmdExecTest.startContainerWithVolumes:98
Expected: </opt/webapp1>
     but: was </opt/webapp2>

KostyaSha · 2016-03-02T22:52:29Z

InspectExecCmdExecTest.inspectExecNetworkSettings:123 fixed

KostyaSha · 2016-03-02T22:53:57Z

 StartContainerCmdExecTest.startContainerWithVolumes:98
Expected: </opt/webapp1>
     but: was </opt/webapp2>

fails only when you run class test (methods are in parallel?)

marcuslinke · 2016-03-02T22:57:22Z

Probably somethink like that. But how to fix test then?

Comments from the review on Reviewable.io

KostyaSha · 2016-03-02T23:00:00Z

Mount has no Builders, so it impossible to fill it and test with contains()

KostyaSha · 2016-03-02T23:10:27Z

Seems fixed.

KostyaSha · 2016-03-02T23:11:45Z

😴

KostyaSha · 2016-03-02T23:15:32Z

running 1.9.1 test locally again.
PS what i need specify for push tests?

KostyaSha · 2016-03-02T23:20:27Z

Maybe withTTY test fails also because of parallel... will check tomorrow.

KostyaSha · 2016-03-03T05:37:37Z

only

Failed tests:
  AttachContainerCmdImplTest.attachContainerWithTTY:108
Expected: a string containing "stdout\r\nstderr"
     but: was ""

marcuslinke · 2016-03-03T07:05:42Z

Strange... For me this test passes successfully each time.

marcuslinke · 2016-03-03T08:05:39Z

Btw. for runnning push tests you need a registry (personally I use docker hub) and configure registry and credentials in .docker-java.properties

KostyaSha · 2016-03-03T10:51:08Z

@marcuslinke try run test for the whole class.

KostyaSha · 2016-03-03T12:01:16Z

@marcuslinke if it passes for you then ok, i can't pick it under debugger. Are changes good now?

KostyaSha · 2016-03-03T12:05:06Z

@marcuslinke https://github.com/KostyaSha/dind/tree/fedora/fedora:1.9
./build.sh
docker run -d -e PORT=4243 -p 4444:4243 --privileged dind_fedora:1.9
DOCKER_HOST=tcp://192.168.99.100:4444

marcuslinke · 2016-03-03T21:33:39Z

@KostyaSha AttachContainerCmdImplTest.attachContainerWithTTY is not failing for me even when running all tests from maven. Now I get only:

Tests run: 275, Failures: 2, Errors: 0, Skipped: 2, Time elapsed: 1,007.209 sec <<< FAILURE! - in TestSuite
testBuildFromPrivateRegistry(com.github.dockerjava.core.command.BuildImageCmdImplTest)  Time elapsed: 4.698 sec  <<< FAILURE!
com.github.dockerjava.api.exception.DockerClientException: Could not build image: Error: image testuser/busybox:latest not found
    at com.github.dockerjava.core.command.BuildImageCmdImplTest.testBuildFromPrivateRegistry(BuildImageCmdImplTest.java:298)

testBuildFromPrivateRegistry(com.github.dockerjava.netty.exec.BuildImageCmdExecTest)  Time elapsed: 4.29 sec  <<< FAILURE!
com.github.dockerjava.api.exception.DockerClientException: Could not build image: Error: image testuser/busybox:latest not found
    at com.github.dockerjava.netty.exec.BuildImageCmdExecTest.testBuildFromPrivateRegistry(BuildImageCmdExecTest.java:290)

The strange thing about it is that it seems related to not setting the API version anymore at AbstractDockerClientTest. So when setting the version matching the docker deamon than the test passes. When removing explicit version the test fails. This counts for both docker versions...

KostyaSha · 2016-03-04T17:48:47Z

Total Tests: 486 (±0)
Failed Tests: 5 (+1)
com.github.dockerjava.netty.exec.ExecStartCmdExecTest.execStartAttachStdin
com.github.dockerjava.netty.exec.ExecStartCmdExecTest.execStartAttachStdinToShell
com.github.dockerjava.core.command.EventsCmdImplTest.testEventStreamTimeBound
com.github.dockerjava.core.command.AttachContainerCmdImplTest.attachContainerWithTTY
com.github.dockerjava.netty.exec.AttachContainerCmdExecTest.attachContainerWithStdin

against 1.9 dind from maven build, but AttachContainerCmdExecTest runs successfully in IDEA. Maybe timeouts?

KostyaSha · 2016-03-04T17:54:01Z

For tty test i'm getting com.github.dockerjava.api.exception.InternalServerErrorException: http: Hijack is incompatible with use of CloseNotifier

KostyaSha · 2016-03-04T17:56:56Z

Maybe spotify/docker-client#258

KostyaSha · 2016-03-04T18:03:46Z

codenvy-legacy/che-plugins@8bd0366

KostyaSha · 2016-03-04T19:06:05Z

1.9.1

Failed Tests: 3 (-2)
com.github.dockerjava.netty.exec.ExecStartCmdExecTest.execStartAttachStdin
Exception java.lang.AssertionError

Message: expected:<STDIN > but was:<>

Stacktrace:

at org.testng.Assert.fail(Assert.java:89)
at org.testng.Assert.failNotEquals(Assert.java:489)
at org.testng.Assert.assertEquals(Assert.java:118)
at org.testng.Assert.assertEquals(Assert.java:171)
at org.testng.Assert.assertEquals(Assert.java:181)
at com.github.dockerjava.netty.exec.ExecStartCmdExecTest.execStartAttachStdin(ExecStartCmdExecTest.java:123)


com.github.dockerjava.netty.exec.ExecStartCmdExecTest.execStartAttachStdinToShell
Exception java.lang.AssertionError

Message: Expected: a string containing "etc\n" but: was ""

Stacktrace:


at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
at com.github.dockerjava.netty.exec.ExecStartCmdExecTest.execStartAttachStdinToShell(ExecStartCmdExecTest.java:156)

com.github.dockerjava.core.command.AttachContainerCmdImplTest.attachContainerWithTTY
Message: Expected: a string containing "stdout\r\nstderr" but: was ""

Stacktrace:


at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
at com.github.dockerjava.core.command.AttachContainerCmdImplTest.attachContainerWithTTY(AttachContainerCmdImplTest.java:108)

KostyaSha · 2016-03-04T19:06:58Z

1.10.2

Failed Tests: 4 (+4)
com.github.dockerjava.netty.exec.EventsCmdExecTest.testEventStreaming1
Exception java.lang.AssertionError

Message: Received only: [] expected:<true> but was:<false>

Stacktrace:

at org.testng.Assert.fail(Assert.java:89)
at org.testng.Assert.failNotEquals(Assert.java:489)
at org.testng.Assert.assertTrue(Assert.java:37)
at com.github.dockerjava.netty.exec.EventsCmdExecTest.testEventStreaming1(EventsCmdExecTest.java:90)

com.github.dockerjava.netty.exec.ExecStartCmdExecTest.execStartAttachStdin
com.github.dockerjava.netty.exec.ExecStartCmdExecTest.execStartAttachStdinToShell
com.github.dockerjava.core.command.AttachContainerCmdImplTest.attachContainerWithTTY

KostyaSha · 2016-03-04T19:07:41Z

To anybody, any testing help is welcome!

KostyaSha · 2016-03-04T19:46:47Z

com.github.dockerjava.netty.exec.EventsCmdExecTest.testEventStreaming2 fails also from IDE after long delay.

marcuslinke · 2016-03-04T19:57:51Z

@KostyaSha All tests passed successfully here. Both for 1.21 and 1.22 also. Do you run the tests via DinD?

KostyaSha · 2016-03-04T19:58:33Z

I run via DinD and via http without TLS (but the same errors i got on non-dind instance).

marcuslinke · 2016-03-04T19:59:15Z

So maybe the issues you have are related to that somehow?

KostyaSha · 2016-03-04T20:00:02Z

I had the same issues on native daemon in docker-machine (boot2docker). I may guess that issue in http vs unix connection.

marcuslinke · 2016-03-04T20:01:44Z

I use docker-machine/boot2docker with TLS here and it works.

KostyaSha · 2016-03-04T20:05:03Z

Then merge?

marcuslinke · 2016-03-04T20:07:03Z

👍

1.22 cleanup

marcuslinke · 2016-03-04T20:09:54Z

So we modify CHANGELOG.md to say that 3.0.0 will support 1.22 now?

marcuslinke · 2016-03-04T20:14:41Z

@KostyaSha ...and maybe release 3.0.0-RC3?

KostyaSha · 2016-03-04T20:17:13Z

🏧 1.22 is partially supported, we need review contributed PR and update it with tests. We should try somehow avoid delays (one evening i'm proposing change, second you review, third i fix and loop) :)

KostyaSha · 2016-03-04T20:19:48Z

@marcuslinke sure, do rc3 so people will have ability to try changes in more gradient way.

marcuslinke · 2016-03-04T20:27:20Z

@KostyaSha I fear it will be difficult to speed up development. That would need more actively maintainers. For me personally its impossible to spend more time on this. Oh and I'm away next week for holidays by the way...

KostyaSha · 2016-03-04T20:28:22Z

I hope we will end refactorings and clean-ups and then it will take less time for supporting/updating it.

KostyaSha force-pushed the 1.22-cleanup branch from cc25cce to bd1737b Compare March 1, 2016 22:26

marcuslinke reviewed Mar 2, 2016
View reviewed changes

KostyaSha self-assigned this Mar 3, 2016

KostyaSha added this to the 3.0.0 milestone Mar 3, 2016

KostyaSha force-pushed the 1.22-cleanup branch from d9eb783 to 241257c Compare March 4, 2016 16:22

marcuslinke added a commit that referenced this pull request Mar 4, 2016

Merge pull request #488 from KostyaSha/1.22-cleanup

49304d8

1.22 cleanup

marcuslinke merged commit 49304d8 into docker-java:master Mar 4, 2016

KostyaSha added API/1.21 labels Mar 4, 2016

KostyaSha mentioned this pull request Mar 4, 2016

upd to 1.22 API update part 1 #469

Closed

marcuslinke changed the title ~~1.22 cleanup~~ Support remote API 1.22 subset Mar 4, 2016

KostyaSha mentioned this pull request Mar 20, 2016

docker-java doesn't work 'out of the box' with v2 registries #506

Closed

KostyaSha mentioned this pull request Apr 4, 2016

Thread-safety and pooling #536

Closed

2 tasks


		public UpdateContainerCmd withBlkioWeight(Integer blkioWeight);

		public UpdateContainerCmd withContainerId(String containerId);

Conversation

KostyaSha commented Mar 1, 2016

Uh oh!

KostyaSha commented Mar 1, 2016

Uh oh!

codecov-io commented Mar 1, 2016

Current coverage is 21.61%

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

marcuslinke commented Mar 2, 2016

Uh oh!

marcuslinke commented Mar 2, 2016

Uh oh!

marcuslinke Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

KostyaSha Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

KostyaSha Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

marcuslinke commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

marcuslinke commented Mar 2, 2016

Uh oh!

marcuslinke commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

marcuslinke commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 2, 2016

Uh oh!

KostyaSha commented Mar 3, 2016

Uh oh!

marcuslinke commented Mar 3, 2016

Uh oh!

marcuslinke commented Mar 3, 2016

Uh oh!

KostyaSha commented Mar 3, 2016

Uh oh!

KostyaSha commented Mar 3, 2016

Uh oh!

KostyaSha commented Mar 3, 2016

Uh oh!

marcuslinke commented Mar 3, 2016

Uh oh!

KostyaSha commented Mar 4, 2016

Uh oh!

KostyaSha commented Mar 4, 2016

Uh oh!

KostyaSha commented Mar 4, 2016

Uh oh!

KostyaSha commented Mar 4, 2016

Uh oh!

KostyaSha commented Mar 4, 2016

Uh oh!

KostyaSha commented Mar 4, 2016

Uh oh!

KostyaSha commented Mar 4, 2016

Uh oh!

KostyaSha commented Mar 4, 2016

Current coverage is `21.61%`