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
