You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Kris Sterckx <no...@github.com> on 2013/11/07 08:57:03 UTC
[jclouds-labs-openstack] OS Neutron Extension Router (#47)
Hello, I've added the first Neutron v2.0 extension :
* Router
I've run the live tests against a DevStack setup which is using the Havana version of OpenStack.
Find below the contents of my localrc file :
ADMIN_PASSWORD=password
MYSQL_PASSWORD=password
RABBIT_PASSWORD=password
SERVICE_PASSWORD=password
SERVICE_TOKEN=token
disable_service n-net
enable_service q-svc
enable_service q-agt
enable_service q-dhcp
enable_service q-meta
enable_service quantum
enable_service q-lbaas
OVS_ENABLE_TUNNELING=False
NOVA_BRANCH=stable/havana
CINDER_BRANCH=stable/havana
GLANCE_BRANCH=stable/havana
HORIZON_BRANCH=stable/havana
KEYSTONE_BRANCH=stable/havana
NEUTRON_BRANCH=stable/havana
LOGFILE=/opt/stack/logs/stack.sh.log
VERBOSE=True
LOG_COLOR=False
SCREEN_LOGDIR=/opt/stack/logs
OVS_VLAN_RANGES=RegionOne:1:4000
To remove all resources, run the following commands :
neutron router-list | awk '{print $2}' | xargs -I{} neutron router-delete "{}"
neutron port-list | awk '{print $2}' | xargs -I{} neutron port-delete "{}"
neutron subnet-list | awk '{print $2}' | xargs -I{} neutron subnet-delete "{}"
neutron net-list | awk '{print $2}' | xargs -I{} neutron net-delete "{}"
You can merge this Pull Request by running:
git pull https://github.com/KrisSterckx/jclouds-labs-openstack os-neutron-router
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-openstack/pull/47
-- Commit Summary --
* OS Neutron Extension Router
-- File Changes --
M openstack-neutron/pom.xml (1)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/NeutronApi.java (8)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/AllocationPool.java (2)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/BulkNetwork.java (2)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/BulkPort.java (2)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/BulkSubnet.java (2)
A openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/ExternalGatewayInfo.java (105)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/HostRoute.java (2)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/IP.java (2)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/Network.java (2)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/NetworkType.java (7)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/Port.java (2)
A openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/Router.java (151)
A openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/RouterInterface.java (127)
M openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/domain/Subnet.java (2)
A openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/extensions/RouterApi.java (209)
A openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/functions/ParseRouterDetails.java (93)
A openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/functions/ParseRouters.java (93)
A openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/options/CreateRouterOptions.java (168)
A openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/options/EmptyOptions.java (48)
A openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2_0/options/UpdateRouterOptions.java (168)
A openstack-neutron/src/test/java/org/jclouds/openstack/neutron/v2_0/extensions/RouterApiExpectTest.java (295)
A openstack-neutron/src/test/java/org/jclouds/openstack/neutron/v2_0/extensions/RouterApiLiveTest.java (192)
M openstack-neutron/src/test/java/org/jclouds/openstack/neutron/v2_0/features/NetworkApiLiveTest.java (16)
M openstack-neutron/src/test/java/org/jclouds/openstack/neutron/v2_0/features/PortApiLiveTest.java (15)
M openstack-neutron/src/test/java/org/jclouds/openstack/neutron/v2_0/features/SubnetApiLiveTest.java (15)
A openstack-neutron/src/test/java/org/jclouds/openstack/neutron/v2_0/parse/ParseRouterTest.java (55)
A openstack-neutron/src/test/resources/list_routers.json (74)
A openstack-neutron/src/test/resources/router.json (7)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-openstack/pull/47.patch
https://github.com/jclouds/jclouds-labs-openstack/pull/47.diff
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> @@ -37,10 +37,7 @@ public String getValue() {
> }
>
> public static NetworkType fromValue(String value) {
> - for (NetworkType networkType : values()) {
> - if (networkType.getValue().equalsIgnoreCase(value))
> - return networkType;
> - }
> - return null;
> + if (value == null) return null;
This is not a valid input value. But I thought I'd put a null check in to avoid NullPointerExceptions.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7886839
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> @@ -116,6 +122,7 @@
> @Path("/{id}")
> @SelectJson("subnet")
> @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> + @Nullable
> Subnet get(@PathParam("id") String id);
[minor] I think we tend to put `@Nullable` on the same line as the object, i.e.
```
@Fallback(...
@Nullable Subnet get(...
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r9439885
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
Thanks for submitting, @KrisSterckx! Overall, looks good! A couple of comments/question that relate to multiple places:
* A few domain model classes are made `final`. Is this required by the code in some way? Otherwise, could you explain the reasoning?
* Add Javadoc `@see ...` tags to link related interface methods?
* Some interface methods use have `XYZOptions...` as the last parameter. Is the options parameter intended to be optional or mandatory there?
* Could you add descriptive messages to `assertTrue(...)` and `assertFalse(...)` tests? With a message, it's much easier to figure out what went wrong when looking at DEV@cloud or BuildHive.
* In general, jclouds is trying to use MockWebServer tests rather than expect tests, because a real web server uncovers a couple of things that the expect tests have not caught. Not that these tests need to be changed now, but just FYI...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-28137688
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> + * @return the list of all router references configured for the tenant.
> + */
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouters.class)
> + @Transform(ParseRouters.ToPagedIterable.class)
> + @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
> + @QueryParams(keys = {"fields", "fields", "fields"}, values = {"id", "tenant_id", "name"})
> + PagedIterable<? extends ReferenceWithName> list();
> +
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouters.class)
> + @Fallback(KeystoneFallbacks.EmptyPaginatedCollectionOnNotFoundOr404.class)
> + @QueryParams(keys = {"fields", "fields", "fields"}, values = {"id", "tenant_id", "name"})
> + PaginatedCollection<? extends ReferenceWithName> list(PaginationOptions options);
No, didn't even notice I used different return types. I'll use the PagedIterable (and change it in existing APIs too)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7886887
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> + authenticatedGET().endpoint(endpoint + "/routers").addQueryParam("fields", "id", "tenant_id", "name").build(),
> + HttpResponse.builder().statusCode(200).payload(payloadFromResourceWithContentType("/list_routers.json", APPLICATION_JSON)).build())
> + .getRouterExtensionForZone(ZONE).get();
> +
> + Set<? extends ReferenceWithName> references = api.list().concat().toSet();
> + assertEquals(references, listOfReferencesWithNames());
> + }
> +
> + public void testListReferencesReturns4xx() {
> + RouterApi api = requestsSendResponses(
> + keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess,
> + authenticatedGET().endpoint(endpoint + "/routers").addQueryParam("fields", "id", "tenant_id", "name").build(),
> + HttpResponse.builder().statusCode(404).build())
> + .getRouterExtensionForZone(ZONE).get();
> +
> + assertTrue(api.list().concat().isEmpty());
What exactly do you mean by this messages? Because from the code you can tell we will get a 404 HTTP response. And then we test the fact to see if the specified fallback is working as expected. Should I include such messages on each expected test failure?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r8019270
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + @Override
> + public boolean equals(Object obj) {
> + if (this == obj) return true;
> + if (obj == null || getClass() != obj.getClass()) return false;
> + Router that = Router.class.cast(obj);
> + return super.equals(obj)
> + && Objects.equal(this.adminStateUp, that.adminStateUp)
> + && Objects.equal(this.state, that.state)
> + && Objects.equal(this.externalGatewayInfo, that.externalGatewayInfo);
> + }
> +
> + protected Objects.ToStringHelper string() {
> + return super.string()
> + .add("adminStateUp", adminStateUp).add("state", state).add("externalGatewayInfo", externalGatewayInfo != null ? externalGatewayInfo.toString() : "");
You should be able to use [`omitNullValues`](http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/base/Objects.ToStringHelper.html#omitNullValues\(\)) to handle the null case. And I take it adminStateUp and state _can_ be null? Or not?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r9439787
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> +import java.beans.ConstructorProperties;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import static org.jclouds.openstack.v2_0.options.PaginationOptions.Builder.marker;
> +
> +/**
> + * @author Nick Livens
> + */
> +@Beta
> +@Singleton
> +public class ParseRouterDetails extends ParseJson<Routers> {
> + static class Routers extends PaginatedCollection<Router> {
> +
> + @ConstructorProperties({ "routers", "routers_links" })
> + protected Routers(Iterable<Router> routers, Iterable<Link> routers_links) {
> + super(routers, routers_links);
Will do
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887124
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
Thanks for responding to the review comments, @KrisSterckx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-29208427
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> @@ -37,10 +37,7 @@ public String getValue() {
> }
>
> public static NetworkType fromValue(String value) {
> - for (NetworkType networkType : values()) {
> - if (networkType.getValue().equalsIgnoreCase(value))
> - return networkType;
> - }
> - return null;
> + if (value == null) return null;
Is `null` a valid input value here at all?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546851
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Zack Shoylev <no...@github.com>.
> Also, expect tests rather than mock tests here.
That should be ok. Should we go ahead and merge this then?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-36667930
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Everett Toews <no...@github.com>.
I ran all of the LiveTests and everything is running 100%. I used the command
mvn -Plive -Dtest.openstack-neutron.endpoint=http://xxx.xxx.xxx.xxx:5000/v2.0/ -Dtest.openstack-neutron.identity=admin:admin -Dtest.openstack-neutron.credential=devstack clean install
What's left to be done on this PR before we can merge?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-34105642
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> assertTrue(networkApi.delete(net.getId()));
> }
> }
> }
> +
> + private Predicate<Network> createPredicate(final String networkId) {
Rename to "networkIdEquals" or similar?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546907
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> +
> + /**
> + * Returns the list of all routers currently defined in Neutron for the current tenant. The list provides the unique
> + * identifier of each router configured for the tenant
> + *
> + * @return the list of all router references configured for the tenant.
> + */
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouters.class)
> + @Transform(ParseRouters.ToPagedIterable.class)
> + @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
> + @QueryParams(keys = {"fields", "fields", "fields"}, values = {"id", "tenant_id", "name"})
> + PagedIterable<? extends ReferenceWithName> list();
> +
> + @Named("router:list")
Well, there's no link for the overloaded version. It's exactly the same call, but we filter the OS response to only include the defined fields (id, tenant_id and name).
I could remove this method though since it actually has no use.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887008
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + * Returns all routers currently defined in Neutron for the current tenant.
> + *
> + * @return the list of all routers configured for the tenant
> + */
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouterDetails.class)
> + @Transform(ParseRouterDetails.ToPagedIterable.class)
> + @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
> + PagedIterable<? extends Router> listInDetail();
> +
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouterDetails.class)
> + @Fallback(KeystoneFallbacks.EmptyPaginatedCollectionOnNotFoundOr404.class)
> + PaginatedCollection<? extends Router> listInDetail(PaginationOptions options);
Same questions as above regarding collection vs. iterable, and a `@see ...` Javadoc comment to link to overloaded version?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546862
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + protected String network_id;
> +
> + protected ExternalGatewayInfo(String network_id) {
> + this.network_id = network_id;
> + }
> + }
> + }
> +
> + protected String name;
> + protected Boolean adminStateUp;
> + protected ExternalGatewayInfo externalGatewayInfo;
> +
> + protected UpdateRouterOptions() {
> + this.name = null;
> + this.adminStateUp = null;
> + this.externalGatewayInfo = null;
`this(null, null, null)`. But again, where do we need this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r9440124
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @Override
> + protected Function<Object, IterableWithMarker<ReferenceWithName>> markerToNextForArg0(Optional<Object> arg0) {
> + String zone = arg0.isPresent() ? arg0.get().toString() : null;
> + final RouterApi routerApi = api.getRouterExtensionForZone(zone).get();
> + return new Function<Object, IterableWithMarker<ReferenceWithName>>() {
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public IterableWithMarker<ReferenceWithName> apply(Object input) {
> + return IterableWithMarker.class.cast(routerApi.list(marker(input.toString())));
> + }
> +
> + @Override
> + public String toString() {
> + return "list()";
See comment above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546899
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> @@ -37,10 +37,7 @@ public String getValue() {
> }
>
> public static NetworkType fromValue(String value) {
> - for (NetworkType networkType : values()) {
> - if (networkType.getValue().equalsIgnoreCase(value))
> - return networkType;
> - }
> - return null;
> + if (value == null) return null;
If it's not a valid/expected input value, is it sensible to swallow it? If it indicates a problem, should we rather be failing here? If we want to handle spurious nulls more gracefully, add a log warning?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7892634
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + authenticatedGET().endpoint(endpoint + "/routers").addQueryParam("fields", "id", "tenant_id", "name").build(),
> + HttpResponse.builder().statusCode(200).payload(payloadFromResourceWithContentType("/list_routers.json", APPLICATION_JSON)).build())
> + .getRouterExtensionForZone(ZONE).get();
> +
> + Set<? extends ReferenceWithName> references = api.list().concat().toSet();
> + assertEquals(references, listOfReferencesWithNames());
> + }
> +
> + public void testListReferencesReturns4xx() {
> + RouterApi api = requestsSendResponses(
> + keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess,
> + authenticatedGET().endpoint(endpoint + "/routers").addQueryParam("fields", "id", "tenant_id", "name").build(),
> + HttpResponse.builder().statusCode(404).build())
> + .getRouterExtensionForZone(ZONE).get();
> +
> + assertTrue(api.list().concat().isEmpty());
Add message to help understand test failure?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546903
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Bayer <no...@github.com>.
Closed #47.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> @@ -29,7 +29,7 @@
> * @author Nick Livens
> * @see <a href="http://docs.openstack.org/api/openstack-network/2.0/content/Subnets.html">api doc</a>
> */
> -public class AllocationPool {
> +public final class AllocationPool {
Ah, yes, I recall. Thanks for the reminder, @KrisSterckx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7892595
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> @@ -29,7 +29,7 @@
> * @author Nick Livens
> * @see <a href="http://docs.openstack.org/api/openstack-network/2.0/content/Subnets.html">api doc</a>
> */
> -public class AllocationPool {
> +public final class AllocationPool {
On further thought, I'm going to remove the final again since everyone should be able to subclass these classes
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r8019079
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + @Path("/{id}")
> + @SelectJson("router")
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> + Router get(@PathParam("id") String id);
> +
> + /**
> + * Create a new router
> + *
> + * @param options optional arguments
> + * @return the newly created router
> + */
> + @Named("router:create")
> + @POST
> + @SelectJson("router")
> + @MapBinder(CreateRouterOptions.class)
> + Router create(CreateRouterOptions... options);
An array of options objects?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546865
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + *
> + * @author Nick Livens
> + * @see <a href=
> + * "http://docs.openstack.org/api/openstack-network/2.0/content/router_ext.html">api doc</a>
> + */
> +@Path("/v2.0/routers")
> +@RequestFilters(AuthenticateRequest.class)
> +@Consumes(MediaType.APPLICATION_JSON)
> +public interface RouterApi {
> +
> + /**
> + * Returns the list of all routers currently defined in Neutron for the current tenant. The list provides the unique
> + * identifier of each router configured for the tenant
> + *
> + * @return the list of all router references configured for the tenant.
> + */
Add a `@see ...` Javadoc comment to link to overloaded version?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546858
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
@KrisSterckx Review might take a bit this time. We're a bit tied up with the 1.7.0 release right now, so thanks for your patience... ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-31026137
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #129](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/129/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-30919568
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> assertTrue(subnetApi.delete(net.getId()));
> }
> assertTrue(networkApi.delete(networkId));
> }
> }
> +
> + private Predicate<Subnet> createPredicate(final String subnetId) {
See above comment
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887248
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + @ResponseParser(ParseRouterDetails.class)
> + @Fallback(KeystoneFallbacks.EmptyPaginatedCollectionOnNotFoundOr404.class)
> + PaginatedCollection<? extends Router> listInDetail(PaginationOptions options);
> +
> + /**
> + * Returns the specific router.
> + *
> + * @param id the id of the router to return
> + * @return Router or null if not found
> + */
> + @Named("router:get")
> + @GET
> + @Path("/{id}")
> + @SelectJson("router")
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> + Router get(@PathParam("id") String id);
Add `@Nullable` to return type?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546863
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> +
> + /**
> + * Returns the list of all routers currently defined in Neutron for the current tenant. The list provides the unique
> + * identifier of each router configured for the tenant
> + *
> + * @return the list of all router references configured for the tenant.
> + */
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouters.class)
> + @Transform(ParseRouters.ToPagedIterable.class)
> + @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
> + @QueryParams(keys = {"fields", "fields", "fields"}, values = {"id", "tenant_id", "name"})
> + PagedIterable<? extends ReferenceWithName> list();
> +
> + @Named("router:list")
> I could remove this method though since it actually has no use.
If it's not used, indeed remove? Same functionality with less code...nice!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7892651
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + .adminStateUp(options.getAdminStateUp())
> + .externalGatewayInfo(options.getExternalGatewayInfo());
> + }
> + }
> +
> + private static class ConcreteBuilder extends Builder<ConcreteBuilder> {
> + @Override
> + protected ConcreteBuilder self() {
> + return this;
> + }
> + }
> +
> + protected static class UpdateRouterRequest {
> + protected String name;
> + protected Boolean admin_state_up;
> + protected ExternalGatewayInfo external_gateway_info;
`adminStateUp` and `externalGatewayInfo`? I don't think we use underscore_delimited_vars in jclouds (much).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r9440071
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #130](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/130/) FAILURE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-30919673
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> +
> + /**
> + * Returns the list of all routers currently defined in Neutron for the current tenant. The list provides the unique
> + * identifier of each router configured for the tenant
> + *
> + * @return the list of all router references configured for the tenant.
> + */
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouters.class)
> + @Transform(ParseRouters.ToPagedIterable.class)
> + @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
> + @QueryParams(keys = {"fields", "fields", "fields"}, values = {"id", "tenant_id", "name"})
> + PagedIterable<? extends ReferenceWithName> list();
> +
> + @Named("router:list")
Add a `@see ...` Javadoc comment to link to overloaded version?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546860
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Zack Shoylev <no...@github.com>.
This should be closed. merged https://github.com/jclouds/jclouds-labs-openstack/pull/83 instead.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-38584962
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + authenticatedGET().endpoint(endpoint + "/routers").addQueryParam("fields", "id", "tenant_id", "name").build(),
> + HttpResponse.builder().statusCode(200).payload(payloadFromResourceWithContentType("/list_routers.json", APPLICATION_JSON)).build())
> + .getRouterExtensionForZone(ZONE).get();
> +
> + Set<? extends ReferenceWithName> references = api.list().concat().toSet();
> + assertEquals(references, listOfReferencesWithNames());
> + }
> +
> + public void testListReferencesReturns4xx() {
> + RouterApi api = requestsSendResponses(
> + keystoneAuthWithUsernameAndPasswordAndTenantName, responseWithKeystoneAccess,
> + authenticatedGET().endpoint(endpoint + "/routers").addQueryParam("fields", "id", "tenant_id", "name").build(),
> + HttpResponse.builder().statusCode(404).build())
> + .getRouterExtensionForZone(ZONE).get();
> +
> + assertTrue(api.list().concat().isEmpty());
> What exactly do you mean by this messages?
I'll start out by admitting that I am much more interested in this than the other jclouds reviewers, mainly because I often end up clicking on the PR builder links and looking at the notification emails to try to figure out build status. So I'll be fine if we leave things as-is.
Having said that, what I mean is writing assertions like
```
assertTrue(api.list().concat().isEmpty(), "Expected no results to be returned, but was " + api.list().concat());
```
or similar, rather than just
```
assertTrue(api.list().concat().isEmpty());
```
With the latter style, if you drill down to the code you obviously can also see what the test is trying to do. But in a Jenkins report all you get is "Assertion failed", and then I usually have to go over to GitHub to drill down into the source code to figure out what's going on.
For tests with only _one_ assertion the test name itself can be enough, but in this case, for instance, it would also be nice to know _what_ was returned, rather than just that _something_ was returned.
Does this make some kind of sense? ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r8063301
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> +import java.beans.ConstructorProperties;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import static org.jclouds.openstack.v2_0.options.PaginationOptions.Builder.marker;
> +
> +/**
> + * @author Nick Livens
> + */
> +@Beta
> +@Singleton
> +public class ParseRouterDetails extends ParseJson<Routers> {
> + static class Routers extends PaginatedCollection<Router> {
> +
> + @ConstructorProperties({ "routers", "routers_links" })
> + protected Routers(Iterable<Router> routers, Iterable<Link> routers_links) {
> + super(routers, routers_links);
If we're going to use `@ConstructorProperties` here, I think we can use "Java" names for the actual vars? I.e. `routerLinks` or `routersLinks` rather than `routers_links`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546872
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Zack Shoylev <no...@github.com>.
Ok the plan is to merge this and then I will be working on it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-38487773
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Zack Shoylev <no...@github.com>.
Oops! But this is the one that will be merged if I did it right: https://github.com/jclouds/jclouds-labs-openstack/pull/83
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-38584011
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #131](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/131/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-30920216
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + }
> +
> + public Builder toBuilder() {
> + return new ConcreteBuilder().fromExternalGatewayInfo(this);
> + }
> +
> + public static abstract class Builder {
> + protected abstract Builder self();
> +
> + protected String networkId;
> +
> + /**
> + * @see ExternalGatewayInfo#getNetworkId()
> + */
> + public Builder networkId(String networkId) {
> + this.networkId = networkId;
Null check here or in the class' constructor? Or can `networkId` be null..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546850
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Zack Shoylev <no...@github.com>.
I can merge it any time, but probably will not have time for a follow-up PR this week.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-37213484
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @Override
> + protected Function<Object, IterableWithMarker<Router>> markerToNextForArg0(Optional<Object> arg0) {
> + String zone = arg0.isPresent() ? arg0.get().toString() : null;
> + final RouterApi routerApi = api.getRouterExtensionForZone(zone).get();
> + return new Function<Object, IterableWithMarker<Router>>() {
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public IterableWithMarker<Router> apply(Object input) {
> + return IterableWithMarker.class.cast(routerApi.listInDetail(marker(input.toString())));
> + }
> +
> + @Override
> + public String toString() {
> + return "list()";
Remove the `toString` or give it a more descriptive result?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546898
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> That should be ok. Should we go ahead and merge this then?
Would be grateful for some responses to the outstanding comments? Even if it's "if these are not blockers, can we please merge and address in a follow-up"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-36741122
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> +
> + @Override
> + protected Function<Object, IterableWithMarker<ReferenceWithName>> markerToNextForArg0(Optional<Object> arg0) {
> + String zone = arg0.isPresent() ? arg0.get().toString() : null;
> + final RouterApi routerApi = api.getRouterExtensionForZone(zone).get();
> + return new Function<Object, IterableWithMarker<ReferenceWithName>>() {
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public IterableWithMarker<ReferenceWithName> apply(Object input) {
> + return IterableWithMarker.class.cast(routerApi.list(marker(input.toString())));
> + }
> +
> + @Override
> + public String toString() {
> + return "list()";
:-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7892664
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> @@ -29,7 +29,7 @@
> * @author Nick Livens
> * @see <a href="http://docs.openstack.org/api/openstack-network/2.0/content/Subnets.html">api doc</a>
> */
> -public class AllocationPool {
> +public final class AllocationPool {
You said we should make those classes final which are not meant to be subclassed. So I changed every single class which wasn't meant to be subclassed to final.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7886702
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + * @return the list of all router references configured for the tenant.
> + */
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouters.class)
> + @Transform(ParseRouters.ToPagedIterable.class)
> + @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
> + @QueryParams(keys = {"fields", "fields", "fields"}, values = {"id", "tenant_id", "name"})
> + PagedIterable<? extends ReferenceWithName> list();
> +
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouters.class)
> + @Fallback(KeystoneFallbacks.EmptyPaginatedCollectionOnNotFoundOr404.class)
> + @QueryParams(keys = {"fields", "fields", "fields"}, values = {"id", "tenant_id", "name"})
> + PaginatedCollection<? extends ReferenceWithName> list(PaginationOptions options);
Any reason for collection vs. iterable return type above?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546857
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + *
> + * @author Nick Livens
> + * @see <a href=
> + * "http://docs.openstack.org/api/openstack-network/2.0/content/router_ext.html">api doc</a>
> + */
> +@Path("/v2.0/routers")
> +@RequestFilters(AuthenticateRequest.class)
> +@Consumes(MediaType.APPLICATION_JSON)
> +public interface RouterApi {
> +
> + /**
> + * Returns the list of all routers currently defined in Neutron for the current tenant. The list provides the unique
> + * identifier of each router configured for the tenant
> + *
> + * @return the list of all router references configured for the tenant.
> + */
> I could remove this method though since it actually has no use.
If possible, yes, please remove if not needed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r9439834
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> +
> + @Override
> + protected Function<Object, IterableWithMarker<Router>> markerToNextForArg0(Optional<Object> arg0) {
> + String zone = arg0.isPresent() ? arg0.get().toString() : null;
> + final RouterApi routerApi = api.getRouterExtensionForZone(zone).get();
> + return new Function<Object, IterableWithMarker<Router>>() {
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public IterableWithMarker<Router> apply(Object input) {
> + return IterableWithMarker.class.cast(routerApi.listInDetail(marker(input.toString())));
> + }
> +
> + @Override
> + public String toString() {
> + return "list()";
I'll give it a more descriptive result
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887157
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Zack Shoylev <no...@github.com>.
Some issue patching and looking into it atm.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-38497005
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> assertTrue(subnetApi.delete(net.getId()));
> }
> assertTrue(networkApi.delete(networkId));
> }
> }
> +
> + private Predicate<Subnet> createPredicate(final String subnetId) {
See above comment. And since we're using this in multiple places, move out to a test util class?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546910
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> What's left to be done on this PR before we can merge?
@everett-toews: Just added some comments about variable names and a couple of other minor points. Also, expect tests rather than mock tests here.
>From my perspective, neither is something that couldn't be addressed in a follow-up PR if we want to get this in sooner.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-34110632
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> + *
> + * @author Nick Livens
> + * @see <a href=
> + * "http://docs.openstack.org/api/openstack-network/2.0/content/router_ext.html">api doc</a>
> + */
> +@Path("/v2.0/routers")
> +@RequestFilters(AuthenticateRequest.class)
> +@Consumes(MediaType.APPLICATION_JSON)
> +public interface RouterApi {
> +
> + /**
> + * Returns the list of all routers currently defined in Neutron for the current tenant. The list provides the unique
> + * identifier of each router configured for the tenant
> + *
> + * @return the list of all router references configured for the tenant.
> + */
Well, there's no link for the overloaded version. It's exactly the same call, but we filter the OS response to only include the defined fields (id, tenant_id and name).
I could remove this method though since it actually has no use.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887002
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> + @ResponseParser(ParseRouterDetails.class)
> + @Fallback(KeystoneFallbacks.EmptyPaginatedCollectionOnNotFoundOr404.class)
> + PaginatedCollection<? extends Router> listInDetail(PaginationOptions options);
> +
> + /**
> + * Returns the specific router.
> + *
> + * @param id the id of the router to return
> + * @return Router or null if not found
> + */
> + @Named("router:get")
> + @GET
> + @Path("/{id}")
> + @SelectJson("router")
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> + Router get(@PathParam("id") String id);
Will do
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887039
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> +
> + @Override
> + protected Function<Object, IterableWithMarker<ReferenceWithName>> markerToNextForArg0(Optional<Object> arg0) {
> + String zone = arg0.isPresent() ? arg0.get().toString() : null;
> + final RouterApi routerApi = api.getRouterExtensionForZone(zone).get();
> + return new Function<Object, IterableWithMarker<ReferenceWithName>>() {
> +
> + @SuppressWarnings("unchecked")
> + @Override
> + public IterableWithMarker<ReferenceWithName> apply(Object input) {
> + return IterableWithMarker.class.cast(routerApi.list(marker(input.toString())));
> + }
> +
> + @Override
> + public String toString() {
> + return "list()";
See comment above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887194
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + return this;
> + }
> + }
> +
> + protected static class UpdateRouterRequest {
> + protected String name;
> + protected Boolean admin_state_up;
> + protected ExternalGatewayInfo external_gateway_info;
> +
> + protected UpdateRouterRequest() {
> + }
> +
> + protected static final class ExternalGatewayInfo {
> + protected String network_id;
> +
> + protected ExternalGatewayInfo(String network_id) {
`networkId`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r9440102
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + */
> + public T state(State state) {
> + this.state = state;
> + return self();
> + }
> +
> + /**
> + * @see Router#getExternalGatewayInfo()
> + */
> + public T externalGatewayInfo(ExternalGatewayInfo externalGatewayInfo) {
> + this.externalGatewayInfo = externalGatewayInfo;
> + return self();
> + }
> +
> + public Router build() {
> + return new Router(id, tenantId, name, adminStateUp, state, externalGatewayInfo);
Null checks for any of the fields, here on in the constructor?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546853
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> assertTrue(networkApi.delete(net.getId()));
> }
> }
> }
> +
> + private Predicate<Network> createPredicate(final String networkId) {
I'll move this method to a test util class and make it more generic.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887246
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #88](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/88/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-27944126
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> + @Path("/{id}")
> + @SelectJson("router")
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> + Router get(@PathParam("id") String id);
> +
> + /**
> + * Create a new router
> + *
> + * @param options optional arguments
> + * @return the newly created router
> + */
> + @Named("router:create")
> + @POST
> + @SelectJson("router")
> + @MapBinder(CreateRouterOptions.class)
> + Router create(CreateRouterOptions... options);
Optional
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887105
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> + protected String network_id;
> +
> + protected ExternalGatewayInfo(String network_id) {
> + this.network_id = network_id;
> + }
> + }
> + }
> +
> + protected String name;
> + protected Boolean adminStateUp;
> + protected ExternalGatewayInfo externalGatewayInfo;
> +
> + protected CreateRouterOptions() {
> + this.name = null;
> + this.adminStateUp = null;
> + this.externalGatewayInfo = null;
Simply call
```
this(null, null, null)
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r9440015
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> + * Returns all routers currently defined in Neutron for the current tenant.
> + *
> + * @return the list of all routers configured for the tenant
> + */
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouterDetails.class)
> + @Transform(ParseRouterDetails.ToPagedIterable.class)
> + @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
> + PagedIterable<? extends Router> listInDetail();
> +
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouterDetails.class)
> + @Fallback(KeystoneFallbacks.EmptyPaginatedCollectionOnNotFoundOr404.class)
> + PaginatedCollection<? extends Router> listInDetail(PaginationOptions options);
Look above comments.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887020
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> + }
> +
> + public Builder toBuilder() {
> + return new ConcreteBuilder().fromExternalGatewayInfo(this);
> + }
> +
> + public static abstract class Builder {
> + protected abstract Builder self();
> +
> + protected String networkId;
> +
> + /**
> + * @see ExternalGatewayInfo#getNetworkId()
> + */
> + public Builder networkId(String networkId) {
> + this.networkId = networkId;
I'll add null checks to obligated fields.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887955
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Everett Toews <no...@github.com>.
I'm okay with this being merged as long as a follow up PR can be done right away.
Will you have time for that this week @zack-shoylev?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47#issuecomment-37209984
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> + */
> + public T state(State state) {
> + this.state = state;
> + return self();
> + }
> +
> + /**
> + * @see Router#getExternalGatewayInfo()
> + */
> + public T externalGatewayInfo(ExternalGatewayInfo externalGatewayInfo) {
> + this.externalGatewayInfo = externalGatewayInfo;
> + return self();
> + }
> +
> + public Router build() {
> + return new Router(id, tenantId, name, adminStateUp, state, externalGatewayInfo);
I'll add null checks to the obligated fields
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7887945
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Kris Sterckx <no...@github.com>.
> +
> + /**
> + * Returns the list of all routers currently defined in Neutron for the current tenant. The list provides the unique
> + * identifier of each router configured for the tenant
> + *
> + * @return the list of all router references configured for the tenant.
> + */
> + @Named("router:list")
> + @GET
> + @ResponseParser(ParseRouters.class)
> + @Transform(ParseRouters.ToPagedIterable.class)
> + @Fallback(Fallbacks.EmptyPagedIterableOnNotFoundOr404.class)
> + @QueryParams(keys = {"fields", "fields", "fields"}, values = {"id", "tenant_id", "name"})
> + PagedIterable<? extends ReferenceWithName> list();
> +
> + @Named("router:list")
On further thought, the overloaded version is used by the parser classes. I'm adding a @see javadoc comment
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r8019248
Re: [jclouds-labs-openstack] OS Neutron Extension Router (#47)
Posted by Andrew Phillips <no...@github.com>.
> @@ -29,7 +29,7 @@
> * @author Nick Livens
> * @see <a href="http://docs.openstack.org/api/openstack-network/2.0/content/Subnets.html">api doc</a>
> */
> -public class AllocationPool {
> +public final class AllocationPool {
Any reason for making this and the domain classes below final? This may be something discussed as part of the original PR, but if so I'm afraid I don't recall the details ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/47/files#r7546845