revert HostConfig.Builder.binds() behavior that appended to list

Partially revert behavior introduced in 1f4d0c7 where the
`HostConfig.Builder.binds(..)` method appends to the existing list
instead of replaces it.

We found a bunch of internal Spotify code that assumed that this method
always did a replacement, and we found it easier to revert the change
here in docker-client (while adding new methods for appending to the
list).

Author: mattnworb

docs/user_manual.md

@@ -439,7 +439,7 @@ try (OutputStream imageOutput = new BufferedOutputStream(new FileOutputStream(im
   }
 }
 ```
- 
+
 
 ### Get a tarball containing all images.
 
@@ -448,7 +448,7 @@ Not implemented yet. PRs welcome.
 ### Load a tarball with a set of images and tags into docker
 
 Not implemented yet. PRs welcome.
- 
+
 ### Exec Create
 
 ```java
@@ -488,19 +488,19 @@ There are two ways to make a bind:
 2. Create a `Bind` object and pass it to `binds()`.
 
 If you only need to create a volume in a container, and you don't need it to mount any
-particular directory on the host, you can use the `volumes()` method on the 
+particular directory on the host, you can use the `volumes()` method on the
 `ContainerConfig.Builder`.
 
 ```java
-final HostConfig hostConfig = 
+final HostConfig hostConfig =
   HostConfig.builder()
-    .binds("/local/path:/remote/path")
-    .binds(Bind.from("/another/local/path")
+    .appendBinds("/local/path:/remote/path")
+    .appendBinds(Bind.from("/another/local/path")
                .to("/another/remote/path")
                .readOnly(true)
                .build())
     .build();
-final ContainerConfig volumeConfig = 
+final ContainerConfig volumeConfig =
   ContainerConfig.builder()
     .image("busybox:latest")
     .volumes("/foo")   // This volume will not mount any host directory
@@ -509,8 +509,8 @@ final ContainerConfig volumeConfig =
 ```
 
 #### A note on mounts
-Be aware that, starting with API version 1.20 (docker version 1.8.x), information 
-about a container's volumes is returned with the key `"Mounts"`, not `"Volumes"`.  
+Be aware that, starting with API version 1.20 (docker version 1.8.x), information
+about a container's volumes is returned with the key `"Mounts"`, not `"Volumes"`.
 As such, the `ContainerInfo.volumes()` method is deprecated. Instead, use
 `ContainerInfo.mounts()`.
 

src/main/java/com/spotify/docker/client/messages/HostConfig.java

@@ -17,14 +17,14 @@
 
 package com.spotify.docker.client.messages;
 
+import com.fasterxml.jackson.annotation.JsonAutoDetect;
+import com.fasterxml.jackson.annotation.JsonProperty;
 import com.google.common.base.MoreObjects;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 
-import com.fasterxml.jackson.annotation.JsonAutoDetect;
-import com.fasterxml.jackson.annotation.JsonProperty;
-
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -152,7 +152,7 @@ public String networkMode() {
   public List<String> securityOpt() {
     return securityOpt;
   }
-  
+
   public List<Device> devices() {
     return devices;
   }
@@ -524,17 +524,22 @@ private Builder(final HostConfig hostConfig) {
       this.ipcMode = hostConfig.ipcMode;
     }
 
+    /**
+     * Set the list of binds to the parameter, replacing any existing value.
+     * <p>To append to the list instead, use one of the appendBinds() methods.</p>
+     */
     public Builder binds(final List<String> binds) {
       if (binds != null && !binds.isEmpty()) {
-        if (this.binds != null) {
-          binds.addAll(0, this.binds);
-        }
         this.binds = ImmutableList.copyOf(binds);
       }
 
       return this;
     }
 
+    /**
+     * Set the list of binds to the parameter, replacing any existing value.
+     * <p>To append to the list instead, use one of the appendBinds() methods.</p>
+     */
     public Builder binds(final String... binds) {
       if (binds != null && binds.length > 0) {
         return binds(Lists.newArrayList(binds));
@@ -543,16 +548,45 @@ public Builder binds(final String... binds) {
       return this;
     }
 
+    /**
+     * Set the list of binds to the parameter, replacing any existing value.
+     * <p>To append to the list instead, use one of the appendBinds() methods.</p>
+     */
     public Builder binds(final Bind... binds) {
       if (binds == null || binds.length == 0) {
         return this;
       }
 
+      return binds(toStringList(binds));
+    }
+
+    private static List<String> toStringList(final Bind[] binds) {
       final List<String> bindStrings = Lists.newArrayList();
       for (final Bind bind : binds) {
         bindStrings.add(bind.toString());
       }
-      return binds(bindStrings);
+      return bindStrings;
+    }
+
+    /** Append binds to the existing list in this builder. */
+    public Builder appendBinds(final Iterable<String> newBinds) {
+      final List<String> list = new ArrayList<>(this.binds);
+      list.addAll(Lists.newArrayList(newBinds));
+      this.binds = ImmutableList.copyOf(list);
+
+      return this;
+    }
+
+    /** Append binds to the existing list in this builder. */
+    public Builder appendBinds(final Bind... binds) {
+      appendBinds(toStringList(binds));
+      return this;
+    }
+
+    /** Append binds to the existing list in this builder. */
+    public Builder appendBinds(final String... binds) {
+      appendBinds(Lists.newArrayList(binds));
+      return this;
     }
 
     public List<String> binds() {
@@ -803,7 +837,7 @@ public Builder devices(final Device... devices) {
     public List<Device> devices() {
       return devices;
     }
-    
+
     public Builder memory(final Long memory) {
       this.memory = memory;
       return this;

src/test/java/com/spotify/docker/client/DefaultDockerClientTest.java

@@ -1760,8 +1760,8 @@ public void testContainerVolumes() throws Exception {
                 .readOnly(true)
                 .build();
     final HostConfig hostConfig = HostConfig.builder()
-            .binds("/local/path:/remote/path")
-            .binds(bind)
+            .appendBinds("/local/path:/remote/path")
+            .appendBinds(bind)
             .build();
     final ContainerConfig volumeConfig = ContainerConfig.builder()
             .image(BUSYBOX_LATEST)

src/test/java/com/spotify/docker/client/messages/HostConfigTest.java

@@ -17,17 +17,18 @@
 
 package com.spotify.docker.client.messages;
 
-import com.fasterxml.jackson.databind.ObjectMapper;
+import com.spotify.docker.client.ObjectMapperProvider;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.base.Charsets;
+import com.google.common.collect.ImmutableList;
 import com.google.common.io.Resources;
 
-import com.spotify.docker.client.ObjectMapperProvider;
-
 import org.junit.Before;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.List;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertThat;
@@ -68,4 +69,35 @@ public void testJsonOnFailure() throws Exception {
   private static String fixture(String filename) throws IOException {
     return Resources.toString(Resources.getResource(filename), Charsets.UTF_8).trim();
   }
+
+  @Test
+  public void testReplaceBinds() {
+    final List<String> initialBinds = ImmutableList.of("/one:/one", "/two:/two");
+    final HostConfig hostConfig = HostConfig.builder()
+        .binds(initialBinds)
+        .binds(initialBinds)
+        .build();
+
+    assertThat("Calling .binds() multiple times should replace the list each time",
+               hostConfig.binds(), is(initialBinds));
+  }
+
+  @Test
+  public void testAppendBinds() {
+    final List<String> initialBinds = ImmutableList.of("/one:/one", "/two:/two");
+    final HostConfig hostConfig = HostConfig.builder()
+        .binds(initialBinds)
+        .appendBinds("/three:/three")
+        .appendBinds("/four:/four")
+        .build();
+
+    final List<String> expected = ImmutableList.<String>builder()
+        .addAll(initialBinds)
+        .add("/three:/three")
+        .add("/four:/four")
+        .build();
+
+    assertThat("Calling .appendBinds should append to the list, not replace",
+               hostConfig.binds(), is(expected));
+  }
 }