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 Wed, 01 Aug 2018 16:29:22 GMT
nacx commented on this pull request.

Thanks, @andreaturli!

There are many comments but in general, it looks pretty good.

Apart from the comments, we need proper tests for most of the things. Basically, every class
in the `compute/functions` package needs to have a proper unit test.
Also, add unit tests for every utility class and small function you have created.

> +Aliyun ECS provider works exactly as any other jclouds provider.
+Notice that as Aliyun supports dozens of locations and to limit the scope of some operations,
one may want to use:
+
+and
+```bash
+jclouds.regions
+```
+which is by default `null`. If you want to target only the `north europe` region, you can
use
+
+```bash
+jclouds.regions="eu-central-1"
+```
+
+# Setting Up Test Environment
+
+Get or create the `User Access Key` and `Access Key Secret	`for your account at `https://usercenter.console.aliyun.com/#/manage/ak`

[nit] Remove the extra spaces after "secret"

> +      this.instanceSuspendedPredicate = instanceSuspendedPredicate;
+      this.regionIds = regionIds;
+      this.cleanupResources = cleanupResources;
+   }
+
+   @Override
+   public NodeAndInitialCredentials<Instance> createNodeWithGroupEncodedIntoName(String
group, String name, Template template) {
+      String instanceType = template.getHardware().getId();
+      String regionId = template.getLocation().getId();
+      String imageId = template.getImage().getId();
+
+      ECSServiceTemplateOptions templateOptions = template.getOptions().as(ECSServiceTemplateOptions.class);
+
+      String keyPairName = templateOptions.getKeyPairName();
+      String securityGroupId = Iterables.getOnlyElement(templateOptions.getGroups());
+      String vSwitchId = templateOptions.getVSwitchId();

This seems to be mandatory, but its presence is not enforced anywhere. Should we validate
it is there in the create nodes strategy and fail with an appropriate message if missing?
It is not good to have mandatory options that are not part of the abstraction. It would be
better if we could automatically create one vSwitch or have a default value instead of requiring
users to always provide this particular template option.

> +      String instanceType = template.getHardware().getId();
+      String regionId = template.getLocation().getId();
+      String imageId = template.getImage().getId();
+
+      ECSServiceTemplateOptions templateOptions = template.getOptions().as(ECSServiceTemplateOptions.class);
+
+      String keyPairName = templateOptions.getKeyPairName();
+      String securityGroupId = Iterables.getOnlyElement(templateOptions.getGroups());
+      String vSwitchId = templateOptions.getVSwitchId();
+      Map<String, String> tags = tagsAsValuesOfEmptyString(templateOptions);
+      TagOptions tagOptions = TagOptions.Builder.keys(tags.keySet());
+
+      InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId,
name, instanceType,
+              CreateInstanceOptions.Builder
+                      .vSwitchId(vSwitchId)
+                      .internetChargeType("PayByTraffic")

The two charge field types should be custom template options.

> +      String regionId = template.getLocation().getId();
+      String imageId = template.getImage().getId();
+
+      ECSServiceTemplateOptions templateOptions = template.getOptions().as(ECSServiceTemplateOptions.class);
+
+      String keyPairName = templateOptions.getKeyPairName();
+      String securityGroupId = Iterables.getOnlyElement(templateOptions.getGroups());
+      String vSwitchId = templateOptions.getVSwitchId();
+      Map<String, String> tags = tagsAsValuesOfEmptyString(templateOptions);
+      TagOptions tagOptions = TagOptions.Builder.keys(tags.keySet());
+
+      InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId,
name, instanceType,
+              CreateInstanceOptions.Builder
+                      .vSwitchId(vSwitchId)
+                      .internetChargeType("PayByTraffic")
+                      .internetMaxBandwidthOut(5)

Same here.

> +      String vSwitchId = templateOptions.getVSwitchId();
+      Map<String, String> tags = tagsAsValuesOfEmptyString(templateOptions);
+      TagOptions tagOptions = TagOptions.Builder.keys(tags.keySet());
+
+      InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId,
name, instanceType,
+              CreateInstanceOptions.Builder
+                      .vSwitchId(vSwitchId)
+                      .internetChargeType("PayByTraffic")
+                      .internetMaxBandwidthOut(5)
+                      .instanceChargeType("PostPaid")
+                      .instanceName(name)
+                      .keyPairName(keyPairName)
+                      .tagOptions(tagOptions)
+      );
+      String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+      instanceSuspendedPredicate.apply(regionAndInstanceId);

