Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
Closed #319. -- 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/319#event-805473797
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
Pushed to master as [4e397202](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/4e397202). Thanks @alibazlamit! -- 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/319#issuecomment-250173385
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
Squashed. Thanks. -- 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/319#issuecomment-250150406
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
Mind squashing the commits so I can cleanly merge 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/319#issuecomment-250109508
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
nacx commented on this pull request. > +import com.google.inject.TypeLiteral; +import java.io.ByteArrayInputStream; +import java.util.logging.Level; +import java.util.logging.Logger; +import java.util.zip.ZipInputStream; +import javax.inject.Inject; +import javax.inject.Singleton; +import org.apache.jclouds.oneandone.rest.domain.VPNConfig; +import org.jclouds.http.functions.ParseJson; +import org.jclouds.json.Json; + +@Singleton +public class VPNConfigParser extends ParseJson { + + @Inject + public VPNConfigParser(Json json) { Perfect. Thanks! -- 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/319
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
nacx approved this pull request. -- 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/319#pullrequestreview-1891528
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
alibazlamit commented on this pull request. > +import com.google.inject.TypeLiteral; +import java.io.ByteArrayInputStream; +import java.util.logging.Level; +import java.util.logging.Logger; +import java.util.zip.ZipInputStream; +import javax.inject.Inject; +import javax.inject.Singleton; +import org.apache.jclouds.oneandone.rest.domain.VPNConfig; +import org.jclouds.http.functions.ParseJson; +import org.jclouds.json.Json; + +@Singleton +public class VPNConfigParser extends ParseJson { + + @Inject + public VPNConfigParser(Json json) { I tried removing the parser class that create an error it could not parse the JSON correctly for a reason so i decided to leave it like this, i have removed the public modifier and pushed the change. Thanks -- 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/319
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
@alibazlamit pushed 1 commit. 426a288 remove public constructor -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/319/files/4331e099037b24edea158bfa3d20228c9c39035d..426a288b57f1de9a554568766ebd966399ab0d39
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
nacx commented on this pull request. > +import com.google.inject.TypeLiteral; +import java.io.ByteArrayInputStream; +import java.util.logging.Level; +import java.util.logging.Logger; +import java.util.zip.ZipInputStream; +import javax.inject.Inject; +import javax.inject.Singleton; +import org.apache.jclouds.oneandone.rest.domain.VPNConfig; +import org.jclouds.http.functions.ParseJson; +import org.jclouds.json.Json; + +@Singleton +public class VPNConfigParser extends ParseJson { + + @Inject + public VPNConfigParser(Json json) { The visibility of this constructor still needs to be reduced by removing the public modifier (see [this](https://github.com/google/guice/wiki/KeepConstructorsHidden)). Or you can completely remove the parser class and bring back the `@SelectJson` annotation in the api method. -- 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/319
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
@alibazlamit pushed 1 commit. 4331e09 Updated with changes -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/319/files/debbc9b5bc3c2d072ee55d9f184d3c59ed778fb8..4331e099037b24edea158bfa3d20228c9c39035d
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
alibazlamit commented on this pull request. > + public static class ToZipStream implements Function ZipInputStream> { + + @Inject + public ToZipStream() { + super(); + } + + @Override + public ZipInputStream apply(VPNConfig input) { + try { +byte[] decoded = base64().decode(input.content()); +ZipInputStream zipStream = new ZipInputStream(new ByteArrayInputStream(decoded)); +return zipStream; + } catch (Exception ex) { + Logger.getLogger(VPNConfigParser.class.getName()).log(Level.SEVERE, null, ex); +return null; Removed the try catch, if the API fails to return the config the fallback with work as needed and return a null object, no need for a try catch in there, if any other error should be returned from the API the error handler will do its job. Changes pushed -- 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/319
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
nacx requested changes on this pull request. Done reviewing. Thanks @alibazlamit! > @@ -62,9 +65,10 @@ @Named("vpn:configurations:get") @GET @Path("/{vpnId}/configuration_file") - @SelectJson("config_zip_file") + @ResponseParser(VPNConfigParser.class) Is this parser class needed? You could leave the `@SelectJson` annotation and just create the transformation function as a top level class. > +import com.google.inject.TypeLiteral; +import java.io.ByteArrayInputStream; +import java.util.logging.Level; +import java.util.logging.Logger; +import java.util.zip.ZipInputStream; +import javax.inject.Inject; +import javax.inject.Singleton; +import org.apache.jclouds.oneandone.rest.domain.VPNConfig; +import org.jclouds.http.functions.ParseJson; +import org.jclouds.json.Json; + +@Singleton +public class VPNConfigParser extends ParseJson { + + @Inject + public VPNConfigParser(Json json) { Remove the `public`modifier if this parser class remains. > +import org.jclouds.json.Json; + +@Singleton +public class VPNConfigParser extends ParseJson { + + @Inject + public VPNConfigParser(Json json) { + super(json, TypeLiteral.get(VPNConfig.class)); + } + + public static class ToZipStream implements Function { + + @Inject + public ToZipStream() { + super(); + } Remove this constructor? > + + public static class ToZipStream implements Function { + + @Inject + public ToZipStream() { + super(); + } + + @Override + public ZipInputStream apply(VPNConfig input) { + try { +byte[] decoded = base64().decode(input.content()); +ZipInputStream zipStream = new ZipInputStream(new ByteArrayInputStream(decoded)); +return zipStream; + } catch (Exception ex) { + Logger.getLogger(VPNConfigParser.class.getName()).log(Level.SEVERE, null, ex); Don't use java logging. jclouds provides a logging abstraction that allows users to configure their preferred logging framework. In order to get the right log, just declare the following class variable of type `org.jclouds.logging.Logger`: ```java @Resource protected Logger logger = Logger.NULL; ``` This will configure a null logger by default, but use the one configured by users providing a logging module when creating the jclouds context. > + public static class ToZipStream implements Function ZipInputStream> { + + @Inject + public ToZipStream() { + super(); + } + + @Override + public ZipInputStream apply(VPNConfig input) { + try { +byte[] decoded = base64().decode(input.content()); +ZipInputStream zipStream = new ZipInputStream(new ByteArrayInputStream(decoded)); +return zipStream; + } catch (Exception ex) { + Logger.getLogger(VPNConfigParser.class.getName()).log(Level.SEVERE, null, ex); +return null; Instead of returning `null` on *any* failure, are there situations where it makes sense returning null? Can we return null just in those cases, and propagate the exception otherwise? -- 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/319#pullrequestreview-1608407
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
alibazlamit commented on this pull request. > + @GET + @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class) + List getList(GenericQueryOptions options); + + @Named("vpn:get") + @GET + @Path("/{vpnId}") + @Fallback(Fallbacks.NullOnNotFoundOr404.class) + Vpn get(@PathParam("vpnId") String vpnId); + + @Named("vpn:configurations:get") + @GET + @Path("/{vpnId}/configuration_file") + @SelectJson("config_zip_file") + @Fallback(Fallbacks.NullOnNotFoundOr404.class) + String getConfiguration(@PathParam("vpnId") String vpnId); @nacx Added the function could you please check the PR, Thanks. -- 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/319
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
@alibazlamit pushed 1 commit. debbc9b Fixed eol -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/319/files/cdc081ebd87932804176473980ef7faf59076b4a..debbc9b5bc3c2d072ee55d9f184d3c59ed778fb8
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
@alibazlamit pushed 1 commit. cdc081e Updated Changes. -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/319/files/4f275a8ce0ed1e4738587b3ed6a9e410cf70a259..cdc081ebd87932804176473980ef7faf59076b4a
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
alibazlamit commented on this pull request. > + @GET + @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class) + List getList(GenericQueryOptions options); + + @Named("vpn:get") + @GET + @Path("/{vpnId}") + @Fallback(Fallbacks.NullOnNotFoundOr404.class) + Vpn get(@PathParam("vpnId") String vpnId); + + @Named("vpn:configurations:get") + @GET + @Path("/{vpnId}/configuration_file") + @SelectJson("config_zip_file") + @Fallback(Fallbacks.NullOnNotFoundOr404.class) + String getConfiguration(@PathParam("vpnId") String vpnId); OK ill add 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/319
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
nacx commented on this pull request. > + @GET + @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class) + List getList(GenericQueryOptions options); + + @Named("vpn:get") + @GET + @Path("/{vpnId}") + @Fallback(Fallbacks.NullOnNotFoundOr404.class) + Vpn get(@PathParam("vpnId") String vpnId); + + @Named("vpn:configurations:get") + @GET + @Path("/{vpnId}/configuration_file") + @SelectJson("config_zip_file") + @Fallback(Fallbacks.NullOnNotFoundOr404.class) + String getConfiguration(@PathParam("vpnId") String vpnId); Well, I know it is unnecessary, but if we already know we are returning a zip file, why not returning a proper stream? I mean, we know it is not *a regular string*, the same way that we return objects instead of raw json. Responses are not simple strings and we try to return useful objects. Also many users that consume apis look first at the method signatures and in the worst case they read the docs :) Returning a ZipInputStream here would serve two purposes: better document what this method does, and provide a better interface for users. Adding that should be trivial, as the function should only ned to decode the string and create a stream for it, not a big deal we'll struggle to maintain. -- 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/319
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
alibazlamit commented on this pull request. > + @GET + @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class) + List getList(GenericQueryOptions options); + + @Named("vpn:get") + @GET + @Path("/{vpnId}") + @Fallback(Fallbacks.NullOnNotFoundOr404.class) + Vpn get(@PathParam("vpnId") String vpnId); + + @Named("vpn:configurations:get") + @GET + @Path("/{vpnId}/configuration_file") + @SelectJson("config_zip_file") + @Fallback(Fallbacks.NullOnNotFoundOr404.class) + String getConfiguration(@PathParam("vpnId") String vpnId); @nacx I think its an unnecessary step,users will have to still convert the ZipInputStream to a file and choose a path,i would leave the whole to who ever wants to use that method,on the other hand if you think its must be there ill implement that. -- 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/319
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
@alibazlamit pushed 1 commit. 4f275a8 Review changes applied -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/319/files/057a2df15be24f6d76029394e39493c08738639e..4f275a8ce0ed1e4738587b3ed6a9e410cf70a259
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
nacx requested changes on this pull request. Thanks @alibazlamit! > +import org.jclouds.rest.annotations.Fallback; +import org.jclouds.rest.annotations.MapBinder; +import org.jclouds.rest.annotations.RequestFilters; +import org.jclouds.rest.annotations.SelectJson; +import org.jclouds.rest.binders.BindToJsonPayload; + +@Path("/vpns") +@Produces("application/json") +@Consumes("application/json") +@RequestFilters(AuthenticateRequest.class) +public interface VpnApi { + + @Named("vpn:list") + @GET + @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class) + List getList(); Rename to `list`. > + assertNotNull(resultWithQuery); + assertFalse(resultWithQuery.isEmpty()); + Assert.assertTrue(resultWithQuery.size() > 0); + } + + @Test + public void testGet() { + Vpn result = vpnApi().get(currentVpn.id()); + + assertNotNull(result); + assertEquals(result.id(), currentVpn.id()); + } + + @Test + public void testGetConfiguration() throws InterruptedException { + Thread.sleep(15000); Is this really needed? Is there a more deterministic way of waiting? > @@ -37,7 +37,7 @@ -2.0.0-SNAPSHOT +2.0.0-SNAPSHOT [nit] Discard this change. > + @GET + @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class) + List getList(GenericQueryOptions options); + + @Named("vpn:get") + @GET + @Path("/{vpnId}") + @Fallback(Fallbacks.NullOnNotFoundOr404.class) + Vpn get(@PathParam("vpnId") String vpnId); + + @Named("vpn:configurations:get") + @GET + @Path("/{vpnId}/configuration_file") + @SelectJson("config_zip_file") + @Fallback(Fallbacks.NullOnNotFoundOr404.class) + String getConfiguration(@PathParam("vpnId") String vpnId); Interesting, so this method returns a base64 encoded zip file containing the VPN files. Would it make sense to return a `ZipInputStream`, so users can easily consume and store the returned files? You could create a `Function` that takes that base64 string and returns a proper ZipInputStream, and then annotate this method with `@Transform(YourFunction.class)`, so jclouds automatically transforms the response. WDYT? -- 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/319#pullrequestreview-1293800
Re: [jclouds/jclouds-labs] JCLOUDS-1181 oneandone-vpn-api (#319)
@alibazlamit pushed 1 commit. 89ae677 removed checkstyle skip -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds-labs/pull/319/files/8ca167c97b501d2fe28b6a3a34fbdc0df96dbf7f..89ae677061d0aae07945036e187487e3964f8d42