jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aled Sage <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] edit VirtualMachineApi.create to createOrUpdate (#368)
Date Thu, 02 Mar 2017 23:10:51 GMT
aledsage commented on this pull request.

LGTM - only a couple of very minor comments.

> +      VirtualMachineProperties oldProperties = vm.properties();
+      StorageProfile oldStorageProfile = oldProperties.storageProfile();
+
+      String blob = storageService.storageServiceProperties().primaryEndpoints().get("blob");
+      VHD vhd = VHD.create(blob + "vhds/" + vmName + "new-data-disk.vhd");
+      DataDisk newDataDisk = DataDisk.create(vmName + "new-data-disk", "1", 1, vhd, "Empty");
+      List<DataDisk> oldDataDisks = oldStorageProfile.dataDisks();
+
+      ImmutableList<DataDisk> newDataDisks = ImmutableList.<DataDisk> builder().addAll(oldDataDisks).add(newDataDisk).build();
+      StorageProfile newStorageProfile = oldStorageProfile.toBuilder().dataDisks(newDataDisks).build();
+      VirtualMachineProperties newProperties = oldProperties.toBuilder().storageProfile(newStorageProfile).build();
+
+      VirtualMachine newVm = vm.toBuilder().properties(newProperties).build();
+      vm = api().createOrUpdate(vmName, newVm.location(), newVm.properties(), newVm.tags(),
newVm.plan());
+
+      assertEquals(vm.properties().storageProfile().dataDisks().size(), 2);

Perhaps assert it's equal to `oldDataDisks.size() + 1` (or assert earlier that the oldDataDisks
size is 1)?

> @@ -16,11 +16,6 @@
  */
 package org.jclouds.azurecompute.arm.features;
 
-import static org.assertj.core.api.Assertions.assertThat;

Would be nice if the PR didn't move the imports around, but no strong feelings from me.

> @@ -88,11 +88,11 @@
    @Path("/{vmname}")
    @QueryParams(keys = "validating", values = "false")
    @Produces(MediaType.APPLICATION_JSON)
-   VirtualMachine create(@PathParam("vmname") String vmname,
-                         @PayloadParam("location") String location,
-                         @PayloadParam("properties") VirtualMachineProperties properties,
-                         @PayloadParam("tags") Map<String, String> tags,
-                         @Nullable @PayloadParam("plan") Plan plan);
+   VirtualMachine createOrUpdate(@PathParam("vmname") String vmname,

Could change javadoc of this method as well (not that the javadoc on most of these methods
is particularly helpful, e.g. `start()`'s javadoc says "The start Virtual Machine operation"!)?

-- 
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/368#pullrequestreview-24868205
Mime
View raw message