Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
nacx requested changes on this pull request. > +annotations-java5 +RELEASE +compile + + +org.jetbrains +annotations-java5 +RELEASE +compile + + +org.jetbrains +annotations-java5 +RELEASE +compile + 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 firstInterestingImage = Iterables.tryFind(listImages(), new Predicate() { - 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.>of("IpAddress", ImmutableList.of("192.168.0.1"))) + .publicIpAddress(ImmutableMap.>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 privateIpAddresses, List publicIpAddresses) { + assertNotNull(result); + assertEquals(resu
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
andreaturli commented on this pull request. > +annotations-java5 +RELEASE +compile + + +org.jetbrains +annotations-java5 +RELEASE +compile + + +org.jetbrains +annotations-java5 +RELEASE +compile + where this crap come from? :( -- 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#discussion_r207828506
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
andreaturli commented on this pull request. > @@ -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 good point, Ill look into it -- 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#discussion_r207828745
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
andreaturli commented on this pull request. > +// 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); +// } +// +//} still working on this, forget to exclude from commit, sorry -- 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#discussion_r207828836
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
andreaturli commented on this pull request. > +@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(); don't know, they are not well documented -- 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#discussion_r207833707
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
andreaturli commented on this pull request. > @@ -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 @nacx as it can't start with `aliyun` shall I use `alibaba-ecs` as id for the provider in the `ECSComputeServiceProviderMetadata` ? -- 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#discussion_r207836467
Re: [jclouds/jclouds-labs] [JCLOUDS-1430] Aliyun ECS (#443)
nacx commented on this pull request. > @@ -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 Sounds good. Preconfigure that and remove this from here. -- 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#discussion_r207868271