andreaturli commented on this pull request.
thanks @jawnclarke it is really good start, some minor comments from me.
I'd like to understand better if we could avoid a generic `Response` for the
`create*` operations although the DimensionDataCloudControlResponseUtils is
well tested and may be ok
> + @Named("network:getNetworkDomain")
+ @GET
+ @Path("/networkDomain/{id}")
+ @Fallback(NullOnNotFoundOr404.class)
+ NetworkDomain getNetworkDomain(@PathParam("id") String networkDomainId);
+
+ @Named("network:listNetworkDomainsWithName")
+ @GET
+ @Path("/networkDomain")
+ @Transform(ParseNetworkDomains.ToPagedIterable.class)
+ @ResponseParser(ParseNetworkDomains.class)
+ @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
+ PagedIterable<NetworkDomain> listNetworkDomainsWithDatacenterIdAndName(
+ @QueryParam("datacenterId") String datacenterId, @QueryParam("name")
String name);
+
+ @Named("network:list")
[minor} any reason why this is `network:list` instead of
`network:listNetworkDomain` like the others?
Actually I think you could rename them
`"networkDomain:{deploy,get,list,delete}"`, makes sense?
> + @GET
+ @Path("/networkDomain")
+ @Transform(ParseNetworkDomains.ToPagedIterable.class)
+ @ResponseParser(ParseNetworkDomains.class)
+ @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
+ PagedIterable<NetworkDomain> listNetworkDomains();
+
+ @Named("network:deleteNetworkDomain")
+ @POST
+ @Path("/deleteNetworkDomain")
+ @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ @Fallback(NullOnNotFoundOr404.class)
+ Response deleteNetworkDomain(@PayloadParam("id") String networkDomainId);
+
+ @Named("network:deployVlan")
[minor] similarly to the above comment, maybe `"vlan:deploy"` ?
> + @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ @Fallback(NullOnNotFoundOr404.class)
+ Response deleteNetworkDomain(@PayloadParam("id") String networkDomainId);
+
+ @Named("network:deployVlan")
+ @POST
+ @Path("/deployVlan")
+ @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ Response deployVlan(@PayloadParam("networkDomainId") String
networkDomainId, @PayloadParam("name") String name,
+ @PayloadParam("description") String description,
+ @PayloadParam("privateIpv4BaseAddress") String privateIpv4BaseAddress,
+ @PayloadParam("privateIpv4PrefixSize") Integer privateIpv4PrefixSize);
+
+ @Named("network:getVlan")
[minor] similarly to the above comment, maybe `"vlan:get"` ?
> + @POST
+ @Path("/deployVlan")
+ @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ Response deployVlan(@PayloadParam("networkDomainId") String
networkDomainId, @PayloadParam("name") String name,
+ @PayloadParam("description") String description,
+ @PayloadParam("privateIpv4BaseAddress") String privateIpv4BaseAddress,
+ @PayloadParam("privateIpv4PrefixSize") Integer privateIpv4PrefixSize);
+
+ @Named("network:getVlan")
+ @GET
+ @Path("/vlan/{id}")
+ @Fallback(NullOnNotFoundOr404.class)
+ Vlan getVlan(@PathParam("id") String vlanId);
+
+ @Named("network:vlan")
[minor] similarly to the above comment, maybe `"vlan:list"` ?
> +
+ @Named("network:getVlan")
+ @GET
+ @Path("/vlan/{id}")
+ @Fallback(NullOnNotFoundOr404.class)
+ Vlan getVlan(@PathParam("id") String vlanId);
+
+ @Named("network:vlan")
+ @GET
+ @Path("/vlan")
+ @ResponseParser(ParseVlans.class)
+ @Fallback(Fallbacks.EmptyIterableWithMarkerOnNotFoundOr404.class)
+ PaginatedCollection<Vlan> listVlans(@QueryParam("networkDomainId") String
networkDomainId,
+ PaginationOptions options);
+
+ @Named("network:vlan")
[minor] similarly to the above comment, maybe `"vlan:list"` ?
> +
+ @Named("network:vlan")
+ @GET
+ @Path("/vlan")
+ @Transform(ParseVlans.ToPagedIterable.class)
+ @ResponseParser(ParseVlans.class)
+ @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
+ PagedIterable<Vlan> listVlans(@QueryParam("networkDomainId") String
networkDomainId);
+
+ @Named("network:listVlansWithNetworkDomainIdAndName")
+ @GET
+ @Path("/vlan")
+ @Transform(ParseVlans.ToPagedIterable.class)
+ @ResponseParser(ParseVlans.class)
+ @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
+ PagedIterable<Vlan>
listVlansWithNetworkDomainIdAndName(@QueryParam("networkDomainId") String
networkDomainId,
why do you need this one and the above `listVlans`?
> +import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
+import javax.ws.rs.core.MediaType;
+import java.util.List;
+
+@RequestFilters({ BasicAuthentication.class, OrganisationIdFilter.class })
+@Consumes(MediaType.APPLICATION_JSON)
+@Path("/{jclouds.api-version}/network")
+public interface NetworkApi {
+
+ @Named("network:deployNetworkDomain")
+ @POST
+ @Path("/deployNetworkDomain")
+ @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ Response deployNetworkDomain(@PayloadParam("datacenterId") String
datacenterId, @PayloadParam("name") String name,
why is that a generic `Response`? I've seen
`DimensionDataCloudControlResponseUtils` wouldn't be possible to have something
like `NetworkDomain` as return value?
> +
+ @Named("network:list")
+ @GET
+ @Path("/networkDomain")
+ @Transform(ParseNetworkDomains.ToPagedIterable.class)
+ @ResponseParser(ParseNetworkDomains.class)
+ @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
+ PagedIterable<NetworkDomain> listNetworkDomains();
+
+ @Named("network:deleteNetworkDomain")
+ @POST
+ @Path("/deleteNetworkDomain")
+ @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ @Fallback(NullOnNotFoundOr404.class)
+ Response deleteNetworkDomain(@PayloadParam("id") String networkDomainId);
generally jclouds uses `@Fallback(VoidOnNotFoundOr404.class)` and uses `void
deleteNetworkDomain(...)`
do you think `Response` is more useful here?
> + @Named("network:listVlansWithNetworkDomainIdAndName")
+ @GET
+ @Path("/vlan")
+ @Transform(ParseVlans.ToPagedIterable.class)
+ @ResponseParser(ParseVlans.class)
+ @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
+ PagedIterable<Vlan>
listVlansWithNetworkDomainIdAndName(@QueryParam("networkDomainId") String
networkDomainId,
+ @QueryParam("name") String name);
+
+ @Named("network:deleteVlan")
+ @POST
+ @Path("/deleteVlan")
+ @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ @Fallback(NullOnNotFoundOr404.class)
+ Response deleteVlan(@PayloadParam("id") String vlanId);
generally jclouds uses @Fallback(VoidOnNotFoundOr404.class) and uses void
delete*(...)
do you think Response is more useful here?
> +
+ @Named("network:publicIpBlock")
+ @GET
+ @Path("/publicIpBlock")
+ @Transform(ParsePublicIpBlocks.ToPagedIterable.class)
+ @ResponseParser(ParsePublicIpBlocks.class)
+ @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
+ PagedIterable<PublicIpBlock>
listPublicIPv4AddressBlocks(@QueryParam("networkDomainId") String
networkDomainId);
+
+ @Named("network:removePublicIpBlock")
+ @POST
+ @Path("/removePublicIpBlock")
+ @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ @Fallback(NullOnNotFoundOr404.class)
+ Response removePublicIpBlock(@PayloadParam("id") String publicIpBlockId);
jclouds uses @Fallback(VoidOnNotFoundOr404.class) and uses void delete*(...)
do you think Response is more useful here?
> + @ResponseParser(ParseNatRules.class)
+ @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
+ PagedIterable<NatRule> listNatRules(@QueryParam("networkDomainId") String
networkDomainId);
+
+ @Named("server:getNatRule")
+ @GET
+ @Path("/natRule/{id}")
+ @Fallback(NullOnNotFoundOr404.class)
+ NatRule getNatRule(@PathParam("id") String natRuleId);
+
+ @Named("network:deleteNatRule")
+ @POST
+ @Path("/deleteNatRule")
+ @Produces(MediaType.APPLICATION_JSON)
+ @MapBinder(BindToJsonPayload.class)
+ Response deleteNatRule(@PayloadParam("id") String natRuleId);
generally jclouds uses @Fallback(VoidOnNotFoundOr404.class) and uses void
delete*(...)
do you think Response is more useful 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/389#pullrequestreview-37874891
