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] [JCLOUDS-1430] Aliyun ECS (#443)
Date Mon, 06 Aug 2018 09:31:46 GMT
nacx requested changes on this pull request.



> +            <artifactId>annotations-java5</artifactId>
+            <version>RELEASE</version>
+            <scope>compile</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.jetbrains</groupId>
+            <artifactId>annotations-java5</artifactId>
+            <version>RELEASE</version>
+            <scope>compile</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.jetbrains</groupId>
+            <artifactId>annotations-java5</artifactId>
+            <version>RELEASE</version>
+            <scope>compile</scope>
+        </dependency>

Remove all this

>                        .instanceName(name)
                       .keyPairName(keyPairName)
                       .tagOptions(tagOptions)
       );
-      String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
-      instanceSuspendedPredicate.apply(regionAndInstanceId);
+      String regionAndInstanceId = slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+      if (!instanceSuspendedPredicate.apply(regionAndInstanceId)) {

Add some log here explaining what went wrong.

>     }
 
    @Override
-   public Image getImage(final String id) {
-      Optional<Image> firstInterestingImage = Iterables.tryFind(listImages(), new Predicate<Image>()
{
-         public boolean apply(Image input) {
-            return input.id().equals(id);
-         }
-      });
-      if (!firstInterestingImage.isPresent()) {
-         throw new IllegalStateException("Cannot find image with the required slug " + id);
-      }
-      return firstInterestingImage.get();
+   public ImageInRegion getImage(final String id) {
+      RegionAndId regionAndId = fromSlashEncoded(id);
+      return ImageInRegion.create(regionAndId.regionId(), Iterables.getFirst(api.imageApi().list(regionAndId.regionId(),

This returns an object even if the image does not exist. That-s not correct. Just return `null`.

> @@ -54,6 +46,7 @@ protected Properties setupProperties() {
       Properties properties = super.setupProperties();
       vpcId = setIfTestSystemPropertyPresent(properties,  provider + ".vpcId");
       vSwitchId = setIfTestSystemPropertyPresent(properties,  provider + ".vSwitchId");
+      group = "jclouds"; // otherwise jclouds will use provider name as group but `aliyun`
is a forbidden prefix for tags

Then we should probably preconfigure the `prefix` property in the provider metadata too, so
users don't have to do it manually. We must provide defaults that work.

> +//      expect(serverApi.getServer(serverId)).andReturn(server);
+//   }
+//
+//   private void applyWithExpectedErrorMessage(String expectedErrorMessage) {
+//      try {
+//         cleanupServer.apply(serverId);
+//      } catch (IllegalStateException e) {
+//         assertEquals(expectedErrorMessage, e.getMessage());
+//      }
+//   }
+//
+//   private void networkApiExpectations() {
+//      expect(api.getNetworkApi()).andReturn(networkApi);
+//   }
+//
+//}

Why is all this commented?

> +
+      final Image image = imageInRegionToImage.apply(ImageInRegion.create(Regions.EU_CENTRAL_1.getName(),
ecsImage));
+      assertEquals(ecsImage.id(), image.getProviderId());
+      assertEquals(ecsImage.name(), image.getName());
+      assertEquals(Image.Status.AVAILABLE, image.getStatus());
+      final org.jclouds.compute.domain.OperatingSystem operatingSystem = image.getOperatingSystem();
+
+      assertEquals(ecsImage.osName(), operatingSystem.getName());
+      assertEquals(ecsImage.description(), operatingSystem.getDescription());
+      assertTrue(operatingSystem.is64Bit());
+      assertEquals(region, image.getLocation());
+   }
+
+   Date parseDate(final String dateString) {
+      return DatatypeConverter.parseDateTime(dateString).getTime();
+   }

Add tests that check the special cases such as the `OTHER_OS_MAP`.

> +              .resourceGroupId("resourceGroupId")
+              .osType("osType")
+              .osName("osName")
+              .instanceNetworkType("instanceNetworkType")
+              .hostname("hostname")
+              .creationTime(new SimpleDateFormatDateService().iso8601DateParse("2014-03-22T05:16:45.784120972Z"))
+              .status(Instance.Status.RUNNING)
+              .clusterId("clusterId")
+              .recyclable(false)
+              .gpuSpec("")
+              .dedicatedHostAttribute(DedicatedHostAttribute.create("id", "name"))
+              .instanceChargeType("instanceChargeType")
+              .gpuAmount(1)
+              .expiredTime(new SimpleDateFormatDateService().iso8601DateParse("2014-03-22T09:16:45.784120972Z"))
+              .innerIpAddress(ImmutableMap.<String, List<String>>of("IpAddress",
ImmutableList.of("192.168.0.1")))
+              .publicIpAddress(ImmutableMap.<String, List<String>>of("IpAddress",
ImmutableList.of("47.254.152.220")))

Add more values to the addresses to make sure to test we them all.

> +   }
+
+   private void assertNodeMetadata(NodeMetadata result, org.jclouds.compute.domain.OperatingSystem
os, String imageId,
+                                   NodeMetadata.Status status, List<String> privateIpAddresses,
List<String> publicIpAddresses) {
+      assertNotNull(result);
+      assertEquals(result.getProviderId(), instance.id());
+      assertEquals(result.getName(), instance.name());
+      assertEquals(result.getHostname(), instance.hostname());
+      assertEquals(result.getGroup(), "[" + serverName + "]");
+      assertEquals(result.getHardware(), hardware);
+      assertEquals(result.getOperatingSystem(), os);
+      assertEquals(result.getLocation(), location);
+      assertEquals(result.getImageId(), RegionAndId.slashEncodeRegionAndId(regionId, imageId));
+      assertEquals(result.getStatus(), status);
+      assertEquals(result.getPrivateAddresses(), privateIpAddresses);
+      assertEquals(result.getPublicAddresses(), publicIpAddresses);

There is custom logic to extract the tags. Make sure you test tags too.
Also add a couple tests to verify what happens when the image and hardware are not found.

> +import org.jclouds.logging.Logger;
+import org.jclouds.rest.AuthorizationException;
+
+import javax.annotation.Resource;
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+import java.util.List;
+
+import static org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_SUSPENDED;
+import static org.jclouds.compute.config.ComputeServiceProperties.TIMEOUT_NODE_TERMINATED;
+
+@Singleton
+public class CleanupResources {
+
+   public static final String RESOURCE_TYPE = "securitygroup";

Rename this.

> +@AutoValue
+public abstract class EipAddress {
+
+   EipAddress() {
+   }
+
+   @SerializedNames({ "IpAddress", "AllocationId", "InternetChargeType" })
+   public static EipAddress create(String ipAddress, String allocationId, String internetChargeType)
{
+      return new AutoValue_EipAddress(ipAddress, allocationId, internetChargeType);
+   }
+
+   public abstract String ipAddress();
+
+   public abstract String allocationId();
+
+   public abstract String internetChargeType();

Are these values fixed so we can have them in an enum?

> +   public void testListInstances() throws InterruptedException {
+      server.enqueue(jsonResponse("/instances-first.json"));
+      server.enqueue(jsonResponse("/instances-last.json"));
+
+      Iterable<Instance> instances = api.instanceApi().list(Regions.EU_CENTRAL_1.getName()).concat();
+      assertEquals(size(instances), 20); // Force the PagedIterable to advance
+      assertEquals(server.getRequestCount(), 2);
+      assertSent(server, "GET", "DescribeInstances");
+      assertSent(server, "GET", "DescribeInstances", 2);
+   }
+
+   public void testListInstancesReturns404() {
+      server.enqueue(response404());
+      Iterable<Instance> instances = api.instanceApi().list(Regions.EU_CENTRAL_1.getName()).concat();
+      assertTrue(isEmpty(instances));
+      assertEquals(server.getRequestCount(), 1);

Still not addressed

> +
+   public void testListInstanceTypes() throws InterruptedException {
+      server.enqueue(jsonResponse("/instanceTypes.json"));
+
+      List<InstanceType> instanceTypes = api.instanceApi().listTypes();
+      assertEquals(size(instanceTypes), 308);
+      assertEquals(server.getRequestCount(), 1);
+      assertSent(server, "GET", "DescribeInstanceTypes");
+   }
+
+   public void testListInstanceTypesReturns404() {
+      server.enqueue(response404());
+      List<InstanceType> instanceTypes = api.instanceApi().listTypes();
+      assertTrue(isEmpty(instanceTypes));
+      assertEquals(server.getRequestCount(), 1);
+   }

Still not addressed.

-- 
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/443#pullrequestreview-143506207
Mime
View raw message