jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] Adding Dimension Data ServerImageAPI (#380)
Date Mon, 10 Apr 2017 13:51:06 GMT
nacx requested changes on this pull request.

Thanks @trevorflanagan! Just a couple of (hopefully) small comments.

> +
+      public abstract Builder datacenterId(String datacenterId);
+
+      public abstract Builder name(String name);
+
+      public abstract Builder description(String description);
+
+      public abstract Builder cluster(Cluster cluster);
+
+      public abstract Builder guest(Guest guest);
+
+      public abstract Builder cpu(CPU cpu);
+
+      public abstract Builder memoryGb(int memoryGb);
+
+      public abstract Builder nics(List<ImageNic> disks);

fix parameter name

> +
+      public abstract Builder description(String description);
+
+      public abstract Builder cluster(Cluster cluster);
+
+      public abstract Builder guest(Guest guest);
+
+      public abstract Builder cpu(CPU cpu);
+
+      public abstract Builder memoryGb(int memoryGb);
+
+      public abstract Builder nics(List<ImageNic> disks);
+
+      public abstract Builder disks(List<Disk> disks);
+
+      public abstract Builder tags(List<TagWithIdAndName> disks);

fix parameter name

> +
+      public abstract Builder progress(Progress progress);
+
+      abstract CustomerImage autoBuild();
+
+      abstract List<Disk> disks();
+
+      abstract List<String> softwareLabels();
+
+      abstract List<ImageNic> nics();
+
+      public CustomerImage build() {
+         disks(disks() != null ? ImmutableList.copyOf(disks()) : ImmutableList.<Disk>of());
+         softwareLabels(softwareLabels() != null ? ImmutableList.copyOf(softwareLabels())
: ImmutableList.<String>of());
+         nics(nics() != null ? ImmutableList.copyOf(nics()) : ImmutableList.<ImageNic>of());
+         return autoBuild();

Make tags immutable too.

> +import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+@AutoValue
+public abstract class VmTools {
+
+   VmTools() {
+   }
+
+   public abstract String type();
+
+   @Nullable
+   public abstract String versionStatus();
+
+   @Nullable
+   public abstract String runningStatus();

Does this have known values we could model in an enum?

> +
+   VmTools() {
+   }
+
+   public abstract String type();
+
+   @Nullable
+   public abstract String versionStatus();
+
+   @Nullable
+   public abstract String runningStatus();
+
+   @Nullable
+   public abstract Integer apiVersion();
+
+   public abstract Builder toBuilder();

For consistency in the domain model it would be worth unifying the model: add the `toBuilder`
method to all objects (some are missing) as long as adding the default empty constructor to
all them too (some objects are missing it, especially inner ones).

> +   @Named("image:getOsImage")
+   @GET
+   @Path("/osImage/{id}")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   OsImage getOsImage(@PathParam("id") String id);
+
+   @Named("image:getCustomerImage")
+   @GET
+   @Path("/customerImage/{id}")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   CustomerImage getCustomerImage(@PathParam("id") String id);
+
+   final class ParseOsImages extends ParseJson<OsImages> {
+
+      @Inject
+      public ParseOsImages(Json json) {

Remove the public modifier.

> +   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   CustomerImage getCustomerImage(@PathParam("id") String id);
+
+   final class ParseOsImages extends ParseJson<OsImages> {
+
+      @Inject
+      public ParseOsImages(Json json) {
+         super(json, TypeLiteral.get(OsImages.class));
+      }
+
+      public static class ToPagedIterable extends ArgsToPagedIterable<OsImage, ToPagedIterable>
{
+
+         private DimensionDataCloudControlApi api;
+
+         @Inject
+         public ToPagedIterable(DimensionDataCloudControlApi api) {

Remove the public modifier

> +import com.google.common.base.Supplier;
+import org.jclouds.http.HttpException;
+import org.jclouds.http.HttpRequest;
+import org.jclouds.http.HttpRequestFilter;
+import org.jclouds.location.Zone;
+
+import javax.inject.Inject;
+import java.util.Set;
+
+/**
+ * Adds set of Datacenter IDs as set in jclouds.zones JVM property.
+ */
+public class DatacenterIdFilter implements HttpRequestFilter {
+   @Inject
+   @Zone
+   protected Supplier<Set<String>> datacenterIdsSupplier;

Prefer the constructor injection and declare the variables final.

> +
+@Test(groups = "live", testName = "ServerImageApiLiveTest", singleThreaded = true)
+public class ServerImageApiLiveTest extends BaseDimensionDataCloudControlApiLiveTest {
+
+   @Test
+   public void testListOsImages() {
+      FluentIterable<OsImage> osImages = api().listOsImages().concat();
+      assertNotNull(osImages);
+      for (OsImage osImage : osImages) {
+         assertNotNull(osImage);
+      }
+   }
+
+   @Test
+   public void testGetOsImage() {
+   }

Implement this

> +   @Test
+   public void testGetOsImage() {
+   }
+
+   @Test
+   public void testListCustomerImages() {
+      FluentIterable<CustomerImage> customerImages = api().listCustomerImages().concat();
+      assertNotNull(customerImages);
+      for (CustomerImage customerImage : customerImages) {
+         assertNotNull(customerImage);
+      }
+   }
+
+   @Test
+   public void testGetCustomerImage() {
+   }

Implement this if there is any customer image to get.

> +                              .build()).vmTools(
+                        VmTools.builder().versionStatus("CURRENT").runningStatus("NOT_RUNNING").apiVersion(9354)
+                              .type("VMWARE_TOOLS").build()).osCustomization(true).build())
+                  .virtualHardware(VirtualHardware.builder().version("vmx-08").upToDate(false).build())
+                  .tags(ImmutableList.of(CustomerImage.TagWithIdAndName.builder().tagKeyName("DdTest3")
+                              .tagKeyId("ee58176e-305b-4ec2-85e0-330a33729a94").build(),
+                        CustomerImage.TagWithIdAndName.builder().tagKeyName("Lukas11")
+                              .tagKeyId("c5480364-d3cd-4391-9536-5c1af683a8f1").value("j").build(),
+                        CustomerImage.TagWithIdAndName.builder().tagKeyName("Lukas5")
+                              .tagKeyId("a3e869df-6427-404f-99c2-b50f526369aa").build()))
+                  .softwareLabels(ImmutableList.<String>of()).nics(ImmutableList.<ImageNic>of()).source(
+                        CustomerImage.Source.builder().artifacts(ImmutableList
+                              .of(CustomerImage.Artifact.builder().value("cb4b8674-09a4-4194-9593-9cdc81489de1")
+                                    .type("SERVER_ID").build())).type("CLONE").build()).build());
+      return new CustomerImages(customerImages, 1, 2, 2, 250);
+   }

Add a specific check to verify the "type" variable we are setting manually in the constructor?
(Or am I missing the check if it is already done?). Do the same in the other parse test.

> +import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
+
+/**
+ * Mock tests for the {@link ServerImageApi} class.
+ */
+@Test(groups = "unit", testName = "ServerImageApiMockTest", singleThreaded = true)
+public class ServerImageApiMockTest extends BaseAccountAwareCloudControlMockTest {
+
+   public void testGetOsImage() throws Exception {
+      server.enqueue(jsonResponse("/osImage.json"));
+      OsImage osImage = api.getServerImageApi().getOsImage("id");
+      assertNotNull(osImage);
+      assertSent(HttpMethod.GET, getImageUrl().appendPath("/osImage/id").toString());
+   }

Configure the tests and assertions in this class to properly verify that the `DatacenterIdFilter`
is applied where needed.

-- 
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/380#pullrequestreview-31843161
Mime
View raw message