nacx commented on this pull request.
> + 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. + +--> +<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; xmlns="http://maven.apache.org/POM/4.0.0"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.jclouds.labs</groupId> + <artifactId>jclouds-labs</artifactId> + <version>2.2.0-SNAPSHOT</version> + </parent> + <artifactId>aliyun-ecs</artifactId> + <name>Cloudsoft :: jclouds Alibaba Elastic Compute Service API</name> Cloudsoft? > + <groupId>org.apache.felix</groupId> + <artifactId>maven-bundle-plugin</artifactId> + <version>3.5.0</version> + <extensions>true</extensions> + <configuration> + <obrRepository>NONE</obrRepository> + <instructions> + <!--<Bundle-Activator>${jclouds.osgi.activator}</Bundle-Activator>--> + <Bundle-SymbolicName>${project.artifactId}</Bundle-SymbolicName> + <!--<Export-Package>${jclouds.osgi.export}</Export-Package>--> + <Import-Package>${guava.osgi.import},${jclouds.osgi.import}</Import-Package> + <DynamicImport-Package>*</DynamicImport-Package> + <!--<Fragment-Host>${jclouds.osgi.fragment}</Fragment-Host>--> + </instructions> + </configuration> + </plugin> Why do we need to configure the bundle here? The default config is not OK? > + <artifactId>maven-bundle-plugin</artifactId> + <version>3.5.0</version> + <extensions>true</extensions> + <configuration> + <obrRepository>NONE</obrRepository> + <instructions> + <!--<Bundle-Activator>${jclouds.osgi.activator}</Bundle-Activator>--> + <Bundle-SymbolicName>${project.artifactId}</Bundle-SymbolicName> + <!--<Export-Package>${jclouds.osgi.export}</Export-Package>--> + <Import-Package>${guava.osgi.import},${jclouds.osgi.import}</Import-Package> + <DynamicImport-Package>*</DynamicImport-Package> + <!--<Fragment-Host>${jclouds.osgi.fragment}</Fragment-Host>--> + </instructions> + </configuration> + </plugin> + <!--TODO to be removed--> Remove it then > +import static org.jclouds.reflect.Reflection2.typeToken; + +public class ECSServiceApiMetadata extends BaseHttpApiMetadata<ECSComputeServiceApi> { + + public ECSServiceApiMetadata() { + this(new Builder()); + } + + protected ECSServiceApiMetadata(Builder builder) { + super(builder); + } + + public static Properties defaultProperties() { + Properties properties = BaseHttpApiMetadata.defaultProperties(); + properties.put(TEMPLATE, + "osFamily=CENTOS,os64Bit=true,osVersionMatches=7.4,hardwareId=ecs.t5-lc2m1.nano,locationId=eu-central-1"); Let's just fix the OS by default. The default location should be configured by the jclouds *Implicit Location Supplier*. The default hardware is, by default, the smallest one. > +import org.jclouds.http.HttpErrorHandler; +import org.jclouds.http.annotation.ClientError; +import org.jclouds.http.annotation.Redirection; +import org.jclouds.http.annotation.ServerError; +import org.jclouds.location.suppliers.ImplicitLocationSupplier; +import org.jclouds.location.suppliers.implicit.FirstRegion; +import org.jclouds.rest.ConfiguresHttpApi; +import org.jclouds.rest.config.HttpApiModule; + +@ConfiguresHttpApi +public class ECSComputeServiceHttpApiModule extends HttpApiModule<ECSComputeServiceApi> { + + @Override + protected void configure() { + super.configure(); + bind(ImplicitLocationSupplier.class).to(FirstRegion.class).in(Scopes.SINGLETON); Doesn't the default implementatio (`OnlyLocationOrFirstRegionOptionallyMatchingRegionId`) do pretty much the same? > + + @Override + protected void configure() { + bind(GsonModule.DateAdapter.class).to(AliyunDateAdapter.class); + } + + /** + * Data adapter for the date formats used by Aliyun. + * <p> + * Essentially this is a workaround for the Aliyun getUsage() API call returning a corrupted form of ISO-8601 + * dates, which doesn't have seconds 2018-06-20T13:39Z + */ + public static class AliyunDateAdapter extends GsonModule.Iso8601DateAdapter { + + @Inject + private AliyunDateAdapter(DateService dateService) { Remove the `private` modifier so you can properly test it. > + + public abstract String status(); + + public abstract String imageName(); + + public abstract Boolean isSupportIoOptimizeds(); + + public abstract Boolean isSelfShared(); + + public abstract Boolean isCopied(); + + public abstract Boolean isSubscribed(); + + public abstract String platform(); + + public abstract String size(); Are all fields non-nullable? Also consider providing a builder, since it has too many fields. > + + /** + * Returns a region enum corresponding to the given region name. + * + * @param regionName + * The name of the region. Ex.: eu-west-1 + * @return Region enum representing the given region name. + */ + public static Regions fromName(String regionName) { + for (Regions region : Regions.values()) { + if (region.getName().equals(regionName)) { + return region; + } + } + throw new IllegalArgumentException("Cannot create enum from " + regionName + " value!"); + } Is there any API call to retrieve the list of available regions, instead of having them hardcoded? > + AP_NORTHEAST_1("ap-northeast-1", "Japan (Tokyo)"), + AP_SOUTH_1("ap-south-1", "India (Mumbai)"), + AP_SOUTHEAST_1("ap-southeast-1", "Singapore"), + AP_SOUTHEAST_2("ap-southeast-2", "Australia (Sydney)"), + AP_SOUTHEAST_3("ap-southeast-3", "Malaysia (Kuala Lumpur)"), + AP_SOUTHEAST_5("ap-southeast-5", "Indonesia (Jakarta)"), + CN_NORTH_1("cn-qingdao", "China (Qingdao)"), + CN_NORTH_2("cn-beijing", "China (Beijing)"), + CN_NORTH_3("cn-zhangjiakou", "China (Zhangjiakou)"), + CN_NORTH_5("cn-huhehaote", "China (Huhehaote)"), + CN_EAST_1("cn-hangzhou", "China (Zhangjiakou)"), + CN_EAST_2("cn-shanghai", "China (Shanghai)"), + CN_SOUTH_1("cn-shenzhen", "China (Shenzhen)"), + CN_SOUTH_2("cn-hongkong", "China (Hongkong)"), + CN_NORTHWEST_1("cn-northwest-1", "India (Mumbai)"), + ME_EAST_1("me-east-1", "UAE (Dubai)"); Consider adding the ISO codes here too, to make it easier to reuse the enum. > + public ListImagesOptions imageName(String imageName) { + queryParameters.put(IMAGE_NAME_PARAM, imageName); + return this; + } + + public ListImagesOptions imageOwnerAlias(String imageOwnerAlias) { + queryParameters.put(IMAGE_OWNER_ALIAS_PARAM, imageOwnerAlias); + return this; + } + + public ListImagesOptions usage(String usage) { + queryParameters.put(USAGE_PARAM, usage); + return this; + } + + public ListImagesOptions paginationOptions(final PaginationOptions paginationOptions) { Could it be better for this class to extend `PaginationOptions` instead? > + * limitations under the License. + */ +package org.jclouds.aliyun.ecs.domain.options; + +import org.jclouds.http.options.BaseHttpRequestOptions; + +import static com.google.common.base.Preconditions.checkState; + +public class PaginationOptions extends BaseHttpRequestOptions { + + public static final String PAGE_NUMBER = "pageNumber"; + public static final String PAGE_SIZE = "pageSize"; + + public PaginationOptions pageNumber(int pageNumber) { + checkState(pageNumber > 0, "pageSize must be > 0"); + checkState(pageNumber <= 10000, "limit must be <= 50"); Precondition and message don't match. > + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.jclouds.aliyun.ecs.domain.options; + +import org.jclouds.http.options.BaseHttpRequestOptions; + +import static com.google.common.base.Preconditions.checkState; + +public class PaginationOptions extends BaseHttpRequestOptions { + + public static final String PAGE_NUMBER = "pageNumber"; + public static final String PAGE_SIZE = "pageSize"; + + public PaginationOptions pageNumber(int pageNumber) { + checkState(pageNumber > 0, "pageSize must be > 0"); Wrong message > + +import javax.inject.Inject; +import javax.inject.Named; +import javax.inject.Singleton; +import javax.ws.rs.Consumes; +import javax.ws.rs.GET; +import javax.ws.rs.QueryParam; +import javax.ws.rs.core.MediaType; + +/** + * https://www.alibabacloud.com/help/doc-detail/25534.htm?spm=a2c63.p38356.b99.330.79eb59abhmnMDE + */ +@Consumes(MediaType.APPLICATION_JSON) +@RequestFilters(FormSign.class) +@QueryParams(keys = {"Version", "Format", "SignatureVersion", "ServiceCode", "SignatureMethod"}, + values = {"2014-05-26", "JSON", "1.0", "ecs", "HMAC-SHA1"}) Is this date the API version that can be set in the `ContextBuilder`? In that case instead of hardcoding you can resolve its value. Look at the Chef API for an example. > + +/** + * https://www.alibabacloud.com/help/doc-detail/25534.htm?spm=a2c63.p38356.b99.330.79eb59abhmnMDE + */ +@Consumes(MediaType.APPLICATION_JSON) +@RequestFilters(FormSign.class) +@QueryParams(keys = {"Version", "Format", "SignatureVersion", "ServiceCode", "SignatureMethod"}, + values = {"2014-05-26", "JSON", "1.0", "ecs", "HMAC-SHA1"}) +public interface ImageApi { + + @Named("image:list") + @GET + @QueryParams(keys = "Action", values = "DescribeImages") + @ResponseParser(ParseImages.class) + @Fallback(Fallbacks.EmptyIterableWithMarkerOnNotFoundOr404.class) + PaginatedCollection<Image> list(@QueryParam("RegionId") String region, ListImagesOptions... options); Don't do this, as it is not OK to allow passing N option objects. Provide a method without options, and an overloaded one with just one options parameter. > + @ResponseParser(ParseImages.class) + @Transform(ParseImages.ToPagedIterable.class) + @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class) + PagedIterable<Image> list(@QueryParam("RegionId") String region); + + @Singleton + final class ParseImages extends ParseJson<Images> { + + @Inject + ParseImages(final Json json) { + super(json, TypeLiteral.get(Images.class)); + } + + static class ToPagedIterable extends Arg0ToPagedIterable<Image, ToPagedIterable> { + + private ECSComputeServiceApi api; Make it final > + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * 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.filters; + +import java.io.UnsupportedEncodingException; +import java.net.URLEncoder; + +public class AcsURLEncoder { Can't you just use [Strings2#urlEncode](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/util/Strings2.java#L56)? Or do you need to specifically handle the `~` characterd? > + decodedParams.put("Timestamp", timestamp); + decodedParams.put("SignatureNonce", signatureNonce); + + String prefix; + try { + prefix = request.getMethod() + SEPARATOR + AcsURLEncoder.percentEncode("/") + SEPARATOR; + } catch (UnsupportedEncodingException e) { + throw Throwables.propagate(e); + } + + // encode each parameter value first, + String stringToSign = prefix; + ImmutableSortedSet.Builder<String> builder = ImmutableSortedSet.naturalOrder(); + for (Map.Entry<String, String> entry : decodedParams.entries()) + builder.add(Strings2.urlEncode(entry.getKey()) + "=" + Strings2.urlEncode(entry.getValue())); + stringToSign += Strings2.urlEncode(Joiner.on("&").join(builder.build())); If you use the jclouds `Uris` and `UriBuilder` classes I think you don't have to worry about manually encoding the parameters. > + + @Inject + FormSign(@Provider Supplier<Credentials> creds) { + this.creds = creds; + } + + public HttpRequest filter(HttpRequest request) throws HttpException { + Credentials currentCreds = checkNotNull(creds.get(), "credential supplier returned null"); + + Signer signer = Signer.getSigner(); + + String signature; + + Multimap<String, String> decodedParams = queryParser().apply(request.getEndpoint().getRawQuery()); + + SimpleDateFormat df = new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'"); This could be declared as a constant. > +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; +import javax.xml.bind.DatatypeConverter; +import java.security.InvalidKeyException; +import java.security.NoSuchAlgorithmException; + +public class HmacSHA1Signer extends Signer { + + private static final String ALGORITHM_NAME = "HmacSHA1"; + + @Override + public String signString(String stringToSign, String accessKeySecret) { + try { + Mac mac = Mac.getInstance(ALGORITHM_NAME); + mac.init(new SecretKeySpec(accessKeySecret.getBytes(Charsets.UTF_8), ALGORITHM_NAME)); + byte[] signData = mac.doFinal(stringToSign.getBytes(Charsets.UTF_8)); Instead of manually encrypting, get injected a [Crypto](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/crypto/Crypto.java#L46) object and use it. This way users will be able to configure they preferred security provider, such as Bouncycastle. > + } + + protected void assertNodeRunning(String instanceId) { + assertTrue(instanceRunning.apply(instanceId), + String.format("Instance %s did not start in the configured timeout", instanceId)); + } + + protected void assertNodeSuspended(String instanceId) { + assertTrue(instanceSuspended.apply(instanceId), + String.format("Instance %s was not suspended in the configured timeout", instanceId)); + } + + protected void assertNodeTerminated(String instanceId) { + assertTrue(instanceTerminated.apply(instanceId), + String.format("Instance %s was not terminated in the configured timeout", instanceId)); + } Remove these methods until needed. > + assertEquals(request.getMethod(), method); + Map<String, String> queryParameters = Splitter.on('&').trimResults().withKeyValueSeparator("=").split(request.getPath()); + assertEquals(queryParameters.get("Action"), action); + assertEquals(Integer.valueOf(queryParameters.get("pageNumber")), page); + assertEquals(request.getHeader("Accept"), "application/json"); + return request; + } + + protected RecordedRequest assertSent(MockWebServer server, String method, String path, String json) + throws InterruptedException { + RecordedRequest request = assertSent(server, method, path); + assertEquals(request.getHeader("Content-Type"), "application/json"); + assertEquals(parser.parse(new String(request.getBody(), Charsets.UTF_8)), parser.parse(json)); + return request; + } +} Remove all methods that are not used. > +import static org.testng.Assert.assertTrue; + +@Test(groups = "unit", testName = "ImageApiMockTest", singleThreaded = true) +public class ImageApiMockTest extends BaseECSComputeServiceApiMockTest { + + public void testListImages() throws InterruptedException { + server.enqueue(jsonResponse("/images-first.json")); + server.enqueue(jsonResponse("/images-second.json")); + server.enqueue(jsonResponse("/images-last.json")); + + Iterable<Image> images = api.imageApi().list(Regions.EU_CENTRAL_1.getName()).concat(); + assertEquals(size(images), 28); // Force the PagedIterable to advance + assertEquals(server.getRequestCount(), 3); + assertSent(server, "GET", "DescribeImages"); + assertSent(server, "GET", "DescribeImages", 2); + assertSent(server, "GET", "DescribeImages", 3); We should also verify that we're sending the page size parameter. > +import static org.testng.Assert.assertTrue; + +@Test(groups = "unit", testName = "ImageApiMockTest", singleThreaded = true) +public class ImageApiMockTest extends BaseECSComputeServiceApiMockTest { + + public void testListImages() throws InterruptedException { + server.enqueue(jsonResponse("/images-first.json")); + server.enqueue(jsonResponse("/images-second.json")); + server.enqueue(jsonResponse("/images-last.json")); + + Iterable<Image> images = api.imageApi().list(Regions.EU_CENTRAL_1.getName()).concat(); + assertEquals(size(images), 28); // Force the PagedIterable to advance + assertEquals(server.getRequestCount(), 3); + assertSent(server, "GET", "DescribeImages"); + assertSent(server, "GET", "DescribeImages", 2); + assertSent(server, "GET", "DescribeImages", 3); And to leave the pagination stuff properly tested, add a test for a single page response, and for an empty list response. -- 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/438#pullrequestreview-133975031