You want to check the result of the predicate and fail if it returned `false`.

> +
+      InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId,
name, instanceType,
+              CreateInstanceOptions.Builder
+                      .vSwitchId(vSwitchId)
+                      .internetChargeType("PayByTraffic")
+                      .internetMaxBandwidthOut(5)
+                      .instanceChargeType("PostPaid")
+                      .instanceName(name)
+                      .keyPairName(keyPairName)
+                      .tagOptions(tagOptions)
+      );
+      String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+      instanceSuspendedPredicate.apply(regionAndInstanceId);
+      api.instanceApi().allocatePublicIpAddress(regionId, instanceRequest.getInstanceId());
+      api.instanceApi().powerOn(instanceRequest.getInstanceId());
+      instanceRunningPredicate.apply(regionAndInstanceId);

You want to check the result of the predicate and fail if it returned `false`.

> +      String vSwitchId = templateOptions.getVSwitchId();
+      Map<String, String> tags = tagsAsValuesOfEmptyString(templateOptions);
+      TagOptions tagOptions = TagOptions.Builder.keys(tags.keySet());
+
+      InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId,
name, instanceType,
+              CreateInstanceOptions.Builder
+                      .vSwitchId(vSwitchId)
+                      .internetChargeType("PayByTraffic")
+                      .internetMaxBandwidthOut(5)
+                      .instanceChargeType("PostPaid")
+                      .instanceName(name)
+                      .keyPairName(keyPairName)
+                      .tagOptions(tagOptions)
+      );
+      String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+      instanceSuspendedPredicate.apply(regionAndInstanceId);

Also, when failing after having called the instance create API, we should rollback all the
created resources.

> +
+      InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId,
name, instanceType,
+              CreateInstanceOptions.Builder
+                      .vSwitchId(vSwitchId)
+                      .internetChargeType("PayByTraffic")
+                      .internetMaxBandwidthOut(5)
+                      .instanceChargeType("PostPaid")
+                      .instanceName(name)
+                      .keyPairName(keyPairName)
+                      .tagOptions(tagOptions)
+      );
+      String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+      instanceSuspendedPredicate.apply(regionAndInstanceId);
+      api.instanceApi().allocatePublicIpAddress(regionId, instanceRequest.getInstanceId());
+      api.instanceApi().powerOn(instanceRequest.getInstanceId());
+      instanceRunningPredicate.apply(regionAndInstanceId);

Failing here should rollback the public ip too.

