nacx requested changes on this pull request. Thanks @geomacy!
> +import org.jclouds.json.SerializedNames; +import org.jclouds.logging.Logger; + +import com.google.auto.value.AutoValue; + +/** + * A route in an Amazon EC2 Route Table. + * + * @see <a href="http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_Route.html"; >doc</a> + */ +@AutoValue +public abstract class Route { + + @Resource + @Named(ComputeServiceConstants.COMPUTE_LOGGER) + private static Logger logger = Logger.NULL; This class is not instantiated by Guice, so the logger won't be injected here. Let's just remove the logs in this class; we don't have this kind of warnings in enums. > + + + @Nullable + public abstract String destinationCidrBlock(); + + @Nullable + public abstract String gatewayId(); + + @Nullable + public abstract RouteState state(); + + @Nullable + public abstract String origin(); + + @SerializedNames({"destinationCidrBlock", "gatewayId", "state"}) + public static Route create(String destinationCidrBlock, String gatewayId, RouteState state) { Add the `origin` field too. > + @AutoValue.Builder + public abstract static class Builder { + public abstract Builder destinationCidrBlock(String destinationCidrBlock); + + public abstract Builder gatewayId(String gatewayId); + + public abstract Builder state(RouteState state); + + public abstract Builder origin(String origin); + + @Nullable abstract String destinationCidrBlock(); + @Nullable abstract String gatewayId(); + @Nullable abstract RouteState state(); + @Nullable abstract String origin(); + + abstract Route autoBuild(); We use the hidden `autoBuild` method when we need to customize how we build objects, for example, to enforce immutable collections. It is not needed here. Just a `public abstract Route build();` should suffice. > + } + + @AutoValue.Builder + public abstract static class Builder { + public abstract Builder destinationCidrBlock(String destinationCidrBlock); + + public abstract Builder gatewayId(String gatewayId); + + public abstract Builder state(RouteState state); + + public abstract Builder origin(String origin); + + @Nullable abstract String destinationCidrBlock(); + @Nullable abstract String gatewayId(); + @Nullable abstract RouteState state(); + @Nullable abstract String origin(); These non-visible accessors are not needed. Remove them. We use them when we need access to the internal builder values in a custom build method to enforce immutability. > + + public static Builder builder() { + return new AutoValue_RouteTable.Builder(); + } + + @AutoValue.Builder + public abstract static class Builder { + + public abstract Builder id(String id); + public abstract Builder vpcId(String vpcId); + public abstract Builder routeSet(List<Route> routeSet); + public abstract Builder associationSet(List<RouteTableAssociation> associationSet); + public abstract Builder tags(Map<String, String> tags); + + @Nullable abstract String id(); + @Nullable abstract String vpcId(); Remove these two hidden accessors. > + return new AutoValue_RouteTableAssociation.Builder(); + } + + @AutoValue.Builder + public abstract static class Builder { + public abstract Builder id(String id); + public abstract Builder routeTableId(String routeTableId); + public abstract Builder subnetId(String subnetId); + public abstract Builder main(Boolean main); + + @Nullable abstract String id(); + @Nullable abstract String routeTableId(); + @Nullable abstract String subnetId(); + @Nullable abstract Boolean main(); + + abstract RouteTableAssociation autoBuild(); Same comments as before. Keep in mind to remove the unused hidden accessors. > @@ -32,6 +37,10 @@ @AutoValue public abstract class VPC { + @Resource + @Named(ComputeServiceConstants.COMPUTE_LOGGER) + private static Logger logger = Logger.NULL; Remove this logger too. > + * Delete a {@link RouteTable}, supplying options. + * + * @param region The region to delete the table from + * @param routeTableId The ID of the table to delete + * @param options Options for the request + * @return true if the route table was found and deleted + */ + @Named("DeleteRouteTable") + @POST + @FormParams(keys = ACTION, values = "DeleteRouteTable") + @XMLResponseParser(ReturnValueHandler.class) + @Fallback(Fallbacks.FalseOnNotFoundOr404.class) + boolean deleteRouteTable( + @EndpointParam(parser = RegionToEndpointOrProviderIfNull.class) @Nullable String region, + @FormParam("RouteTableId") String routeTableId, + RouteTableOptions options); This request actually only supports the `dryRun` parameter. Worth changing the signature to just accept a boolean. > + /** + * Creates a {@link RouteTable}, supplying options. + * + * @param region The region to create the table in + * @param vpcId The ID of the VPC + * @param options Options for the request + * @return The route table + */ + @Named("CreateRouteTable") + @POST + @FormParams(keys = ACTION, values = "CreateRouteTable") + @XMLResponseParser(CreateRouteTableResponseHandler.class) + RouteTable createRouteTable( + @EndpointParam(parser = RegionToEndpointOrProviderIfNull.class) @Nullable String region, + @FormParam("VpcId") String vpcId, + RouteTableOptions options); This request actually only supports the dryRun parameter. Worth changing the signature to just accept a boolean. It looks like the options object is more for the `Route` than the `RouteTable` objects. Worth renaming to `RouteOptions`, and create specific options objects for those methods that require them? Otherwise, it can be confusing for users which options can be used with each method. Let's expose just what can be used. > +import org.jclouds.ec2.features.TagApi; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import com.google.common.collect.FluentIterable; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; + +/** + * Tests behavior of {@link RouteTableApi} + */ +@Test(groups = "live") +public class RouteTableApiLiveTest extends BaseApiLiveTest<AWSEC2Api> { + + private static final String TEST_REGION = "eu-west-1"; Better read a System property so users can run the tests against their preferred region? Something like `private static final String TEST_REGION = System.getProperty("jclouds.test.region");` and default to `null` to verify that behaviour too? (jclouds should pick the provider's default region). > + gwApi = api.getInternetGatewayApiForRegion(TEST_REGION).get(); + subnetApi = api.getAWSSubnetApi().get(); + } + + @Test + public void testDescribe() { + vpc = vpcClient.createVpc(TEST_REGION, VPC_CIDR); + assertNotNull(vpc, "Failed to create VPC to test attachments"); + tagger.applyToResources(ImmutableMap.of("Name", simpleName), ImmutableList.of(vpc.id())); + + // When you create a VPC it automatically gets a route table whose single route has the CIDR of the VPC + // and whose "target" is "local". + final FluentIterable<RouteTable> routeTables = routeTableApi.describeRouteTables(TEST_REGION); + assertNotNull(routeTables, "Failed to return list of RouteTables"); + boolean foundVpc = false; + for (RouteTable rt : routeTables) { More idiomatic: ```java Optional<RouteTable> vpcRT = Iterables.tryFind(routeTables, new Predicate<RouteTable>() { @Override public boolean apply(RouteTable input) { return vpc.id().equals(input.vpcId()); } }); assertTrue(vpcRT.isPresent(), "..."); // All other assertions ``` > + gateway = gwApi.createInternetGateway(TEST_REGION, > InternetGatewayOptions.NONE); + assertNotNull(gateway, "Gateway was not successfully created"); + + final Boolean attached = gwApi.attachInternetGateway(TEST_REGION, gateway.id(), vpc.id()); + assertTrue(attached, "Gateway " + gateway.id() + " failed to attach to VPC " + vpc.id()); + + final boolean created = routeTableApi.createRoute(TEST_REGION, routeTable.id(), + gatewayId(gateway.id()) + .destinationCidrBlock(TEST_DESTINATION_CIDR)); + assertTrue(created, "Failed to add route to table " + routeTable.id()); + + final ImmutableList<RouteTable> routeTables = + routeTableApi.describeRouteTables(TEST_REGION, routeTable.id()).toList(); + assertEquals(routeTables.size(), 1, "Could not find existing route table " + routeTable.id()); + boolean foundRoute = false; + for (Route route : routeTables.get(0).routeSet()) { Apply the pattern mentioned above instead of using loops and flags. > + public void testDelete() { + + final ImmutableList<RouteTable> before = + routeTableApi.describeRouteTables(TEST_REGION, routeTable.id()).toList(); + assertEquals(before.size(), 1, "Unexpected response to describe of " + routeTable.id() + ": " + before); + assertEquals(before.get(0).id(), routeTable.id(), "Wrong table returned for " + routeTable.id() + ": " + before); + + final boolean deleted = routeTableApi.deleteRouteTable(TEST_REGION, routeTable.id()); + assertTrue(deleted, "Failed to delete route table " + routeTable.id()); + + final ImmutableList<RouteTable> after = routeTableApi.describeRouteTables(TEST_REGION, routeTable.id()).toList(); + assertEquals(after.size(), 0, "Unexpected response to describe after deleting " + routeTable.id() + ": " + after); + + } + + @AfterClass alwaysRun = true > + "Could not find expected association in routeTable " + > routeTable.id()); + } + + @Test(dependsOnMethods = "testAssociate") + public void testDisassociate() { + final boolean result = routeTableApi.disassociateRouteTable(TEST_REGION, associationId); + assertTrue(result, "Failed to disassociate " + associationId + " from " + routeTable.id()); + + routeTable = routeTableApi.describeRouteTables(TEST_REGION, routeTable.id()).toList().get(0); + assertEquals(routeTable.associationSet().size(), 0, + "Found associations where none should exist in " + routeTable.id() + ": " + routeTable.associationSet()); + + subnetApi.deleteSubnetInRegion(TEST_REGION, subnet.getSubnetId()); + } + + @Test(dependsOnMethods = "testDisassociate") Can it just depend on the `testCreate` to maximize the chances of this test being run in case the associate tests failed? > + + public void describeRouteTables() throws Exception { + enqueueRegions(DEFAULT_REGION); + enqueueXml(DEFAULT_REGION, "/describe_route_tables.xml"); + final ImmutableList<RouteTable> routeTables = routeTableApi().describeRouteTables(DEFAULT_REGION).toList(); + + assertNotNull(routeTables, "Failed to create route table description object"); + assertEquals(routeTables.size(), 3, "Failed to return all entries from test data, returned: " + routeTables); + + for (RouteTable table : routeTables) { + if (ImmutableList.of("rtb-80a3fae4", "rtb-d4605bb0").contains(table.id())) { + assertRoutesForNormalVpc(table, table.id()); + } else if (table.id().equals("rtb-e6c98381")) { + assertRoutesForTestVpc(table, table.id()); + } + } Verify the request that has been sent. > + assertRoutesForNormalVpc(table, table.id()); + } else if (table.id().equals("rtb-e6c98381")) { + assertRoutesForTestVpc(table, table.id()); + } + } + } + + public void describeRouteTablesWithInvalidStateValue() throws Exception { + enqueueRegions(DEFAULT_REGION); + enqueueXml(DEFAULT_REGION, "/describe_route_tables_invalid.xml"); + final ImmutableList<RouteTable> routeTables = routeTableApi().describeRouteTables(DEFAULT_REGION).toList(); + + assertNotNull(routeTables, "Failed to create route table description object"); + assertEquals(routeTables.size(), 1, "Failed to return expected entry from test data, returned: " + routeTables); + + assertEquals(routeTables.get(0).routeSet().get(0).state(), Route.RouteState.UNRECOGNIZED); Verify the request that has been sent. > + } + + public void deleteRoute() throws Exception { + enqueueRegions(DEFAULT_REGION); + enqueueXml(DEFAULT_REGION, "/delete_route.xml"); + final boolean deleted = routeTableApi().deleteRoute(DEFAULT_REGION, "rtb-a77f2ac0", + destinationCidrBlock("172.18.19.0/24")); + assertTrue(deleted, "Failed to match 'true' in test data response"); + + assertPosted(DEFAULT_REGION, "Action=DescribeRegions"); + assertPosted(DEFAULT_REGION, "Action=DeleteRoute&RouteTableId=rtb-a77f2ac0&DestinationCidrBlock=172.18.19.0/24"); + } + + private RouteTableApi routeTableApi() { + return api().getRouteTableApi().get(); + } * Add mock tests that verify the API methods that accept Options objects. * Add mock tests that enqueue 404 responses for all methods that define such fallbacks, to verify their behavior. -- 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/pull/1100#pullrequestreview-39653305
