Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
nacx commented on this pull request.
> + client = view.unwrapApi(AWSEC2Api.class).getVPCApi().get();
+ }
+
+ @Test
+ public void testCreate() {
+ vpc = client.createVpc(null, "10.0.0.0/16", CreateVpcOptions.NONE);
+ assertNotNull(vpc);
+ }
+
+ @Test(dependsOnMethods = "testCreate")
+ public void testGet() {
+ FluentIterable vpcs = client.describeVpcsInRegion(null, vpc.id());
+ assertTrue(vpcs.toList().size() == 1);
+ }
+
+ @Test(dependsOnMethods = "testGet")
I'd consider taht a low-priority and definitely not in the scope of this PR
--
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/1032
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
Squashed and merged at [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/8649c3ff) backported at [1.9.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/7158b94b) thanks @nacx for the review! -- 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/1032#issuecomment-257304267
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
Closed #1032. -- 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/1032#event-841950796
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
LGTM. Thanks @andreaturli! -- 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/1032#issuecomment-257295122
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
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/pull/1032#pullrequestreview-6435963
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
@andreaturli pushed 1 commit. 77ef915 address comments for PR 1032 -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/jclouds/jclouds/pull/1032/files/55c205a2c08959e97ce079f9c2f2d901d57ea524..77ef91522940ee4b60cddea5286e08ba8b1aafbf
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
andreaturli commented on this pull request.
> + client = view.unwrapApi(AWSEC2Api.class).getVPCApi().get();
+ }
+
+ @Test
+ public void testCreate() {
+ vpc = client.createVpc(null, "10.0.0.0/16", CreateVpcOptions.NONE);
+ assertNotNull(vpc);
+ }
+
+ @Test(dependsOnMethods = "testCreate")
+ public void testGet() {
+ FluentIterable vpcs = client.describeVpcsInRegion(null, vpc.id());
+ assertTrue(vpcs.toList().size() == 1);
+ }
+
+ @Test(dependsOnMethods = "testGet")
Possibly but I'd like to refactor all of the liveTests in aws-e2 rather than
only the new `VPCApiLiveTest`, 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/pull/1032
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
andreaturli commented on this pull request.
> + VPC() {}
+
+ public static Builder builder() {
+ return new AutoValue_VPC.Builder();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder id(String id);
+ public abstract Builder state(State state);
+ public abstract Builder cidrBlock(String cidrBlock);
+ public abstract Builder dhcpOptionsId(String dhcpOptionsId);
+ public abstract Builder instanceTenancy(String instanceTenancy);
+ public abstract Builder isDefault(Boolean isDefault);
+ public abstract Builder tags(Map tags);
I did it!?!? Amazing, isn't 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/pull/1032
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
nacx commented on this pull request. > + } + } + } + + @Nullable + public abstract String id(); + @Nullable + public abstract State state(); + @Nullable + public abstract String cidrBlock(); + @Nullable + public abstract String dhcpOptionsId(); + @Nullable + public abstract String instanceTenancy(); + @Nullable + public abstract Boolean isDefault(); Ok! :) -- 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/1032
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
nacx commented on this pull request.
> + VPC() {}
+
+ public static Builder builder() {
+ return new AutoValue_VPC.Builder();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder id(String id);
+ public abstract Builder state(State state);
+ public abstract Builder cidrBlock(String cidrBlock);
+ public abstract Builder dhcpOptionsId(String dhcpOptionsId);
+ public abstract Builder instanceTenancy(String instanceTenancy);
+ public abstract Builder isDefault(Boolean isDefault);
+ public abstract Builder tags(Map tags);
Oops. Have a look
[here](https://github.com/jclouds/jclouds-labs-openstack/blob/f831f59a1df246a9f6c20fc9b458d4ae971ef8ef/openstack-neutron/src/main/java/org/jclouds/openstack/neutron/v2/domain/CreateFirewallPolicy.java#L127-L139).
The idea is to provide a hidden build method, which is the one Google Auto wil
automatically implement, and then manually implement the build method and
enforce immutability there.
--
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/1032
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
andreaturli commented on this pull request.
> + VPC() {}
+
+ public static Builder builder() {
+ return new AutoValue_VPC.Builder();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder id(String id);
+ public abstract Builder state(State state);
+ public abstract Builder cidrBlock(String cidrBlock);
+ public abstract Builder dhcpOptionsId(String dhcpOptionsId);
+ public abstract Builder instanceTenancy(String instanceTenancy);
+ public abstract Builder isDefault(Boolean isDefault);
+ public abstract Builder tags(Map tags);
can't see the link. can you re-post it please?
--
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/1032
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
andreaturli commented on this pull request. > + } + } + } + + @Nullable + public abstract String id(); + @Nullable + public abstract State state(); + @Nullable + public abstract String cidrBlock(); + @Nullable + public abstract String dhcpOptionsId(); + @Nullable + public abstract String instanceTenancy(); + @Nullable + public abstract Boolean isDefault(); I think so, at least looking at http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_Vpc.html where `Required: No` -- 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/1032
Re: [jclouds/jclouds] [aws-ec2] Add CRUD for VPC (#1032)
nacx requested changes on this pull request.
Thanks @andreaturli!
> + VPC() {}
+
+ public static Builder builder() {
+ return new AutoValue_VPC.Builder();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder id(String id);
+ public abstract Builder state(State state);
+ public abstract Builder cidrBlock(String cidrBlock);
+ public abstract Builder dhcpOptionsId(String dhcpOptionsId);
+ public abstract Builder instanceTenancy(String instanceTenancy);
+ public abstract Builder isDefault(Boolean isDefault);
+ public abstract Builder tags(Map tags);
Enforce an immutable map by providing a hidden method for auto-value. [This is
the pattern](d31a85af1fa1b7de3676946ce9eaf29ad23e3b1f) we use.
> + PENDING, UNRECOGNIZED;
+ public String value() {
+ return name().toLowerCase();
+ }
+
+ public static State fromValue(String v) {
+ try {
+return valueOf(v.toUpperCase());
+ } catch (IllegalArgumentException e) {
+return UNRECOGNIZED;
+ }
+ }
+ }
+
+ @Nullable
+ public abstract String id();
Is this really nullable?
> + }
+ }
+ }
+
+ @Nullable
+ public abstract String id();
+ @Nullable
+ public abstract State state();
+ @Nullable
+ public abstract String cidrBlock();
+ @Nullable
+ public abstract String dhcpOptionsId();
+ @Nullable
+ public abstract String instanceTenancy();
+ @Nullable
+ public abstract Boolean isDefault();
Same here. Is this really nullable? Use a primitive type if not?
> + */
+@RequestFilters(FormSigner.class)
+@VirtualHost
+public interface VPCApi {
+
+ /**
+* Describes all of your VPCs
+*
+* @return VPCs or empty if there are none
+* @see http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeVpcs.html";
+* >docs
+*/
+ @Named("DescribeVpcs")
+ @POST
+ @Path("/")
Move this annotation to the class and remove it from all methods.
> +import static org.testng.Assert.assertTrue;
+
+import org.jclouds.aws.ec2.AWSEC2Api;
+import org.jclouds.aws.ec2.domain.VPC;
+import org.jclouds.aws.ec2.options.CreateVpcOptions;
+import org.jclouds.compute.internal.BaseComputeServiceContextLiveTest;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.FluentIterable;
+
+/**
+ * Tests behavior of {@code VPCApi}
+ */
+@Test(groups = "live", singleThreaded = true)
+public class VPCApiLiveTest extends BaseComputeServiceContextLiveTest {
SHouldn't this better extend the BaseHttpApiLiveTest?
> + client = view.unwrapApi(AWSEC2Api.class).getVPCApi().get();
+ }
+
+ @Test
+ public void testCreate() {
+ vpc = client.createVpc(null, "10.0.0.0/16", CreateVpcOptions.NONE);
+ assertNotNull(vpc);
+ }
+
+ @Test(dependsOnMethods = "testCreate")
+ public void testGet() {
+ FluentIterable vpcs = client.describeVpcsInRegion(null, vpc.id());
+ assertTrue(vpcs.toList().size() == 1);
+ }
+
+ @Test(dependsOnMethods = "testGet")
Depend on the `testCreate` to avoid skipping this if the get test fails.
> + assertNotNull(vpc);
+ }
+
+ @Test(dependsOnMethods = "testCreate")
+ public void testGet() {
+ FluentIterable vpcs = client.describeVpcsInRegion(null, vpc.id());
+ assertTrue(vpcs.toList().size() == 1);
+ }
+
+ @Test(dependsOnMethods = "testGet")
+ public void testList() {
+ FluentIterable vpcs = client.describeVpcsInRegion(null);
+ assertTrue(!vpcs.toList().isEmpty());
+ }
+
+ @Test(dependsOnMethods = "testList")
`@Test(dependsOnMethods = {"testList", "testGet"}, alwaysRun = true)` to make
sure this is never skipped and we don't leak resources.
> + @EndpointParam(parser = RegionToEndpointOrProviderIfNull.class)
> @Nullable String region,
+ @FormParam("CidrBlock") String cidrBlock, CreateVpcOptions...
options);
+ /**
+* Deletes {@code VPC}.
+*
+* @param region
+* VPCs are tied to the Region where its files are located within
Amazon S3.
+* @param vpcId
+* The VPC ID.
+*
+*/
+ @Named("DeleteVpc")
+ @POST
+ @Path("/")
+ @FormParams(keys = ACTION, values = "DeleteVpc")
+ @XMLResponseParser(ReturnValueHandler.class)
Add a fallback to return false if the vpc does not exist? It is a common
pattern in jclouds.
> + @Test
+ public void testCreate() {
+ vpc = client.createVpc(null, "10.0.0.0/16", CreateVpcOptions.NONE);
+ assertNotNull(vpc);
+ }
+
+ @Test(dependsOnMethods = "testCreate")
+ public void testGet() {
+ FluentIterable vpcs = client.describeVpcsInRegion(null, vpc.id());
+ assertTrue(vpcs.toList().size() == 1);
+ }
+
+ @Test(dependsOnMethods = "testGet")
+ public void testList() {
+ FluentIterable vpcs = client.describeVpcsInRegion(null);
+ assertTrue(!vpcs.toList().isEmpty());
Better use jus
