jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrea Turli <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] Adding Dimension Data ServerImageAPI (#380)
Date Fri, 07 Apr 2017 10:28:00 GMT
andreaturli commented on this pull request.

thanks @trevorflanagan some minor comments from me but overall looks good to me

> + * 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.dimensiondata.cloudcontrol.domain;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import java.util.Date;
+import java.util.List;
+
+@AutoValue
+public abstract class CustomerImage extends BaseImage {

is it actually possible? I thought it wasn't because of https://github.com/google/auto/blob/master/value/userguide/howto.md#inherit
but may I misinterpreted

> + *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.dimensiondata.cloudcontrol.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+@AutoValue
+public abstract class Guest {
+   public abstract boolean osCustomization();

consider adding the empty constructor

> + * 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.dimensiondata.cloudcontrol.domain;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
+import org.jclouds.javax.annotation.Nullable;
+import org.jclouds.json.SerializedNames;
+
+import java.util.Date;
+import java.util.List;
+
+@AutoValue
+public abstract class CustomerImage extends BaseImage {

isn't the empty constructor recommended in the AutoValue classes

> @@ -24,47 +24,29 @@
 import java.util.List;
 
 @AutoValue
-public abstract class OsImage {
+public abstract class OsImage extends BaseImage {
+   public static final String TYPE = "OS_IMAGE";

same comment about empty constructor as above

> +   @Named("image:list")
+   @GET
+   @Path("/customerImage")
+   @Transform(ParseCustomerImages.ToPagedIterable.class)
+   @ResponseParser(ParseCustomerImages.class)
+   @RequestFilters(DatacenterIdFilter.class)
+   @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
+   PagedIterable<CustomerImage> listCustomerImages();
+
+   @Named("image:get")
+   @GET
+   @Path("/osImage/{id}")
+   @Fallback(Fallbacks.NullOnNotFoundOr404.class)
+   OsImage getOsImage(@PathParam("id") String id);
+
+   @Named("image:get")

[minor] do we want to apply same name as line 90 as this method will get the same config as
the above?

> +
+@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() {
+      // TODO need to add a valid id for this query

please don't commit commented code

> +   public void testGetOsImage() {
+      // TODO need to add a valid id for this query
+   }
+
+   @Test
+   public void testListCustomerImages() {
+      FluentIterable<CustomerImage> customerImages = api().listCustomerImages().concat();
+      assertNotNull(customerImages);
+      for (CustomerImage customerImage : customerImages) {
+         assertNotNull(customerImage);
+      }
+   }
+
+   @Test
+   public void testGetCustomerImage() {
+      // TODO need to add a valid id for this query

please don't commit commented code

> +
+   @Override
+   public String resource() {
+      return "/customerImages.json";
+   }
+
+   @Override
+   @Consumes(MediaType.APPLICATION_JSON)
+   public CustomerImages expected() {
+      List<CustomerImage> customerImages = ImmutableList
+            .of(CustomerImage.builder().id("f27b7ead-9cdc-4cee-be50-8f8e6cec8534").name("CloneForDrs")
+                  .cluster(Cluster.builder().id("QA1_N2_VMWARE_1-01").name("QA1_N2_VMWARE_1-01").build())
+                  .cpu(CPU.builder().count(2).speed("STANDARD").coresPerSocket(1).build()).memoryGb(4).disks(
+                        ImmutableList.of(Disk.builder().id("1bddd4ed-67dc-4e5e-a0d5-b5a6c012ec14").scsiId(0).sizeGb(50)
+                              .speed("HIGHPERFORMANCE").build()))
+                  .createTime(DatatypeConverter.parseDateTime("2016-07-17T23:53:48.000Z").getTime())

[mino] why not using the jclouds service 'SimpleDateFormatDateService' ?

-- 
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-31527546
Mime
View raw message