> +              transform(listLocations(), new Function<Region, String>() {
+                 @Override
+                 public String apply(Region location) {
+                    return location.id();
+                 }
+              }));
+
+      for (String regionId : availableLocationNames) {
+         instanceTypeIds.addAll(getInstanceTypeId(regionId));
+      }
+
+      List<InstanceType> instanceTypes = FluentIterable.from(api.instanceApi().listTypes())
+              .filter(new Predicate<InstanceType>() {
+                 @Override
+                 public boolean apply(@Nullable InstanceType input) {
+                    return contains(instanceTypeIds.build(), input.instanceTypeId());

Don't call `build` in every iteration. Build the list outside.

> +      for (String regionId : availableLocationNames) {
+         instanceTypeIds.addAll(getInstanceTypeId(regionId));
+      }
+
+      List<InstanceType> instanceTypes = FluentIterable.from(api.instanceApi().listTypes())
+              .filter(new Predicate<InstanceType>() {
+                 @Override
+                 public boolean apply(@Nullable InstanceType input) {
+                    return contains(instanceTypeIds.build(), input.instanceTypeId());
+                 }
+              }).toList();
+
+      return instanceTypes;
+   }
+
+   private List<String> getInstanceTypeId(String regionId) {

Rename to plural.

> +                 @Override
+                 public boolean apply(@Nullable InstanceType input) {
+                    return contains(instanceTypeIds.build(), input.instanceTypeId());
+                 }
+              }).toList();
+
+      return instanceTypes;
+   }
+
+   private List<String> getInstanceTypeId(String regionId) {
+      List<String> instanceTypeIds = Lists.newArrayList();
+      for (AvailableZone availableZone : api.instanceApi().listInstanceTypesByAvailableZone(regionId))
{
+         for (AvailableResource availableResource : availableZone.availableResources().get("AvailableResource"))
{
+            for (SupportedResource supportedResource : availableResource.supportedResources()
+                    .get("SupportedResource")) {
+               if ("Available".equals(supportedResource.status())) {

It would be preferred if the status was an enum

> +            }
+         }
+      }
+      return instanceTypeIds;
+   }
+
+   @Override
+   public Iterable<Image> listImages() {
+      final ImmutableList.Builder<Image> images = ImmutableList.builder();
+      final List<String> availableLocationNames = newArrayList(
+              transform(listLocations(), new Function<Region, String>() {
+                 @Override
+                 public String apply(Region location) {
+                    return location.id();
+                 }
+              }));

This `availableLocation` should be refactored to its own method, as the same lines are used
in several places.

> +
+      for (String regionId : availableLocationNames) {
+         images.addAll(api.imageApi().list(regionId).concat());
+      }
+      return images.build();
+   }
+
+   @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);

We'd better not fail here and just return `null`.

