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
