Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-07 Thread Ignasi Barrera 
Rebased and 
[merged](https://git-wip-us.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=9b124ee9f12e0392b6d2f083308297bfcca8ea79).
 Huge thanks @andreaturli!

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-48159799

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-07 Thread Ignasi Barrera 
Closed #57.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#event-138666393

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-07 Thread Andrea Turli 
great @nacx! Thanks again for your valuable reviews

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-48165987

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-02 Thread Andrew Kennedy 
 +  return this;
 +   }
 +
 +   public TemplateOptions commands(IterableString commands) {
 +  for (String command : checkNotNull(commands, commands))
 + checkNotNull(command, all commands must be non-empty);
 +  this.commands = Optional.ListString 
 of(ImmutableList.copyOf(commands));
 +  return this;
 +   }
 +
 +   public TemplateOptions commands(String... commands) {
 +  return commands(ImmutableList.copyOf(checkNotNull(commands, 
 commands)));
 +   }
 +
 +   public TemplateOptions cpuShares(int cpuShares) {
 +  checkNotNull(cpuShares, cpuShares was null);

Why do you do the `checkNotNull` then create an `Optional`? In fact, since this 
is a primitive, it cannot ever *be* null. If we want the null semantics, change 
to the following (and see also `memory`, plus `dns` and `hostname` which are 
strings, so *could* be null...)

```Java
public TemplateOptions cpuShares(@Nullable Integer cpuShares) {
this.cpuShares = Optional.fromNullable(cpuShares);
return this;
}

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14482891

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-02 Thread Andrew Kennedy 
 +
 +  if (templateOptions.getCommands().isPresent()) {
 + containerConfigBuilder.cmd(templateOptions.getCommands().get());
 +  }
 +
 +  if (templateOptions.getMemory().isPresent()) {
 + containerConfigBuilder.memory(templateOptions.getMemory().get());
 +  }
 +
 +  if (templateOptions.getCpuShares().isPresent()) {
 + 
 containerConfigBuilder.cpuShares(templateOptions.getCpuShares().get());
 +  }
 +
 +  if (templateOptions.getVolumes().isPresent()) {
 + MapString, Object volumes = Maps.newLinkedHashMap();
 + for (String containerDir : 
 templateOptions.getVolumes().get().values()) {

This isn't right. You're throwing away the host directory portion. We have two 
use cases for volumes, one is mounting a host directory in a specific location 
on a container, the other is creating a volume in a specific location on a 
container for export to another container. This will only cover the second case.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14484014

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-02 Thread BuildHive 
[jclouds » jclouds-labs 
#1258](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1258/) 
SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47839929

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-02 Thread CloudBees pull request builder plugin 
[jclouds-labs-pull-requests 
#205](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/205/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47840689

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-02 Thread Andrew Kennedy 
 +
 +  if (templateOptions.getCommands().isPresent()) {
 + containerConfigBuilder.cmd(templateOptions.getCommands().get());
 +  }
 +
 +  if (templateOptions.getMemory().isPresent()) {
 + containerConfigBuilder.memory(templateOptions.getMemory().get());
 +  }
 +
 +  if (templateOptions.getCpuShares().isPresent()) {
 + 
 containerConfigBuilder.cpuShares(templateOptions.getCpuShares().get());
 +  }
 +
 +  if (templateOptions.getVolumes().isPresent()) {
 + MapString, Object volumes = Maps.newLinkedHashMap();
 + for (String containerDir : 
 templateOptions.getVolumes().get().values()) {

I missed the `HostConfig` setup a few lines later, which *does* do the host 
binding. We don't yet distinguish between RW and RO bindings, but that is a 
future enhancement probably.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14485991

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-01 Thread Andrea Turli 
Here you can find the result of `mvn clean install -Plive` 
https://gist.github.com/andreaturli/595ce6e44a987ed5be06

unfortunately there are still issue with the DockerComputeServiceLiveTest but I 
think it is good to be accepted on jclouds-labs at least and fix liveTests 
there, if you agree

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47675918

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-01 Thread CloudBees pull request builder plugin 
[jclouds-labs-pull-requests 
#202](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/202/) 
UNSTABLE
Looks like there's a problem with this pull request

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47677495

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-01 Thread BuildHive 
[jclouds » jclouds-labs 
#1242](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1242/) 
UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47677662

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-01 Thread CloudBees pull request builder plugin 
[jclouds-labs-pull-requests 
#203](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/203/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47680678

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-01 Thread BuildHive 
[jclouds » jclouds-labs 
#1243](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1243/) 
SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47680874

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-01 Thread CloudBees pull request builder plugin 
[jclouds-labs-pull-requests 
#204](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/204/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47683209

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-07-01 Thread BuildHive 
[jclouds » jclouds-labs 
#1244](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1244/) 
SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47683314

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread Ignasi Barrera 
 +##Components
 +
 +- jclouds \- acts as a java client to access to docker features
 +- DOCKER_HOST \- hosts Docker API, NB: jclouds-docker assumes that the 
 latest Docker is installed
 +- Image \- it is a docker image that can be started.
 +- Node \- is a docker container
 +
 +## Assumptions
 +
 +- jclouds-docker assumes that the images specified using the template are 
 ssh'able.
 +- jclouds-docker will mount ${user.home} to /root of the container
 +
 +--
 +
 +#Notes:
 +- jclouds-docker is still at alpha stage please report any issues you find 
 at [jclouds issues](https://github.com/jclouds/jclouds/issues?state=open)

Ping! This link still has to be changed.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14173875

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread Ignasi Barrera 
 + * distributed under the License is distributed on an AS IS BASIS,
 + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 + * See the License for the specific language governing permissions and
 + * limitations under the License.
 + */
 +package org.jclouds.docker;
 +
 +import org.jclouds.docker.features.RemoteApi;
 +import org.jclouds.rest.annotations.Delegate;
 +
 +import java.io.Closeable;
 +
 +/**
 + * Provides synchronous access to Docker Remote API.
 + *
 + * @see a 
 href=http://docs.docker.io/en/latest/api/docker_remote_api_v1.8/#docker-remote-api-v1-8;/a

The link points to version 1.8 but the api metadata uses version 1.11. Change 
all links accordingly or just point to a root location that does not contain 
the version in the URL?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14173962

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread Ignasi Barrera 
 +import org.jclouds.rest.Binder;
 +
 +import com.google.common.base.Throwables;
 +import com.google.common.io.Files;
 +
 +/**
 + * @author Andrea Turli
 + */
 +@Singleton
 +public class BindInputStreamToRequest implements Binder {
 +
 +   @Resource
 +   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
 +   protected Logger logger = Logger.NULL;
 +
 +   @SuppressWarnings(unchecked)

This still has to be removed.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174051

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread Ignasi Barrera 
 +
 +   @Resource
 +   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
 +   private Logger logger = Logger.NULL;
 +   private final DockerApi api;
 +   private final ListeningExecutorService userExecutor;
 +   private final PredicateAtomicReferenceImage imageAvailablePredicate;
 +   private final ImageToImage imageToImage;
 +
 +   @Inject
 +   public DockerImageExtension(DockerApi api, 
 @Named(Constants.PROPERTY_USER_THREADS) ListeningExecutorService
 +   userExecutor, @Named(TIMEOUT_IMAGE_AVAILABLE) 
 PredicateAtomicReferenceImage imageAvailablePredicate, ImageToImage 
 imageToImage) {
 +  this.api = checkNotNull(api, api);
 +  this.userExecutor = checkNotNull(userExecutor, userExecutor);
 +  this.imageAvailablePredicate = checkNotNull(imageAvailablePredicate, 
 imageAvailablePredicate);
 +  this.imageToImage = imageToImage;

Add also a null check for this.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174091

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread Ignasi Barrera 
 +  .hardware(new HardwareBuilder()
 +  .id()
 +  .ram(container.getContainerConfig().getMemory())
 +  .processor(new 
 Processor(container.getContainerConfig().getCpuShares(), 
 container.getContainerConfig().getCpuShares()))
 +  .build());
 +  builder.status(toPortableStatus.apply(container.getState()));
 +  builder.imageId(container.getImage());
 +  builder.loginPort(getLoginPort(container));
 +  builder.publicAddresses(getPublicIpAddresses());
 +  builder.privateAddresses(getPrivateIpAddresses(container));
 +
 +  Image image = images.get().get(container.getImage());
 +  builder.imageId(image.getId());
 +  builder.operatingSystem(image.getOperatingSystem());
 +
 +  return builder.build();

Is there a way we can set the credentials here? Even if they are the default 
`root:password`? They may come in handy when getting a node (that has not been 
created by jclouds) using the ComputeService.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174259

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread Ignasi Barrera 
 +throw Throwables.propagate(e);
 + }
 + command.setException(exception);
 +  }
 +   }
 +
 +   public String parseMessage(HttpResponse response) {
 +  if (response.getPayload() == null)
 + return null;
 +  try {
 + return Strings2.toString(response.getPayload());
 +  } catch (IOException e) {
 + throw Throwables.propagate(e);
 +  } finally {
 + try {
 +response.getPayload().getInput().close();

The payload itself is already closeable. Close the payload to avoid the 
deprecation warning?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174535

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread Ignasi Barrera 
 +
 +import com.google.common.io.CharStreams;
 +
 +@Test(groups = unit, testName = BindInputStreamToRequestTest)
 +public class BindInputStreamToRequestTest {
 +
 +   @Test
 +   public void testBindInputStreamToRequest() throws IOException {
 +  BindInputStreamToRequest binder = new BindInputStreamToRequest();
 +
 +  HttpRequest request = 
 HttpRequest.builder().method(GET).endpoint(http://test;).build();
 +  request = binder.bindToRequest(request, 
 File.createTempFile(dockerfile, ));
 +  String rawContent = CharStreams.toString(new 
 InputStreamReader((FileInputStream) request.getPayload().getRawContent(), 
 UTF-8));
 +  assertTrue(rawContent.startsWith(Dockerfile));
 +  
 assertEquals(request.getPayload().getContentMetadata().getContentType(), 
 application/tar);
 +   }

Add also a couple small tests to verify the preconditions.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174685

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread Ignasi Barrera 
 +import org.jclouds.sshj.config.SshjSshClientModule;
 +import org.testng.annotations.AfterMethod;
 +import org.testng.annotations.BeforeMethod;
 +import org.testng.annotations.Test;
 +
 +import javax.annotation.Resource;
 +import javax.inject.Named;
 +import java.io.IOException;
 +import java.util.Map;
 +import java.util.Set;
 +
 +import static org.testng.Assert.assertEquals;
 +import static org.testng.Assert.assertTrue;
 +
 +@Test(groups = live, singleThreaded = true, testName = 
 DockerExperimentLiveTest)
 +public class DockerExperimentLiveTest extends BaseDockerApiLiveTest {

Can this be renamed to something more meaningful?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r14174698

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread Ignasi Barrera 
Thanks @andreaturli! All the points in the checklist have been addressed, and I 
think it is almost ready to be merged. Could you address the last comments? 
(Don't forget about [this 
one](https://github.com/jclouds/jclouds-labs/pull/57/files#r14173875) and [this 
one](https://github.com/jclouds/jclouds-labs/pull/57/files#r14174051)).


---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47076635

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread BuildHive 
[jclouds » jclouds-labs 
#1208](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1208/) 
SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47081016

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-25 Thread CloudBees pull request builder plugin 
[jclouds-labs-pull-requests 
#184](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/184/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-47081238

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-12 Thread CloudBees pull request builder plugin 
[jclouds-labs-pull-requests 
#176](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/176/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45945836

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-12 Thread BuildHive 
[jclouds » jclouds-labs 
#1158](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1158/) 
SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45945956

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-10 Thread CloudBees pull request builder plugin 
[jclouds-labs-pull-requests 
#175](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/175/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45648690

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-10 Thread BuildHive 
[jclouds » jclouds-labs 
#1150](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1150/) 
SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45648657

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-09 Thread BuildHive 
[jclouds » jclouds-labs 
#1139](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1139/) 
SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45490816

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-09 Thread CloudBees pull request builder plugin 
[jclouds-labs-pull-requests 
#174](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/174/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45491046

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-06 Thread Andrea Turli 
 + destroyNode(container.getId());
 + throw new IllegalStateException(String.format(Container %s has not 
 started correctly, container.getId()));
 +  }
 +  return new NodeAndInitialCredentialsContainer(container, 
 container.getId(),
 +  
 LoginCredentials.builder().user(loginUser).password(loginUserPassword).build());
 +   }
 +
 +   @Override
 +   public IterableHardware listHardwareProfiles() {
 +  SetHardware hardware = Sets.newLinkedHashSet();
 +  // todo they are only placeholders at the moment
 +  hardware.add(new 
 HardwareBuilder().ids(micro).hypervisor(lxc).name(micro).ram(512).build());
 +  hardware.add(new 
 HardwareBuilder().ids(small).hypervisor(lxc).name(small).ram(1024).build());
 +  hardware.add(new 
 HardwareBuilder().ids(medium).hypervisor(lxc).name(medium).ram(2048).build());
 +  hardware.add(new 
 HardwareBuilder().ids(large).hypervisor(lxc).name(large).ram(3072).build());
 +  return hardware;

at the moment you can only setup cpuShares. By default, all containers run at 
the same priority and get the same proportion of CPU cycles, but you can tell 
the kernel to give more shares of CPU time to one or more containers when you 
start them via Docker. So it is not really an information about cpu limit, so 
not sure we can use it on hardwareProfiles


---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r13494140

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-04 Thread Imesh Guaratne 
@andreaturli 
I found several problems while trying to run a docker container (apache2) and 
sent PR to your forked branch: 
https://github.com/andreaturli/jclouds-labs/pull/2 WDYT?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45140914

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-03 Thread Andrew Gaul 
@imesh jclouds master branch uses Guava 17.0 which makes ```Stopwatch``` 
constructors private access.  The jclouds dependency supersedes your 
application dependency on an older version of Guava.  You should update your 
application dependency to 17.0 and address the Guava API changes and 
deprecations to make progress.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45007831

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-06-03 Thread Andrea Turli 
@imesh Thanks for your feedback.

That's odd because I can see
```
$ mvn dependency:tree|grep guava
[INFO] |  \- com.google.guava:guava:jar:17.0:compile
```
and I can build jclouds docker master

Are you using jclouds-docker from another application as @andrewgaul was 
guessing?


---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-45011359

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-30 Thread Andrea Turli 
 +   }
 +
 +   @Override
 +   public IterableLocation listLocations() {
 +  return ImmutableSet.of();
 +   }
 +
 +   @Override
 +   public Container getNode(String id) {
 +  return api.getRemoteApi().inspectContainer(id);
 +   }
 +
 +   @Override
 +   public void destroyNode(String id) {
 +  api.getRemoteApi().stopContainer(id);
 +  api.getRemoteApi().removeContainer(id);

removeContainer supports `force` that removes the container even if it was 
running. Default false.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r13232276

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-30 Thread Andrew Gaul 
 +  checkArgument(baseDir.isDirectory(), %s is not a directory, baseDir);
 +  File[] files = baseDir.listFiles();
 +  File tarFile = new File(archivePath);
 +
 +  String token = getLast(Splitter.on(/).split(archivePath.substring(0, 
 archivePath.lastIndexOf(/;
 +
 +  byte[] buf = new byte[1024];
 +  int len;
 +  TarArchiveOutputStream tos = new TarArchiveOutputStream(new 
 FileOutputStream(tarFile));
 +  tos.setLongFileMode(TarArchiveOutputStream.LONGFILE_GNU);
 +  for (File file : files) {
 + TarArchiveEntry tarEntry = new TarArchiveEntry(file);
 + tarEntry.setName(/ + 
 getLast(Splitter.on(token).split(file.toString(;
 + tos.putArchiveEntry(tarEntry);
 + if (!file.isDirectory()) {
 +FileInputStream fin = new FileInputStream(file);

Too many calls to ```TarArchiveStream.close``` on the successful code path, not 
guaranteed to call on the exceptional path, and 
```TarArchiveStream.closeArchiveEntry``` not called when 
```!file.isDirectory```.  Otherwise the ```ByteSource``` change is a good 
improvement.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r13241767

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-25 Thread Andrew Phillips 
Thanks for the detailed review, @nacx!

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-44155452

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +##Components
 +
 +- jclouds \- acts as a java client to access to docker features
 +- DOCKER_HOST \- hosts Docker API, NB: jclouds-docker assumes that the 
 latest Docker is installed
 +- Image \- it is a docker image that can be started.
 +- Node \- is a docker container
 +
 +## Assumptions
 +
 +- jclouds-docker assumes that the images specified using the template are 
 ssh'able.
 +- jclouds-docker will mount ${user.home} to /root of the container
 +
 +--
 +
 +#Notes:
 +- jclouds-docker is still at alpha stage please report any issues you find 
 at [jclouds issues](https://github.com/jclouds/jclouds/issues?state=open)

Change the link to point to the jclouds JIRA

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936596

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +import org.jclouds.rest.Binder;
 +
 +import com.google.common.base.Throwables;
 +import com.google.common.io.Files;
 +
 +/**
 + * @author Andrea Turli
 + */
 +@Singleton
 +public class BindInputStreamToRequest implements Binder {
 +
 +   @Resource
 +   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
 +   protected Logger logger = Logger.NULL;
 +
 +   @SuppressWarnings(unchecked)

This is unnecessary, you can remove it.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936630

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Andrew Gaul 
 +  checkArgument(baseDir.isDirectory(), %s is not a directory, baseDir);
 +  File[] files = baseDir.listFiles();
 +  File tarFile = new File(archivePath);
 +
 +  String token = getLast(Splitter.on(/).split(archivePath.substring(0, 
 archivePath.lastIndexOf(/;
 +
 +  byte[] buf = new byte[1024];
 +  int len;
 +  TarArchiveOutputStream tos = new TarArchiveOutputStream(new 
 FileOutputStream(tarFile));
 +  tos.setLongFileMode(TarArchiveOutputStream.LONGFILE_GNU);
 +  for (File file : files) {
 + TarArchiveEntry tarEntry = new TarArchiveEntry(file);
 + tarEntry.setName(/ + 
 getLast(Splitter.on(token).split(file.toString(;
 + tos.putArchiveEntry(tarEntry);
 + if (!file.isDirectory()) {
 +FileInputStream fin = new FileInputStream(file);

Call ```Files.asByteSource(file).copyTo(tos)``` instead.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936904

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 @@ -0,0 +1,275 @@
 +/*

The entire `features` package must be moved outside the `compute` one.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936907

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  
 builder.operatingSystem(OperatingSystem.builder().description(linux).family(OsFamily.LINUX).build());
 +  return builder.build();
 +   }
 +
 +   private String cleanUpName(String name) {
 +  return name.startsWith(/) ? name.substring(1) : name;
 +   }
 +
 +   private IterableString getPrivateIpAddresses(Container container) {
 +  if (container.getNetworkSettings() == null) return ImmutableList.of();
 +  return ImmutableList.of(container.getNetworkSettings().getIpAddress());
 +   }
 +
 +   private ListString getPublicIpAddresses() {
 +  String dockerIpAddress = 
 URI.create(providerMetadata.getEndpoint()).getHost();
 +  return ImmutableList.of(dockerIpAddress);

Is this always true? Is the host sip the only valid public ip address? Or is 
there any network configuration in Docker that allows the containers to get 
their own public IP address?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936967

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +   }
 +
 +   private IterableString getPrivateIpAddresses(Container container) {
 +  if (container.getNetworkSettings() == null) return ImmutableList.of();
 +  return ImmutableList.of(container.getNetworkSettings().getIpAddress());
 +   }
 +
 +   private ListString getPublicIpAddresses() {
 +  String dockerIpAddress = 
 URI.create(providerMetadata.getEndpoint()).getHost();
 +  return ImmutableList.of(dockerIpAddress);
 +   }
 +
 +   protected static int getLoginPort(Container container) {
 +  if (container.getNetworkSettings() != null) {
 +  MapString, ListMapString,String ports = 
 container.getNetworkSettings().getPorts();
 +  if(ports != null) {

Add a space after the `ìf``.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12936985

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +/**
 + * @author Andrea Turli
 + */
 +public class ImageToImage implements 
 Functionorg.jclouds.docker.domain.Image, org.jclouds.compute.domain.Image {
 +
 +   private static final String CENTOS = centos;
 +   private static final String UBUNTU = ubuntu;
 +
 +   @Resource
 +   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
 +   protected Logger logger = Logger.NULL;
 +
 +   @Override
 +   public Image apply(org.jclouds.docker.domain.Image from) {
 +  checkNotNull(from, image);
 +  String description = 
 checkNotNull(Iterables.getFirst(from.getRepoTags(), null));

Add a message to the null check.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937048

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +
 +import com.google.common.base.Objects;
 +import com.google.common.base.Optional;
 +import com.google.common.collect.ImmutableMap;
 +
 +/**
 + * Contains options supported in the {@code ComputeService#runNode} 
 operation on the
 + * docker provider. h2Usage/h2 The recommended way to instantiate a
 + * DockerTemplateOptions object is to statically import 
 DockerTemplateOptions.* and invoke a static
 + * creation method followed by an instance mutator (if needed):
 + * p/
 + * code
 + * import static 
 org.jclouds.aws.ec2.compute.options.DockerTemplateOptions.Builder.*;
 + * p/
 + * ComputeService api = // get connection
 + * templateBuilder.options(inboundPorts(22, 80, 8080, 443));

It would be great to have the example show the usage of the Docker specific 
options.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937085

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +import org.jclouds.compute.options.TemplateOptions;
 +import org.jclouds.domain.LoginCredentials;
 +import org.jclouds.scriptbuilder.domain.Statement;
 +
 +import com.google.common.base.Objects;
 +import com.google.common.base.Optional;
 +import com.google.common.collect.ImmutableMap;
 +
 +/**
 + * Contains options supported in the {@code ComputeService#runNode} 
 operation on the
 + * docker provider. h2Usage/h2 The recommended way to instantiate a
 + * DockerTemplateOptions object is to statically import 
 DockerTemplateOptions.* and invoke a static
 + * creation method followed by an instance mutator (if needed):
 + * p/
 + * code
 + * import static 
 org.jclouds.aws.ec2.compute.options.DockerTemplateOptions.Builder.*;

Wrong package? :)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937069

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +
 +   @Inject
 +   public DockerComputeServiceAdapter(DockerApi api) {
 +  this.api = checkNotNull(api, api);
 +   }
 +
 +   @Override
 +   public NodeAndInitialCredentialsContainer 
 createNodeWithGroupEncodedIntoName(String group, String name,
 + 
  Template template) {
 +  checkNotNull(template, template was null);
 +  checkNotNull(template.getOptions(), template options was null);
 +  DockerTemplateOptions templateOptions = 
 DockerTemplateOptions.class.cast(template.getOptions());
 +
 +  String imageId = checkNotNull(template.getImage().getId(), template 
 image id must not be null);
 +  String loginUser = 
 template.getImage().getDefaultCredentials().getUser();
 +  String loginUserPassword = 
 template.getImage().getDefaultCredentials().getPassword();

Where are the credentials set? The `ImageToImage` does not set them. Is this 
going to throw a NPE?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937125

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 + destroyNode(container.getId());
 + throw new IllegalStateException(String.format(Container %s has not 
 started correctly, container.getId()));
 +  }
 +  return new NodeAndInitialCredentialsContainer(container, 
 container.getId(),
 +  
 LoginCredentials.builder().user(loginUser).password(loginUserPassword).build());
 +   }
 +
 +   @Override
 +   public IterableHardware listHardwareProfiles() {
 +  SetHardware hardware = Sets.newLinkedHashSet();
 +  // todo they are only placeholders at the moment
 +  hardware.add(new 
 HardwareBuilder().ids(micro).hypervisor(lxc).name(micro).ram(512).build());
 +  hardware.add(new 
 HardwareBuilder().ids(small).hypervisor(lxc).name(small).ram(1024).build());
 +  hardware.add(new 
 HardwareBuilder().ids(medium).hypervisor(lxc).name(medium).ram(2048).build());
 +  hardware.add(new 
 HardwareBuilder().ids(large).hypervisor(lxc).name(large).ram(3072).build());
 +  return hardware;

The list of hardware profiles should also include the cpu. I see two things to 
improve here:

* Provide more cpu/ram combinations (otherwise is not enough flexible), such as 
providers like CloudSigma do (those also have hardcoded hardware profiles).
* Create a SupplierHardware, qualified with something like `@Default` to 
avoid collisions with the jclouds hardware supplier, to let people bind their 
own hardware profiles if they need different ones.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937224

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +   }
 +
 +   @Override
 +   public IterableHardware listHardwareProfiles() {
 +  SetHardware hardware = Sets.newLinkedHashSet();
 +  // todo they are only placeholders at the moment
 +  hardware.add(new 
 HardwareBuilder().ids(micro).hypervisor(lxc).name(micro).ram(512).build());
 +  hardware.add(new 
 HardwareBuilder().ids(small).hypervisor(lxc).name(small).ram(1024).build());
 +  hardware.add(new 
 HardwareBuilder().ids(medium).hypervisor(lxc).name(medium).ram(2048).build());
 +  hardware.add(new 
 HardwareBuilder().ids(large).hypervisor(lxc).name(large).ram(3072).build());
 +  return hardware;
 +   }
 +
 +   @Override
 +   public SetImage listImages() {
 +  //return api.getRemoteApi().listImages();

Remove this line

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937230

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  }, null);
 +   }
 +
 +   @Override
 +   public IterableContainer listNodes() {
 +  SetContainer containers = Sets.newHashSet();
 +  for (Container container : api.getRemoteApi().listContainers()) {
 + // less efficient than just listNodes but returns richer json
 + 
 containers.add(api.getRemoteApi().inspectContainer(container.getId()));
 +  }
 +  return containers;
 +   }
 +
 +   @Override
 +   public IterableContainer listNodesByIds(final IterableString ids) {
 +  return filter(listNodes(), new PredicateContainer() {

This will generate a call to inspect *every* container, when you're only asking 
for a subset. Remove this and directly call `inspectContainer` on the provided 
ids.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937268

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  //return api.getRemoteApi().listImages();
 +  SetImage images = Sets.newHashSet();
 +  for (Image image : api.getRemoteApi().listImages()) {
 + // less efficient than just listNodes but returns richer json that 
 needs repoTags coming from listImages
 + Image inspected = api.getRemoteApi().inspectImage(image.getId());
 + if(image.getRepoTags() != null) {
 +inspected = 
 Image.builder().fromImage(inspected).repoTags(image.getRepoTags()).build();
 + }
 + images.add(inspected);
 +  }
 +  return images;
 +   }
 +
 +   @Override
 +   public Image getImage(final String imageId) {
 +  return find(listImages(), new PredicateImage() {

This will call the api to inspect every image, when you only want to retrieve 
one. Change this to directly call inspectImage.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937282

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +   }
 +
 +   @Override
 +   public IterableLocation listLocations() {
 +  return ImmutableSet.of();
 +   }
 +
 +   @Override
 +   public Container getNode(String id) {
 +  return api.getRemoteApi().inspectContainer(id);
 +   }
 +
 +   @Override
 +   public void destroyNode(String id) {
 +  api.getRemoteApi().stopContainer(id);
 +  api.getRemoteApi().removeContainer(id);

Can the container be removed even if the stop operation fails?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937360

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  this.user = user;
 +  this.memory = checkNotNull(memory, memory);
 +  this.memorySwap = checkNotNull(memorySwap, memorySwap);
 +  this.cpuShares = checkNotNull(cpuShares, cpuShares);
 +  this.attachStdin = checkNotNull(attachStdin, attachStdin);
 +  this.attachStdout = checkNotNull(attachStdout, attachStdout);
 +  this.attachStderr = checkNotNull(attachStderr, attachStderr);
 +  this.exposedPorts = exposedPorts != null ? 
 ImmutableMap.copyOf(exposedPorts) : ImmutableMap.String, Object of();
 +  this.tty = checkNotNull(tty, tty);
 +  this.openStdin = checkNotNull(openStdin, openStdin);
 +  this.stdinOnce = checkNotNull(stdinOnce, stdinOnce);
 +  this.env = env != null ? ImmutableList.copyOf(cmd) : 
 ImmutableList.String of();
 +  this.cmd = cmd != null ? ImmutableList.copyOf(cmd) : 
 ImmutableList.String of();
 +  this.dns = dns != null ? ImmutableList.copyOf(dns) : 
 ImmutableList.String of();
 +  this.imageId = checkNotNull(imageId, imageId);
 +  this.volumes = volumes != null ? ImmutableMap.copyOf(volumes) : 
 ImmutableMap.String, Object of();

`volumes` is not nullable. Add a null check or annotate it properly.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937403

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  this.created = checkNotNull(created, created);
 +  this.path = checkNotNull(path, path);
 +  this.args = checkNotNull(args, args);
 +  this.config = checkNotNull(config, config);
 +  this.state = checkNotNull(state, state);
 +  this.image = checkNotNull(image, image);
 +  this.networkSettings = checkNotNull(networkSettings, 
 networkSettings);
 +  this.resolvConfPath = checkNotNull(resolvConfPath, resolvConfPath);
 +  this.driver = checkNotNull(driver, driver);
 +  this.execDriver = checkNotNull(execDriver, execDriver);
 +  this.volumes = checkNotNull(volumes, volumes);
 +  this.volumesRw = checkNotNull(volumesRW, volumesRW);
 +  this.command = checkNotNull(command, command);
 +  this.status = checkNotNull(status, status);
 +  this.hostConfig = checkNotNull(hostConfig, hostConfig);
 +  this.ports = checkNotNull(ports, ports);

Make this immutable too.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937481

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +String image,  NetworkSettings networkSettings, String 
 resolvConfPath,
 +String driver, String execDriver, MapString, String 
 volumes, MapString, Boolean volumesRW,
 +String command, String status, HostConfig hostConfig, 
 ListPort ports) {
 +  this.id = checkNotNull(id, id);
 +  this.name = checkNotNull(name, name);
 +  this.created = checkNotNull(created, created);
 +  this.path = checkNotNull(path, path);
 +  this.args = checkNotNull(args, args);
 +  this.config = checkNotNull(config, config);
 +  this.state = checkNotNull(state, state);
 +  this.image = checkNotNull(image, image);
 +  this.networkSettings = checkNotNull(networkSettings, 
 networkSettings);
 +  this.resolvConfPath = checkNotNull(resolvConfPath, resolvConfPath);
 +  this.driver = checkNotNull(driver, driver);
 +  this.execDriver = checkNotNull(execDriver, execDriver);
 +  this.volumes = checkNotNull(volumes, volumes);

Make it immutable

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937465

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +String driver, String execDriver, MapString, String 
 volumes, MapString, Boolean volumesRW,
 +String command, String status, HostConfig hostConfig, 
 ListPort ports) {
 +  this.id = checkNotNull(id, id);
 +  this.name = checkNotNull(name, name);
 +  this.created = checkNotNull(created, created);
 +  this.path = checkNotNull(path, path);
 +  this.args = checkNotNull(args, args);
 +  this.config = checkNotNull(config, config);
 +  this.state = checkNotNull(state, state);
 +  this.image = checkNotNull(image, image);
 +  this.networkSettings = checkNotNull(networkSettings, 
 networkSettings);
 +  this.resolvConfPath = checkNotNull(resolvConfPath, resolvConfPath);
 +  this.driver = checkNotNull(driver, driver);
 +  this.execDriver = checkNotNull(execDriver, execDriver);
 +  this.volumes = checkNotNull(volumes, volumes);
 +  this.volumesRw = checkNotNull(volumesRW, volumesRW);

Same here, make it immutable.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937472

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +
 +import com.google.common.base.Objects;
 +import com.google.common.collect.ImmutableSet;
 +
 +import java.beans.ConstructorProperties;
 +import java.util.Set;
 +
 +import static com.google.common.base.Preconditions.checkNotNull;
 +
 +/**
 + * @author Andrea Turli
 + */
 +public class ExposedPorts {
 +
 +   private final String portAndProtocol;
 +   private final SetString hostPorts;

Add the missing `@SerlalizedName` annotations.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937504

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +import java.util.Set;
 +
 +import static com.google.common.base.Preconditions.checkNotNull;
 +
 +/**
 + * @author Andrea Turli
 + */
 +public class ExposedPorts {
 +
 +   private final String portAndProtocol;
 +   private final SetString hostPorts;
 +
 +   @ConstructorProperties({ PortAndProtocol, HostPorts })
 +   public ExposedPorts(String portAndProtocol, SetString hostPorts) {
 +  this.portAndProtocol = checkNotNull(portAndProtocol, 
 portAndProtocol);
 +  this.hostPorts = hostPorts != null ? ImmutableSet.copyOf(hostPorts) : 
 ImmutableSet.String of();

If `hostPorts` can be null, annotate it as `@Nullable`, otherwise add a null 
check.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937518

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +   @SerializedName(Binds)
 +   private final ListString binds;
 +   @SerializedName(Privileged)
 +   private final boolean privileged;
 +   @SerializedName(PortBindings)
 +   private final MapString, ListMapString, String portBindings;
 +   @SerializedName(Links)
 +   private final ListString links;
 +   @SerializedName(PublishAllPorts)
 +   private final boolean publishAllPorts;
 +
 +   @ConstructorProperties({ ContainerIDFile, Binds, Privileged, 
 PortBindings, Links, Size,
 +   PublishAllPorts })
 +   public HostConfig(@Nullable String containerIDFile, @Nullable 
 ListString binds, @Nullable boolean privileged,
 + @Nullable MapString, ListMapString, String 
 portBindings, @Nullable ListString links,
 + @Nullable boolean publishAllPorts) {

Can a `HostConfig` object be actually empty, with all properties being null?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937535

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +   private final String gateway;
 +   @SerializedName(Bridge)
 +   private final String bridge;
 +   @SerializedName(PortMapping)
 +   private final String portMapping;
 +   @SerializedName(Ports)
 +   private final MapString, ListMapString, String ports;
 +
 +   @ConstructorProperties({ IpAddress, IpPrefixLen, Gateway, Bridge, 
 Ports })
 +   public NetworkSettings(String ipAddress, int ipPrefixLen, String gateway, 
 String bridge, String portMapping,
 +  MapString, ListMapString, String ports) {
 +  this.ipAddress = ipAddress;
 +  this.ipPrefixLen = ipPrefixLen;
 +  this.gateway = gateway;
 +  this.bridge = bridge;
 +  this.portMapping = portMapping;

Add null checks for all properties that are not nullable.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937553

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +   public NodeMetadata apply(Container container) {
 +  String name = cleanUpName(container.getName());
 +  String group = nodeNamingConvention.extractGroup(name);
 +  NodeMetadataBuilder builder = new NodeMetadataBuilder();
 +  builder.ids(container.getId())
 +  .name(name)
 +  .group(group)
 +  .hostname(container.getConfig().getHostname())
 +   // TODO Set up hardware
 +  .hardware(new HardwareBuilder()
 +  .id()
 +  .ram(container.getConfig().getMemory())
 +  .processor(new 
 Processor(container.getConfig().getCpuShares(), 
 container.getConfig().getCpuShares()))
 +  .build());
 +  // TODO Set up location properly
 +  LocationBuilder locationBuilder = new LocationBuilder();

If I'm not wrong, the default location, if no locations are configured is the 
provider. You should inject here the location supplier and use that location, 
as the provider is already the host.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937658

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  return result;
 +   }
 +
 +   protected Payload createPayload() throws IOException {
 +  String folderPath = System.getProperty(user.dir) + 
 /docker/src/test/resources;
 +  File parentDir = new File(folderPath + /archive);
 +  parentDir.mkdirs();
 +  URL url = Resources.getResource(Dockerfile);
 +  String content = Resources.toString(url, Charsets.UTF_8);
 +  final File dockerfile = new File(parentDir.getAbsolutePath() + 
 File.separator + Dockerfile);
 +  Files.write(content.getBytes(), dockerfile);
 +  File archive = Archives.tar(parentDir, folderPath + 
 /archive/archive.tar);
 +  FileInputStream data = new FileInputStream(archive);
 +  Payload payload = Payloads.newInputStreamPayload(data);
 +  
 payload.getContentMetadata().setContentLength(data.getChannel().size());
 +  payload.getContentMetadata().setContentType(MediaType.TEXT_PLAIN);

Same as commented above. Is this the right media type? Note that it doesn't 
match the ones defined in the `RemoteApi.build` methods.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937709

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +import org.testng.annotations.Test;
 +
 +import javax.annotation.Resource;
 +import javax.inject.Named;
 +import java.io.IOException;
 +import java.util.Map;
 +import java.util.Set;
 +
 +import static org.testng.Assert.assertEquals;
 +import static org.testng.Assert.assertTrue;
 +
 +/**
 + * @author Andrea Turli
 + */
 +@Test(groups = live, singleThreaded = true, testName = 
 DockerExperimentLiveTest)
 +public class DockerExperimentLiveTest extends BaseDockerApiLiveTest {

Change the name to something more meaningful?
Also, if this test targets the ComputeServie, it should be better to extend 
`BaseViewLiveTest` instead.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937759

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +   public void testVersion() {
 +  Assert.assertEquals(api().getVersion().getVersion(), 0.9.0);
 +   }
 +
 +   @Test(dependsOnMethods = testVersion)
 +   public void testCreateImage() throws IOException, InterruptedException {
 +  CreateImageOptions options = 
 CreateImageOptions.Builder.fromImage(BUSYBOX_IMAGE);
 +  InputStream createImageStream = api().createImage(options);
 +  consumeStream(createImageStream, false);
 +  image = api().inspectImage(BUSYBOX_IMAGE);
 +  assertNotNull(image);
 +   }
 +
 +   @Test(dependsOnMethods = testCreateImage)
 +   public void testListImages() {
 +  Assert.assertNotNull(api().listImages());

There are more after this one. Use them in general in this class.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937821

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  assertNull(api().inspectImage(image.getId()));
 +   }
 +
 +   @Test(dependsOnMethods = testDeleteImage)
 +   public void testBuildImage() throws IOException, InterruptedException, 
 URISyntaxException {
 +  BuildOptions options = 
 BuildOptions.Builder.tag(testBuildImage).verbose(false).nocache(false);
 +  InputStream buildImageStream = api().build(new 
 File(Resources.getResource(centos/Dockerfile).toURI()), options);
 +  String buildStream = consumeStream(buildImageStream, false);
 +  IterableString splitted = 
 Splitter.on(\n).split(buildStream.replace(\r, ).trim());
 +  String lastStreamedLine = Iterables.getLast(splitted).trim();
 +  String rawImageId = Iterables.getLast(Splitter.on(Successfully built 
 ).split(lastStreamedLine));
 +  String imageId = rawImageId.substring(0, 11);
 +  Image image = api().inspectImage(imageId);
 +  api().removeContainer(image.getContainer());
 +  api().deleteImage(imageId, DeleteImageOptions.Builder.force(true));
 +   }

There are still methods int he RemoteApi that don't have the corresponding live 
test. They must be added.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937868

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +   public void testListContainers() throws Exception {
 +  MockWebServer server = mockWebServer();
 +  server.enqueue(new 
 MockResponse().setBody(payloadFromResource(/containers.json)));
 +
 +  DockerApi api = api(server.getUrl(/));
 +  RemoteApi remoteApi = api.getRemoteApi();
 +
 +  try {
 + SetContainer containers = remoteApi.listContainers();
 + assertRequestHasCommonFields(server.takeRequest(), 
 /containers/json);
 + assertEquals(containers.size(), 1);
 +  } finally {
 + api.close();
 + server.shutdown();
 +  }
 +   }

Add the corresponding test when the response is a 404 (to verify the fallback 
configured in the api).

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937911

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  MockWebServer server = mockWebServer();
 +  server.enqueue(new 
 MockResponse().setBody(payloadFromResource(/containers.json)));
 +
 +  DockerApi api = api(server.getUrl(/));
 +  RemoteApi remoteApi = api.getRemoteApi();
 +
 +  try {
 + SetContainer containers = 
 remoteApi.listContainers(ListContainerOptions.Builder.all(true));
 + assertRequestHasParameters(server.takeRequest(), 
 /containers/json, ImmutableMultimap.of(all,
 + true));
 + assertEquals(containers.size(), 1);
 +  } finally {
 + api.close();
 + server.shutdown();
 +  }
 +   }

Add the corresponding test when the response is a 404 (to verify the fallback 
configured in the api).

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12937917

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  MockWebServer server = mockWebServer();
 +  server.enqueue(new 
 MockResponse().setBody(payloadFromResource(/container-creation.json)));
 +
 +  DockerApi api = api(server.getUrl(/));
 +  RemoteApi remoteApi = api.getRemoteApi();
 +  Config config = Config.builder().cmd(ImmutableList.of(date))
 +  .attachStdin(false)
 +  .attachStderr(true)
 +  .attachStdout(true)
 +  .tty(false)
 +  .imageId(base)
 +  .build();
 +  try {
 + Container container = remoteApi.createContainer(test, config);
 + assertNotNull(container);
 + assertEquals(container.getId(), 
 c6c74153ae4b1d1633d68890a68d89c40aa5e284a1ea016cbc6ef0e634ee37b2);

Also check the recorded request.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938136

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Andrew Gaul 
 +
 +  DockerApi api = api(server.getUrl(/));
 +  RemoteApi remoteApi = api.getRemoteApi();
 +
 +  String content = new String(payloadFromResource(/Dockerfile));
 +  File dockerFile = createDockerFile(content);
 +  try {
 + try {
 +remoteApi.build(dockerFile, BuildOptions.NONE);
 +fail(Build container should fail on 404);
 + } catch (ResourceNotFoundException ex) {
 +// Expected exception
 + }
 + assertRequestHasCommonFields(server.takeRequest(), POST, 
 /build);
 +  } finally {
 + dockerFile.deleteOnExit();

Why not ```dockerFile.delete()```?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938165

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 + remoteApi.removeContainer(containerId);
 + assertRequestHasCommonFields(server.takeRequest(), DELETE, 
 /containers/+containerId);
 +  } finally {
 + api.close();
 + server.shutdown();
 +  }
 +   }
 +
 +   public void testRemoveNonExistingContainer() throws Exception {
 +  MockWebServer server = mockWebServer();
 +  server.enqueue(new MockResponse().setResponseCode(404));
 +  DockerApi api = api(server.getUrl(/));
 +  RemoteApi remoteApi = api.getRemoteApi();
 +  String containerId = nonExisting;
 +  try {
 + remoteApi.removeContainer(containerId);

Add a `fail` to fail the test if the `removeContainer` does not throw the 
exception.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938164

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 + server.shutdown();
 +  }
 +   }
 +
 +   public void testStartNonExistingContainer() throws Exception {
 +  MockWebServer server = mockWebServer();
 +  server.enqueue(new MockResponse().setResponseCode(204));
 +  DockerApi api = api(server.getUrl(/));
 +  RemoteApi remoteApi = api.getRemoteApi();
 +  try {
 + try {
 +remoteApi.startContainer(1);
 + } catch (ResourceNotFoundException ex) {
 +// Expected exception
 + }
 + assertRequestHasCommonFields(server.takeRequest(), POST, 
 /containers/1/start);

Move this to the catch block, and add a explicit `fail` after calling the api 
method.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938183

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  try {
 + remoteApi.deleteImage(1);
 + assertRequestHasCommonFields(server.takeRequest(), DELETE, 
 /images/1);
 +  } finally {
 + api.close();
 + server.shutdown();
 +  }
 +   }
 +
 +   public void testDeleteNotExistingImage() throws Exception {
 +  MockWebServer server = mockWebServer();
 +  server.enqueue(new MockResponse().setResponseCode(404));
 +  DockerApi api = api(server.getUrl(/));
 +  RemoteApi remoteApi = api.getRemoteApi();
 +  try {
 + remoteApi.deleteImage(1);

Same pattern. Fail the test here.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938217

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 + assertRequestHasParameters(server.takeRequest(), POST, 
 /images/create, ImmutableMultimap.of(fromImage,
 + base));
 +  } finally {
 + api.close();
 + server.shutdown();
 +  }
 +   }
 +
 +   public void testCreateImageFailure() throws Exception {
 +  MockWebServer server = mockWebServer();
 +  server.enqueue(new MockResponse().setResponseCode(404));
 +  DockerApi api = api(server.getUrl(/));
 +  RemoteApi remoteApi = api.getRemoteApi();
 +  try {
 + remoteApi.createImage(CreateImageOptions.Builder.fromImage(base));
 + assertRequestHasParameters(server.takeRequest(), POST, 
 /images/create, ImmutableMultimap.of(fromImage,

Remove this and fail the test instead.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938208

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
Thanks for the contribution @andreaturli! I look forward to have this merged!
Next steps should be:

**General**
- [ ] Address all review comments.
- [ ] Make all constructors for the domain objects that have a builder 
`protected`, to enforce the use of the builder.
- [ ] Run `mvn checkstyle:checkstyle` and address all violations.

**Tests**
- [ ] Add unit tests for the `BindInputStreamToRequest` class.
- [ ] Add the `DockerImageExtensionLiveTest` that extends 
`BaseImageExtensionLiveTest`.
- [ ] Can the `Archives` class be unit tested in some way?
- [ ] Add tests for the `DockerTemplateOptions` class.

Thanks!

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-43858160

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Andrew Gaul 
 + }
 + assertRequestHasCommonFields(server.takeRequest(), POST, 
 /build);
 +  } finally {
 + dockerFile.deleteOnExit();
 + api.close();
 + server.shutdown();
 +  }
 +   }
 +
 +   private File createDockerFile(String content) {
 +  File newTempDir = Files.createTempDir();
 +  File dockerFile = new File(newTempDir + /dockerFile);
 +  try {
 + Files.write(content, dockerFile, Charsets.UTF_8);
 +  } catch(IOException e) {
 + Assert.fail();

Propagate ```IOException```?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12938577

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Andrew Gaul 
Please run through Checkstyle.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-43858507

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +   @SuppressWarnings(unchecked)
 +   @Override
 +   public R extends HttpRequest R bindToRequest(R request, Object input) {
 +  checkArgument(checkNotNull(input, input) instanceof File, this 
 binder is only valid for File!);
 +  checkNotNull(request, request);
 +
 +  File dockerFile = (File) input;
 +  File tmpDir = Files.createTempDir();
 +  final File targetFile = new File(tmpDir + File.separator + 
 Dockerfile);
 +  try {
 + Files.copy(dockerFile, targetFile);
 + File archive = Archives.tar(tmpDir, File.createTempFile(archive, 
 .tar));
 + FileInputStream data = new FileInputStream(archive);
 + Payload payload = Payloads.newInputStreamPayload(data);
 + 
 payload.getContentMetadata().setContentLength(data.getChannel().size());
 + payload.getContentMetadata().setContentType(MediaType.TEXT_PLAIN);

Worth a try :) If it is supported, I'd change it to `application/tar`, 
otherwise I'll leave it as-is, but change the headers in the 
[RemoteApi#build](https://github.com/andreaturli/jclouds-labs/blob/master/docker/src/main/java/org/jclouds/docker/compute/features/RemoteApi.java#L259-L272)
 methods accordingly.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12952833

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Andrea Turli 
 +  .id()
 +  .ram(container.getConfig().getMemory())
 +  .processor(new 
 Processor(container.getConfig().getCpuShares(), 
 container.getConfig().getCpuShares()))
 +  .build());
 +  // TODO Set up location properly
 +  LocationBuilder locationBuilder = new LocationBuilder();
 +  locationBuilder.description();
 +  locationBuilder.id();
 +  locationBuilder.scope(LocationScope.HOST);
 +  builder.location(locationBuilder.build());
 +  builder.status(toPortableStatus.apply(container.getState()));
 +  builder.imageId(container.getImage());
 +  builder.loginPort(getLoginPort(container));
 +  builder.publicAddresses(getPublicIpAddresses());
 +  builder.privateAddresses(getPrivateIpAddresses(container));
 +  
 builder.operatingSystem(OperatingSystem.builder().description(linux).family(OsFamily.LINUX).build());

unfortunately not, unless we pay an ssh connection to the container. This mean 
that we should start a container from that image and extract the needed 
details. I've opened an issue https://github.com/dotcloud/docker/issues/5480 
but it has been rejected :( 

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12953564

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Andrea Turli 
 +  
 builder.operatingSystem(OperatingSystem.builder().description(linux).family(OsFamily.LINUX).build());
 +  return builder.build();
 +   }
 +
 +   private String cleanUpName(String name) {
 +  return name.startsWith(/) ? name.substring(1) : name;
 +   }
 +
 +   private IterableString getPrivateIpAddresses(Container container) {
 +  if (container.getNetworkSettings() == null) return ImmutableList.of();
 +  return ImmutableList.of(container.getNetworkSettings().getIpAddress());
 +   }
 +
 +   private ListString getPublicIpAddresses() {
 +  String dockerIpAddress = 
 URI.create(providerMetadata.getEndpoint()).getHost();
 +  return ImmutableList.of(dockerIpAddress);

this seems to be the default right now. Docker is running fast so probably this 
could change in next versions ...

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12953630

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +  .id()
 +  .ram(container.getConfig().getMemory())
 +  .processor(new 
 Processor(container.getConfig().getCpuShares(), 
 container.getConfig().getCpuShares()))
 +  .build());
 +  // TODO Set up location properly
 +  LocationBuilder locationBuilder = new LocationBuilder();
 +  locationBuilder.description();
 +  locationBuilder.id();
 +  locationBuilder.scope(LocationScope.HOST);
 +  builder.location(locationBuilder.build());
 +  builder.status(toPortableStatus.apply(container.getState()));
 +  builder.imageId(container.getImage());
 +  builder.loginPort(getLoginPort(container));
 +  builder.publicAddresses(getPublicIpAddresses());
 +  builder.privateAddresses(getPrivateIpAddresses(container));
 +  
 builder.operatingSystem(OperatingSystem.builder().description(linux).family(OsFamily.LINUX).build());

But you have the `imageId` (you're setting it a few lines before). You could 
use that to get the corresponding `Image` from the `ImageSupplier` and populate 
the details of its operating system? That should be closer to the real one than 
the hardcoded Linux.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12953837

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Andrea Turli 
 +
 +   @Inject
 +   public DockerComputeServiceAdapter(DockerApi api) {
 +  this.api = checkNotNull(api, api);
 +   }
 +
 +   @Override
 +   public NodeAndInitialCredentialsContainer 
 createNodeWithGroupEncodedIntoName(String group, String name,
 + 
  Template template) {
 +  checkNotNull(template, template was null);
 +  checkNotNull(template.getOptions(), template options was null);
 +  DockerTemplateOptions templateOptions = 
 DockerTemplateOptions.class.cast(template.getOptions());
 +
 +  String imageId = checkNotNull(template.getImage().getId(), template 
 image id must not be null);
 +  String loginUser = 
 template.getImage().getDefaultCredentials().getUser();
 +  String loginUserPassword = 
 template.getImage().getDefaultCredentials().getPassword();

good catch @nacx! Do you think it is ok to set as default credentials 
root/password for all of the images as this is one of the assumptions to use 
jclouds-docker. wdyt?


---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12954268

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Andrea Turli 
 +   @SuppressWarnings(unchecked)
 +   @Override
 +   public R extends HttpRequest R bindToRequest(R request, Object input) {
 +  checkArgument(checkNotNull(input, input) instanceof File, this 
 binder is only valid for File!);
 +  checkNotNull(request, request);
 +
 +  File dockerFile = (File) input;
 +  File tmpDir = Files.createTempDir();
 +  final File targetFile = new File(tmpDir + File.separator + 
 Dockerfile);
 +  try {
 + Files.copy(dockerFile, targetFile);
 + File archive = Archives.tar(tmpDir, File.createTempFile(archive, 
 .tar));
 + FileInputStream data = new FileInputStream(archive);
 + Payload payload = Payloads.newInputStreamPayload(data);
 + 
 payload.getContentMetadata().setContentLength(data.getChannel().size());
 + payload.getContentMetadata().setContentType(MediaType.TEXT_PLAIN);

The doc says **Content-type** – should be set to `application/tar` but I'm 
not sure it is supported

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12952427

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +
 +   @Inject
 +   public DockerComputeServiceAdapter(DockerApi api) {
 +  this.api = checkNotNull(api, api);
 +   }
 +
 +   @Override
 +   public NodeAndInitialCredentialsContainer 
 createNodeWithGroupEncodedIntoName(String group, String name,
 + 
  Template template) {
 +  checkNotNull(template, template was null);
 +  checkNotNull(template.getOptions(), template options was null);
 +  DockerTemplateOptions templateOptions = 
 DockerTemplateOptions.class.cast(template.getOptions());
 +
 +  String imageId = checkNotNull(template.getImage().getId(), template 
 image id must not be null);
 +  String loginUser = 
 template.getImage().getDefaultCredentials().getUser();
 +  String loginUserPassword = 
 template.getImage().getDefaultCredentials().getPassword();

Having a deeper look, the Image will already have the default credentials 
populated with the value of the `ComputeServiceProperties.IMAGE_LOGIN_USER` 
value, so it should be enough.

However, you should take into account here that the user can have used the 
`templateOptions` to override the username, the password, or the private key. 
The login options provided in the `templateOptions` take preference over the 
image defaults.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12954809

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Ignasi Barrera 
 +   }
 +
 +   @Override
 +   public IterableLocation listLocations() {
 +  return ImmutableSet.of();
 +   }
 +
 +   @Override
 +   public Container getNode(String id) {
 +  return api.getRemoteApi().inspectContainer(id);
 +   }
 +
 +   @Override
 +   public void destroyNode(String id) {
 +  api.getRemoteApi().stopContainer(id);
 +  api.getRemoteApi().removeContainer(id);

Then we should try it and, if possible, just remove the stopContainer call or 
move the remove one to a finally block.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12954881

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-22 Thread Andrea Turli 
 +*/
 +   @Named(images:list)
 +   @GET
 +   @Path(/images/json)
 +   @Fallback(Fallbacks.EmptySetOnNotFoundOr404.class)
 +   SetImage listImages(ListImageOptions options);
 +
 +   /**
 +* Inspect an image
 +*
 +* @param imageId The id of the image to inspect.
 +* @return low-level information on the image name
 +*/
 +   @Named(image:inspect)
 +   @GET
 +   @Path(/images/{name}/json)

it's a strange api, it accepts `name` or `id` ... so I will stick with name

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12952760

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-21 Thread Andrew Kennedy 
 +  MapString, ? volumes, @Nullable String volumesFrom, @Nullable 
 String workingDir,
 +  @Nullable String entrypoint, @Nullable boolean networkDisabled, 
 @Nullable String onBuild) {
 +  this.hostname = hostname;
 +  this.domainName = domainName;
 +  this.user = user;
 +  this.memory = checkNotNull(memory, memory);
 +  this.memorySwap = checkNotNull(memorySwap, memorySwap);
 +  this.cpuShares = checkNotNull(cpuShares, cpuShares);
 +  this.attachStdin = checkNotNull(attachStdin, attachStdin);
 +  this.attachStdout = checkNotNull(attachStdout, attachStdout);
 +  this.attachStderr = checkNotNull(attachStderr, attachStderr);
 +  this.exposedPorts = exposedPorts != null ? 
 ImmutableMap.copyOf(exposedPorts) : ImmutableMap.String, Object of();
 +  this.tty = checkNotNull(tty, tty);
 +  this.openStdin = checkNotNull(openStdin, openStdin);
 +  this.stdinOnce = checkNotNull(stdinOnce, stdinOnce);
 +  this.env = env != null ? ImmutableList.copyOf(cmd) : 
 ImmutableList.String of();

Should be `ImmutableList.copyOf(env)` here...

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57/files#r12930244

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-05-21 Thread Andrew Kennedy 
@andreaturli there's a typo in `Config` but otherwise this looks good
@demobox any idea when this can be merged?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-43833767

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-17 Thread Andrew Phillips 
 jclouds » jclouds-labs #974 UNSTABLE
 jclouds-labs-pull-requests #138 UNSTABLE

Thanks for the update, @andreaturli! We now have [one test 
failure](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/org.apache.jclouds.labs$docker/138/testReport/junit/org.jclouds.docker.compute.functions/ContainerToNodeMetadataTest/testVirtualMachineToNodeMetadata/)
 and a bunch of [Checkstyle 
violations](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/138/org.apache.jclouds.labs$docker/violations/).
 Could you address those?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40725751

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-17 Thread Andrea Turli 
Thanks @demobox I will.

Not sure about the latest 2 checkstyle violations: any hint?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40726804

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-17 Thread BuildHive 
[jclouds » jclouds-labs 
#975](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/975/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40729059

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-17 Thread CloudBees pull request builder plugin 
[jclouds-labs-pull-requests 
#139](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/139/) 
SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40729306

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-17 Thread Andrew Phillips 
 Not sure about the latest 2 checkstyle violations: any hint?

Ah, yes...that one. The warning is not very helpful, I'm afraid :-( I think 
it's missing newline at end of file.

The place to verify that, by the way, is in the [Checkstyle result file in the 
workspace](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/ws/docker/target/checkstyle-result.xml).

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40754523

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-17 Thread Andrew Phillips 
PS: Thanks for the cleanup, of course, @andreaturli!

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40754567

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-17 Thread CloudBees pull request builder plugin 
[jclouds-labs-pull-requests 
#140](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/140/) 
FAILURE
Looks like there's a problem with this pull request

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40757727

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-17 Thread Andrew Phillips 
 jclouds-labs-pull-requests #140 FAILURE

Some internal DEV@cloud failure.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40758454

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-17 Thread BuildHive 
[jclouds » jclouds-labs 
#977](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/977/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40758682

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-12 Thread Andrea Turli 
@imesh this provider is still at its early stage but it covers all of the 
CRUD operations offerred by remote api for image management and container 
management. 

How do you plan to contribute? 

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40273258

Re: [jclouds-labs] [JCLOUDS-500] Initial commit for docker (#57)

2014-04-12 Thread Imesh Guaratne 
@andreaturli Thanks for the quick response! That's great! 
Let me give you some background on this, I'm a committer in Apache Stratos 
(incubating) project and Stratos is using jclouds for IaaS communication. We 
are now looking into supporting Docker in Stratos via jclouds. 

May be first I could checkout your pull request and try it out. I will update 
you after that.
Thanks

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/57#issuecomment-40281850

  1   2   >