> +              transform(listLocations(), new Function<Region, String>() {
+                 @Override
+                 public String apply(Region location) {
+                    return location.id();
+                 }
+              }));
+
+      for (String regionId : availableLocationNames) {
+         instances.addAll(api.instanceApi().list(regionId).concat());
+      }
+      return instances.build();
+   }
+
+   @Override
+   public Iterable<Instance> listNodesByIds(final Iterable<String> ids) {
+      return filter(listNodes(), new Predicate<Instance>() {

Use the API method and pass all given ids as `ListInstancesOptions`.

> +
+      InstanceRequest instanceRequest = api.instanceApi().create(regionId, imageId, securityGroupId,
name, instanceType,
+              CreateInstanceOptions.Builder
+                      .vSwitchId(vSwitchId)
+                      .internetChargeType("PayByTraffic")
+                      .internetMaxBandwidthOut(5)
+                      .instanceChargeType("PostPaid")
+                      .instanceName(name)
+                      .keyPairName(keyPairName)
+                      .tagOptions(tagOptions)
+      );
+      String regionAndInstanceId = RegionAndId.slashEncodeRegionAndId(regionId, instanceRequest.getInstanceId());
+      instanceSuspendedPredicate.apply(regionAndInstanceId);
+      api.instanceApi().allocatePublicIpAddress(regionId, instanceRequest.getInstanceId());
+      api.instanceApi().powerOn(instanceRequest.getInstanceId());
+      instanceRunningPredicate.apply(regionAndInstanceId);

Also, do we really need for the instance to be powered on to retrieve it? Can we just not
wait, get the instance, and let the default jclouds waiting code to the active waiting?

> +         @Override
+         public boolean apply(String input) {
+            return label.contains(input);
+         }
+      }).transform(new Function<String, OsFamily>() {
+         @Override
+         public OsFamily apply(String input) {
+            return OTHER_OS_MAP.get(input);
+         }
+      });
+   }
+
+   @Override
+   public org.jclouds.compute.domain.Image apply(Image input) {
+      ImageBuilder builder = new ImageBuilder();
+      builder.ids(input.id());

Are images global? Or do we need to prefix their IDs with the region too?

> +   private final Supplier<Map<String, ? extends Hardware>> hardwares;
+   private final Supplier<Set<? extends Location>> locations;
+   private final Function<Instance.Status, NodeMetadata.Status> toPortableStatus;
+   private final GroupNamingConvention groupNamingConvention;
+   @Resource @Named(ComputeServiceConstants.COMPUTE_LOGGER) protected Logger logger = Logger.NULL;
+
+   @Inject
+   public InstanceToNodeMetadata(InstanceTypeToHardware instanceTypeToHardware,
+                                 Supplier<Map<String, ? extends Image>> images,
+                                 Supplier<Map<String, ? extends Hardware>> hardwares,
+                                 @Memoized Supplier<Set<? extends Location>>
locations,
+                                 Function<Instance.Status, NodeMetadata.Status> toPortableStatus,
+                                 GroupNamingConvention.Factory groupNamingConvention) {
+      this.instanceTypeToHardware = instanceTypeToHardware;
+      this.images = checkNotNull(images, "images cannot be null");
+      this.hardwares = checkNotNull(hardwares, "hardwares cannot be null");

Null checks are redundant in injected constructors. Remove them.

> +/**
+ * Transforms an {@link Instance} to the jclouds portable model.
+ */
+@Singleton
+public class InstanceToNodeMetadata implements Function<Instance, NodeMetadata> {
+
+   private final InstanceTypeToHardware instanceTypeToHardware;
+   private final Supplier<Map<String, ? extends Image>> images;
+   private final Supplier<Map<String, ? extends Hardware>> hardwares;
+   private final Supplier<Set<? extends Location>> locations;
+   private final Function<Instance.Status, NodeMetadata.Status> toPortableStatus;
+   private final GroupNamingConvention groupNamingConvention;
+   @Resource @Named(ComputeServiceConstants.COMPUTE_LOGGER) protected Logger logger = Logger.NULL;
+
+   @Inject
+   public InstanceToNodeMetadata(InstanceTypeToHardware instanceTypeToHardware,

Remove the public modifier to leave this in default scope.

> +      if (hardware.isPresent()) {
+         builder.hardware(hardware.get());
+      } else {
+         logger.info(">> hardware with id %s for instance %s was not found. "
+                         + "This might be because the image that was used to create the instance
has a new id.",
+                 from.instanceType(), from.instanceId());
+      }
+
+      builder.id(RegionAndId.slashEncodeRegionAndId(from.regionId(), from.instanceId()));
+      builder.providerId(from.instanceId());
+      builder.name(from.instanceName());
+      builder.hostname(String.format("%s", from.hostname()));
+      builder.group(groupNamingConvention.extractGroup(from.instanceName()));
+      builder.status(toPortableStatus.apply(from.status()));
+      builder.privateAddresses(from.innerIpAddress().entrySet().iterator().next().getValue());
+      builder.publicAddresses(from.publicIpAddress().entrySet().iterator().next().getValue());

Why do you just get the first IP and not all the IPs in the set?

> +
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Objects;
+import org.jclouds.compute.options.TemplateOptions;
+
+import static com.google.common.base.Objects.equal;
+
+/**
+ * Custom options for the Alibaba Elastic Compute Service API.
+ */
+public class ECSServiceTemplateOptions extends TemplateOptions implements Cloneable {
+
+   private String keyPairName = "";
+   private String vpcId = "";
+   private String vSwitchId = "";
+   private String userData = "";

Unused?

> +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";

Change the constant name to `RESOURCE_TYPE_SECURITY_GROUP`.

> +         } else {
+            logger.warn(">> security group: (%s) has not been deleted.", securityGroupId);
+         }
+      }
+
+      return instanceDeleted;
+   }
+
+   private List<String> getSecurityGroupIdsUsedByNode(RegionAndId regionAndId) {
+      List<String> securityGroupIds = Lists.newArrayList();
+      PaginatedCollection<Instance> instances = api.instanceApi().list(regionAndId.regionId(),
ListInstancesOptions.Builder.instanceIds(regionAndId.id()));
+      if (instances.isEmpty()) return securityGroupIds;
+
+      Instance instance = Iterables.get(instances, 0);
+      if (instance != null && !instance.securityGroupIds().isEmpty()) {
+         securityGroupIds = instance.securityGroupIds().values().iterator().next();

Just one security group? Why not getting the entire set?

> +
+      String securityGroupId = null;
+      if (!options.getGroups().isEmpty()) {
+         Iterable<String> securityGroupNames = api.securityGroupApi().list(location.getId()).concat().transform(new
Function<SecurityGroup, String>() {
+            @Override
+            public String apply(SecurityGroup input) {
+               return input.name();
+            }
+         });
+         for (String securityGroupName : options.getGroups()) {
+            checkState(Iterables.contains(securityGroupNames, securityGroupName), "Cannot
find security group with name " + securityGroupName + ". \nSecurity groups available are:
\n" + Iterables.toString(securityGroupNames)); // {
+         }
+      } else if (options.getInboundPorts().length > 0) {
+         String name = namingConvention.create().sharedNameForGroup(group);
+         SecurityGroupRequest securityGroupRequest = api.securityGroupApi().create(location.getId(),
+                 CreateSecurityGroupOptions.Builder.securityGroupName(name).vpcId(options.getVpcId()));

The `vpcId` template option seems mandatory. LIke with the `vSwitch` one, options should not
be mandatory. Can we provide a default value, or create one if no value has been provided?
At least, if this is not possible, we should validate the template options as a precondition
to the `execute` method and fail soon with an understandable message.

> +@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();

Candidate for an enum?

> +import org.jclouds.json.SerializedNames;
+
+@AutoValue
+public abstract class DedicatedHostAttribute {
+
+   DedicatedHostAttribute() {
+   }
+
+   @SerializedNames({ "DedicatedHostId", "DedicatedHostName" })
+   public static DedicatedHostAttribute create(String dedicatedHostId, String dedicatedHostName)
{
+      return new AutoValue_DedicatedHostAttribute(dedicatedHostId, dedicatedHostName);
+   }
+
+   public abstract String dedicatedHostId();
+
+   public abstract String dedicatedHostName();

Remove the `dedicatedHost` prefix in these fields. You are already in the `DedicatedHost`
class.

> +            hostname, instanceType, creationTime, status,
+            tags == null ? ImmutableMap.<String, List<Tag>>of() : ImmutableMap.copyOf(tags),
clusterId, recyclable,
+            regionId, gpuSpec, dedicatedHostAttribute,
+            operationLocks == null ? ImmutableMap.<String, List<String>>of()
: ImmutableMap.copyOf(operationLocks),
+            InstanceChargeType, gpuAmount, expiredTime);
+   }
+
+   public abstract Map<String, List<String>> innerIpAddress();
+
+   public abstract String imageId();
+
+   public abstract String instanceTypeFamily();
+
+   public abstract String vlanId();
+
+   @Nullable

We are enforcing presence in the constructor. This will never return `null`.

> +
+   public abstract Boolean recyclable();
+
+   public abstract String regionId();
+
+   public abstract String gpuSpec();
+
+   public abstract DedicatedHostAttribute dedicatedHostAttribute();
+
+   public abstract Map<String, List<String>> operationLocks();
+
+   public abstract String InstanceChargeType();
+
+   public abstract Integer gpuAmount();
+
+   public abstract Date expiredTime();

Are all these properties always (and in every case) present? Or do we need to add some `@Nullable`
annotations?

> +
+import com.google.auto.value.AutoValue;
+import org.jclouds.json.SerializedNames;
+
+@AutoValue
+public abstract class InstanceType {
+
+   InstanceType() {
+   }
+
+   @SerializedNames({ "InstanceTypeId", "CpuCoreCount", "MemorySize" })
+   public static InstanceType create(String instanceTypeId, Integer cpuCoreCount, Double
memorySize) {
+      return new AutoValue_InstanceType(instanceTypeId, cpuCoreCount, memorySize);
+   }
+
+   public abstract String instanceTypeId();

Just `id`.

> +public abstract class NetworkInterface {
+
+   NetworkInterface() {
+   }
+
+   @SerializedNames({ "MacAddress", "PrimaryIpAddress", "NetworkInterfaceId" })
+   public static NetworkInterface create(String macAddress, String primaryIpAddress, String
networkInterfaceId) {
+      return new AutoValue_NetworkInterface(macAddress, primaryIpAddress, networkInterfaceId);
+   }
+
+   @Nullable
+   public abstract String macAddress();
+
+   public abstract String primaryIpAddress();
+
+   public abstract String networkInterfaceId();

Just `id`.

> + * 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.aliyun.ecs.domain;
+
+import com.google.common.base.Enums;
+import com.google.common.base.Joiner;
+import com.google.common.base.Optional;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+/**
+ * The type of the ECS resource. All values must be lowercase.

Should we override the `toString()` method or similar to return them in lowercase when serializing?

> +   public static final String KEYPAIR_PARAM = "KeyPairName";
+   public static final String INTERNET_CHARGE_TYPE_PARAM = "InternetChargeType";
+   public static final String INTERNET_MAX_BANDWITH_IN_PARAM = "InternetMaxBandwidthIn";
+   public static final String INTERNET_MAX_BANDWITH_OUT_PARAM = "InternetMaxBandwidthOut";
+   public static final String INSTANCE_CHARGE_TYPE_PARAM = "InstanceChargeType";
+
+   /**
+    * Configures the name of the instance to be returned.
+    */
+   public CreateInstanceOptions instanceName(String instanceName) {
+      queryParameters.put(INSTANCE_NAME_PARAM, instanceName);
+      return this;
+   }
+
+   /**
+    * Configures the number of the instances to be returned.

Wrong javadoc?

> +              .join(Iterables.transform(Arrays.asList(innerIpAddresses), new Function<String,
String>() {
+                 @Override
+                 public String apply(String s) {
+                    return new StringBuilder(s.length() + 1).append('"').append(s).append('"').toString();
+                 }
+              }));
+      queryParameters.put(INNER_IP_ADDRESSES_PARAM, String.format("[%s]", instanceIdsAsString));
+      return this;
+   }
+
+   public ListInstancesOptions publicIpAddresses(String... publicIpAddresses) {
+      String instanceIdsAsString = Joiner.on(",")
+              .join(Iterables.transform(Arrays.asList(publicIpAddresses), new Function<String,
String>() {
+                 @Override
+                 public String apply(String s) {
+                    return new StringBuilder(s.length() + 1).append('"').append(s).append('"').toString();

Extract this to a `quote` function or similar you can reuse and properly unit test.

> @@ -92,4 +94,14 @@ public boolean apply(@Nullable String input) {
       }), "Cannot starts with " + Iterables.toString(FORBIDDEN_PREFIX));
    }
 
+   /**
+    * This is strictly not needed but apparently tags with `-` can create a problem when
using API, so I've decided to use
+    * base64 encoding
+    * @param value
+    * @return
+    */
+   public String encodeTag(String value) {

Make this method `static`.

> @@ -92,4 +94,14 @@ public boolean apply(@Nullable String input) {
       }), "Cannot starts with " + Iterables.toString(FORBIDDEN_PREFIX));
    }
 
+   /**
+    * This is strictly not needed but apparently tags with `-` can create a problem when
using API, so I've decided to use
+    * base64 encoding
+    * @param value
+    * @return

Properly document the parameter and return value.

> +      vSwitchId = setIfTestSystemPropertyPresent(properties,  provider + ".vSwitchId");
+      return properties;
+   }
+
+   @Override
+   protected TemplateBuilder templateBuilder() {
+      return super.templateBuilder()
+                      .options(vpcId(vpcId)
+                      .vSwitchId(vSwitchId));
+   }
+
+   @Override
+   protected Template addRunScriptToTemplate(Template template) {
+      template.getOptions().runScript(Statements.newStatementList(
+            new Statement[] { AdminAccess.standard(), Statements.exec("sleep 50"), InstallJDK.fromOpenJDK()
}));
+      return template;

Do we really need to override this to wait?

> +
+   @Override
+   protected Template addRunScriptToTemplate(Template template) {
+      template.getOptions().runScript(Statements.newStatementList(
+            new Statement[] { AdminAccess.standard(), Statements.exec("sleep 50"), InstallJDK.fromOpenJDK()
}));
+      return template;
+   }
+
+   @Override
+   protected Module getSshModule() {
+      return new SshjSshClientModule();
+   }
+
+   @Override
+   @Test(expectedExceptions = AuthorizationException.class)
+   public void testCorrectAuthException() throws Exception {

Why do we need to override this test?

> +         context.getComputeService().listImages();
+      } catch (AuthorizationException e) {
+         throw e;
+      } catch (RuntimeException e) {
+         e.printStackTrace();
+         throw e;
+      } finally {
+         if (context != null)
+            context.close();
+      }
+   }
+
+   @Override
+   public void testOptionToNotBlock() throws Exception {
+      // Aliyun ECS ComputeService implementation has to block until the node
+      // is provisioned, to be able to return it.

If we don't block after power on, we can probably remove this override. It would be a nice
to have, as jclouds also provides non-blocking options when deploying nodes.

> +   private String vSwitchId = "vsw-gw8c79bsp4ezbe34ij3w8";
+   private String securityGroupId;
+   private String instanceId;
+
+   @BeforeClass
+   public void setUp() {
+      SecurityGroupRequest request = api.securityGroupApi().create(Regions.EU_CENTRAL_1.getName(),
+              CreateSecurityGroupOptions.Builder.securityGroupName(InstanceApiLiveTest.class.getSimpleName())
+      );
+      securityGroupId = request.getSecurityGroupId();
+
+      Properties properties = super.setupProperties();
+      vSwitchId = setIfTestSystemPropertyPresent(properties,  provider + ".vSwitchId");
+   }
+
+   @AfterClass

Use `alwaysRun + true` to make sure you always cleanup even if tests fail.

> +
+   @Test(groups = "live", dependsOnMethods = "testListInstance")
+   public void testCreate() {
+      InstanceRequest instanceRequest = api().create(Regions.EU_CENTRAL_1.getName(), imageId,
securityGroupId, hostname, instanceType,
+            CreateInstanceOptions.Builder.vSwitchId(vSwitchId));
+      instanceId = instanceRequest.getInstanceId();
+      assertNotNull(instanceId, "Instance must not be null");
+   }
+
+   @Test(groups = "live", dependsOnMethods = "testCreate")
+   public void testGet() {
+      Instance instance = Iterables.getOnlyElement(api().list(Regions.EU_CENTRAL_1.getName(),
ListInstancesOptions.Builder.instanceIds(instanceId)));
+      assertNotNull(instance.instanceId(), "Instance must not be null");
+   }
+
+   @Test(groups = "live", dependsOnMethods = "testGet")

Can depend just on the `testCreate`?

> +   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);

Verify the request that has been sent too.

> +
+   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);
+   }

There are a lot of missing methods in this test class.

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