nacx requested changes on this pull request.

Looks like a great start, thanks @andreaturli!

Once this is cleaned up and tests passing the next steps could be (if I'm not 
wrong):

* Remove the method that creates an storage service in the base live test class 
and update the test methods accordingly.
* Update the image extension with the help of @danielestevez's changes.
* Remoe the azureblob dependency and remove all code that accessed the storage 
accounts manually.

Is this kinda the plan you had in mind?

>        VMImage vmImage;
+      String[] fields = checkNotNull(id, "id").split("/");
       boolean custom = fields.length == 5;

Change this to a public static function that can be called from the adapter, 
instead of duplicating this logic there.

> @@ -33,6 +36,7 @@
    private String blob;
    private AvailabilitySet availabilitySet;
    private String availabilitySetName;
+   protected List<Map<String, String>> dataDisks = ImmutableList.of();

Better create an object to carry the information of the data disks? A `Map` 
looks quite inconvenient and too error-prone, and also harder to document and 
use by end users.

> @@ -100,44 +117,54 @@ public void copyTo(TemplateOptions to) {
          eTo.blob(blob);
          eTo.availabilitySet(availabilitySet);
          eTo.availabilitySet(availabilitySetName);
+         if (!dataDisks.isEmpty()) {

Redundant check. We allow passing an empty list.

> @@ -181,5 +208,18 @@ public static AzureTemplateOptions 
> availabilitySet(String availabilitySetName) {
          AzureTemplateOptions options = new AzureTemplateOptions();
          return options.availabilitySet(availabilitySetName);
       }
+
+      /**
+       * @see AzureTemplateOptions#dataDisks
+       */
+      public static AzureTemplateOptions blockDevices(Map<String, String>... 
dataDisks) {

Why not naming this `dataDisks` if that is the naming used in Azure?

> @@ -136,8 +128,8 @@ protected CreateResourceGroupThenCreateNodes(
       configureSecurityGroupForOptions(group, azureGroupName, 
template.getLocation(), options);
       configureAvailabilitySetForTemplate(template);
 
-      StorageService storageService = getOrCreateStorageService(group, 
azureGroupName, location, template.getImage());
-      
options.blob(storageService.storageServiceProperties().primaryEndpoints().get("blob"));
+      //StorageService storageService = getOrCreateStorageService(group, 
azureGroupName, location, template.getImage());
+      
//options.blob(storageService.storageServiceProperties().primaryEndpoints().get("blob"));

Remove all commented code.

> + * 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.azurecompute.arm.domain;
+
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import com.google.auto.value.AutoValue;
+
+@AutoValue
+public abstract class CreationData {
+
+   @Nullable
+   public abstract String createOption();

Is there an enum we could use here?

> +
+      /** The actual serialized value for a DiskCreateOptionTypes instance. */
+      private String value;
+
+      DiskCreateOptionTypes(String value) {
+         this.value = value;
+      }
+
+      public static DiskCreateOptionTypes fromString(String value) {
+         DiskCreateOptionTypes[] items = DiskCreateOptionTypes.values();
+         for (DiskCreateOptionTypes item : items) {
+            if (item.toString().equalsIgnoreCase(value)) {
+               return item;
+            }
+         }
+         return null;

Better throw an IAE. What is the purpose of returning `null`?

> +
+      /** The actual serialized value for a CachingTypes instance. */
+      private String value;
+
+      CachingTypes(String value) {
+         this.value = value;
+      }
+
+      public static CachingTypes fromString(String value) {
+         CachingTypes[] items = CachingTypes.values();
+         for (CachingTypes item : items) {
+            if (item.toString().equalsIgnoreCase(value)) {
+               return item;
+            }
+         }
+         return null;

Better throw an IAE. What is the purpose of returning `null`?

> +
+   public abstract Builder toBuilder();
+
+   public static Builder builder() {
+      return new AutoValue_Disk.Builder();
+   }
+
+   @AutoValue.Builder
+   public abstract static class Builder {
+      public abstract Builder id(String id);
+      public abstract Builder name(String name);
+      public abstract Builder location(String location);
+      public abstract Builder type(String type);
+      public abstract Builder sku(SKU sku);
+      public abstract Builder properties(DiskProperties properties);
+      public abstract Builder tags(Map<String, String>  tags);

Make sure this map is immutable following 
[this](https://github.com/jclouds/jclouds-labs/blob/master/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/AvailabilitySet.java#L82-L90)
 pattern.

> +   @SerializedNames({"location", "properties", "tags"})
+   public static Image create(final String location, final ImageProperties 
properties, final Map<String, String> tags) {
+      return 
builder().location(location).properties(properties).tags(tags).build();
+   }
+
+   public abstract Builder toBuilder();
+
+   public static Builder builder() {
+      return new AutoValue_Image.Builder();
+   }
+
+   @AutoValue.Builder
+   public abstract static class Builder {
+      public abstract Builder location(String location);
+      public abstract Builder properties(ImageProperties properties);
+      public abstract Builder tags(Map<String, String>  tags);

Make immutable.

> +      Disk dataDisk = api().createOrUpdate(diskName, LOCATION, properties);
+      assertTrue(stateReached("Succeeded"), "start operation did not complete 
in the configured timeout");
+      assertTrue(dataDisk.properties().diskSizeGB() == 2);
+   }
+
+   @Test(dependsOnMethods = "createDisk")
+   public void getDisk() {
+      Disk dataDisk = api().get(diskName);
+      assertNotNull(dataDisk.name());
+      assertTrue(dataDisk.properties().diskSizeGB() == 2);
+   }
+
+   @Test(dependsOnMethods = "createDisk")
+   public void listDisks() {
+      List<Disk> dataDisks = api().list();
+      assertTrue(dataDisks.size() > 0);

Verify that the created disk is there. We don't care about others? Just in case 
the RG is not empty.

> +   private DiskApi api() {
+      return api.getDiskApi(resourceGroupName);
+   }
+
+   private boolean waitForState(String name, final String state) {
+      return api().get(name).properties().provisioningState().equals(state);
+   }
+
+   private boolean stateReached(final String state) {
+      return retry(new Predicate<String>() {
+         @Override
+         public boolean apply(String name) {
+            return waitForState(name, state);
+         }
+      }, 60 * 4 * 1000).apply(diskName);
+   }

Better change these two methods to use the already available predicate to poll 
Provisionable instances:

```java
 private boolean waitUntilAvailable(String name) {
   return resourceAvailable.apply(new Supplier<Provisionable>() {
      @Override public Provisionable get() {
           Disk disk api().get(name);
           return disk == null ? null : disk.properties();
      }
   });
}
```

> +      super.setup();
+      createTestResourceGroup();
+      imageName = "jclouds-vm-image-" + RAND;
+      String vmName = "jclouds-vm-" + RAND;
+
+      virtualMachine = 
api.getVirtualMachineApi(resourceGroupName).createOrUpdate(vmName, LOCATION, 
VirtualMachineProperties.builder().build(),
+              Collections.<String, String> emptyMap(), null);
+   }
+
+   @Test
+   public void deleteImageResourceDoesNotExist() {
+      assertNull(api().delete(imageName));
+   }
+
+   @Test(dependsOnMethods = "deleteImageResourceDoesNotExist")
+   public void CreateVirtualMachineImageFromExistingVM() {

[minor] start lowercase

> +   public void setup() {
+      super.setup();
+      createTestResourceGroup();
+      imageName = "jclouds-vm-image-" + RAND;
+      String vmName = "jclouds-vm-" + RAND;
+
+      virtualMachine = 
api.getVirtualMachineApi(resourceGroupName).createOrUpdate(vmName, LOCATION, 
VirtualMachineProperties.builder().build(),
+              Collections.<String, String> emptyMap(), null);
+   }
+
+   @Test
+   public void deleteImageResourceDoesNotExist() {
+      assertNull(api().delete(imageName));
+   }
+
+   @Test(dependsOnMethods = "deleteImageResourceDoesNotExist")

Remove this test dependency. Make the delete unexisting try to delete a 
randomly generated name, but we don't want the tests to be skipped because of 
that method failing.

> +   private String diskName;
+
+   @BeforeClass
+   @Override
+   public void setup() {
+      super.setup();
+      createTestResourceGroup();
+      diskName = "jclouds-" + RAND;
+   }
+
+   @Test
+   public void deleteDiskResourceDoesNotExist() {
+      assertNull(api().delete(diskName));
+   }
+
+   @Test(dependsOnMethods = "deleteDiskResourceDoesNotExist")

Remove this test dependency. Make the delete unexisting try to delete a 
randomly generated name, but we don't want the tests to be skipped because of 
that method failing.

> +   private ImageApi api() {
+      return api.getVirtualMachineImageApi(resourceGroupName);
+   }
+
+   private boolean waitForState(String name, final String state) {
+      return api().get(name).properties().provisioningState().equals(state);
+   }
+
+   private boolean stateReached(final String state) {
+      return retry(new Predicate<String>() {
+         @Override
+         public boolean apply(String name) {
+            return waitForState(name, state);
+         }
+      }, 60 * 4 * 1000).apply(imageName);
+   }

Same comment about using the already existing predicate to poll provisionables.

-- 
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/372#pullrequestreview-27786837

Reply via email to