devcsrj requested changes on this pull request.

Also, could you post a copy of your the build results of this module in a gist, 
or a pastie?

P.S.: Take care, and make sure **not** to include your credentials!

> +      TemplateOptions options = template.getOptions();
+      final String loginUser = isNullOrEmpty(options.getLoginUser()) ? "root" 
: options.getLoginUser();
+      final String password = options.hasLoginPassword() ? 
options.getLoginPassword() : Passwords.generate();
+      final org.jclouds.compute.domain.Image image = template.getImage();
+
+      // provision all volumes based on hardware
+      List<? extends Volume> volumes = hardware.getVolumes();
+      List<String> volumeIds = 
Lists.newArrayListWithExpectedSize(volumes.size());
+
+      int i = 1;
+      for (final Volume volume : volumes) {
+         try {
+            logger.trace("<< provisioning volume '%s'", volume);
+            final 
org.apache.jclouds.profitbricks.rest.domain.Volume.Request.CreatePayload 
request = 
org.apache.jclouds.profitbricks.rest.domain.Volume.Request.creatingBuilder()
+                    .dataCenterId(dataCenterId)
+                    .image(image.getId())

Is it intentional that here we provision every volume with an image? (i.e.: If 
we attached 5 volumes, each has a bootable OS?)

> +         logger.trace(">> provisioning complete for server. returned 
> id='%s'", serverId);
+
+      } catch (Exception ex) {
+         logger.error(ex, ">> failed to provision server. rollbacking..");
+         destroyVolumes(volumeIds, dataCenterId);
+         throw Throwables.propagate(ex);
+      }
+
+      waitServerUntilAvailable.apply(ServerRef.create(dataCenterId, serverId));
+      waitDcUntilAvailable.apply(dataCenterId);
+
+      //attach bootVolume to Server
+      api.serverApi().attachVolume(Server.Request.attachVolumeBuilder()
+              .dataCenterId(dataCenterId)
+              .serverId(serverId)
+              .volumeId(bootVolume.id())

IIRC, if one creates a server (line 193) with the `bootVolume` param set, it'll 
be automatically attached after that server is provisioned. Is this not the 
case anymore?

> +         public List<Snapshot> call() throws Exception {
+            logger.trace("<< fetching snapshots");
+            List<Snapshot> remoteSnapshots = api.snapshotApi().list(new 
DepthOptions().depth(1));
+            logger.trace(">> snapshots feched.");
+
+            return remoteSnapshots;
+         }
+
+      });
+
+      return Iterables.concat(getUnchecked(images), getUnchecked(snapshots));
+   }
+
+   @Override
+   public Provisionable getImage(String id
+   ) {

Woops right paren

>     DE_FKB("de/fkb", "Germany, Karlsruhe"),
    DE_FRA("de/fra", "Germany, Frankfurt (M)"),
    US_LAS("us/las", "USA, Las Vegas"),
    US_LASDEV("us/lasdev", "USA Developer cluster"),
-   UNRECOGNIZED("unrecognized", "Unrecognized location");
+   UNRECOGNIZED("unrecognized", "Unrecognized location"),
+   Mock("mock", "mock");

[nit] All caps

>        return UNRECOGNIZED;
    }
+
+   @Override
+   public String toString() {
+      return id;

Make this more descriptive, something like:
```
   return "Location[id=" + id + ", description=" + description + "]";
```

..or don't override it at all, and use the actual `getId()` method, to print 
out the ID.

> @@ -61,9 +66,9 @@
             <scope>provided</scope>
         </dependency>
         <dependency>
-           <groupId>org.apache.jclouds.driver</groupId>
-           <artifactId>jclouds-okhttp</artifactId>
-           <version>${jclouds.version}</version>
+            <groupId>org.apache.jclouds.driver</groupId>
+            <artifactId>jclouds-okhttp</artifactId>
+            <version>${jclouds.version}</version>

[nit] Formatting

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/292#pullrequestreview-884467

Reply via email to