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
