Adding "autoRemove" field for HostConfiguration. #526
Conversation
| final ContainerCreation container = sut.createContainer(config, randomName()); | ||
| final ContainerInfo info = sut.inspectContainer(container.id()); | ||
|
|
||
| assertThat(info.hostConfig().autoRemove(), is(true)); |
There was a problem hiding this comment.
In this test, you are just setting the option on container creation and verifying that it was indeed set correctly. A better test would be to start and stop the container, verifying that it does get removed.
There was a problem hiding this comment.
You are right, the test I wrote was quite naive. Anyway, I improved it a bit and just made the commit.
Thanks for the feedback.
Current coverage is 45.79% (diff: 16.66%)@@ master #526 diff @@
==========================================
Files 131 131
Lines 4294 4300 +6
Methods 0 0
Messages 0 0
Branches 636 636
==========================================
+ Hits 1967 1969 +2
- Misses 2139 2143 +4
Partials 188 188
|
There was a problem hiding this comment.
Thanks @diemol for the contribution!
One small question: it looks to me like 1.25 of the API isn't out yet, so there likely wouldn't be an easy way for us to test this change in Travis until there is a docker release of that API version, or would you know how it could be tested?
|
Hi @mattnworb, To test it locally I just faked the behaviour as if the flag was implemented. But I think this does not make sense in the CI and overall, honestly, I would just wait until it gets released. Right now I have a workaround for that missing feature, so I don't need it urgently, it is more a nice to have. Thanks for the nice work you are doing over there, this docker-client simplified my life a lot in the project I am working, so the least I could do is to five something back 😄 |
| return autoRemove; | ||
| } | ||
|
|
||
| public Builder autoRemove(final Boolean autoRemove) { |
There was a problem hiding this comment.
On second thought I am happy to merge this now, in advance of the 1.25 API being released, but would you mind adding a javadoc comment here that this field only has relevance for versions >= 1.25?
There was a problem hiding this comment.
Sure, I'll do it between today and tomorrow.
|
Ah cool @davidxia, you were faster! Thanks! |
Hi,
Regarding the discussion related to this question I had some weeks ago, here is the PR addressing that field I was asking for and that will be added in the API v.1.25.
I added also a small unit test to check the field.