nacx requested changes on this pull request.
Thanks, @geomacy! Most comments I added are minor and the PR looks GREAT.
Thanks for taking the time to write proper mock and live tests (and for the
meaningful assert messages; they help a lot when things fail :)).
Thanks for starting the work on these much-needed APIs!
> @@ -132,4 +133,11 @@
@Delegate
Optional<? extends AWSSubnetApi> getAWSSubnetApiForRegion(
@EndpointParam(parser = RegionToEndpointOrProviderIfNull.class)
@Nullable String region);
+
+
+ /**
+ * Provides synchronous access to InternetGateway services.
+ */
+ @Delegate
+ Optional<? extends InternetGatewayApi> getInternetGatewayApi();
Should we better provide the `InRegion` variant for consistency with other APIs?
> + * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.aws.ec2.binders;
+
+import org.jclouds.aws.util.AWSUtils;
+import org.jclouds.http.HttpRequest;
+import org.jclouds.rest.Binder;
+
+/**
+ * Binds the String [] to form parameters named with InstanceId.index
InternetGatewayId.index
> + }
+
+ InternetGateway() {}
+
+ public static Builder builder() {
+ return new AutoValue_InternetGateway.Builder();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder id(String id);
+ public abstract Builder attachmentSet(List<InternetGatewayAttachment>
attachmentSet);
+ public abstract Builder tags(Map<String, String> tags);
+
+ @Nullable public abstract String id();
Remove this method. If you need to access it from the builder, you could add a
`toBuilder` method to the InternetGateway object.
> + InternetGateway() {}
+
+ public static Builder builder() {
+ return new AutoValue_InternetGateway.Builder();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder id(String id);
+ public abstract Builder attachmentSet(List<InternetGatewayAttachment>
attachmentSet);
+ public abstract Builder tags(Map<String, String> tags);
+
+ @Nullable public abstract String id();
+ @Nullable public abstract List<InternetGatewayAttachment>
attachmentSet();
+ @Nullable public abstract Map<String, String> tags();
Hide these methods by removing the public modifier, like `autoBuid`
> + public static Builder builder() {
+ return new AutoValue_InternetGatewayAttachment.Builder();
+ }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder state(State state);
+
+ public abstract Builder vpcId(String vpcId);
+
+ @Nullable
+ public abstract State state();
+
+ @Nullable
+ public abstract String vpcId();
Remove these two accessors from the builder.
> + }
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder state(State state);
+
+ public abstract Builder vpcId(String vpcId);
+
+ @Nullable
+ public abstract State state();
+
+ @Nullable
+ public abstract String vpcId();
+
+ abstract InternetGatewayAttachment autoBuild();
We don't really need this since we are not customizing the default builder
behavior. Let's remove this and leave only the abstract build() method.
> + *
+ * @param region Region where the VPC exists
+ * @param internetGatewayId ID of the gateway to detach
+ * @param vpcId The ID of the VPC
+ * @param options Options for the request
+ */
+ @Named("DetachInternetGateway")
+ @POST
+ @FormParams(keys = ACTION, values = "DetachInternetGateway")
+ @XMLResponseParser(ReturnValueHandler.class)
+ @Fallback(FalseOnNotFoundOr404.class)
+ Boolean detachInternetGateway(
+ @EndpointParam(parser = RegionToEndpointOrProviderIfNull.class)
@Nullable String region,
+ @FormParam("InternetGatewayId") String internetGatewayId,
+ @FormParam("VpcId") String vpcId,
+ InternetGatewayOptions... options);
I know it is already a pattern in AWS, but it is legacy and wrong: I'd change
the varargs parameter with an overloaded variant: one method with the options
parameter and one without it. We actually don't support passing more than one
options object, so the varargs parameter is not a good method signature. It's
not the right way to model optional parameters.
> + * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.jclouds.aws.ec2.options;
+
+import org.jclouds.ec2.options.internal.BaseEC2RequestOptions;
+
+/**
+ * Contains options supported in the Form API for the InternetGateway
operations. <h2>
+ * Usage</h2> The recommended way to instantiate such an object is to
statically import
+ * CreateInternetGateway.Builder.* and invoke a static creation method
followed by an instance mutator
InternetGatewayOptions.Builder
> +import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSet.Builder;
+import com.google.inject.Inject;
+
+/**
+ * @see <a
href="http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeVpcs.html";>xml</a>
+ */
+public class DescribeInternetGatewaysResponseHandler extends
+
ParseSax.HandlerForGeneratedRequestWithResult<FluentIterable<InternetGateway>> {
+ private final InternetGatewayHandler gatewayHandler;
+ private boolean inAttachmentSet;
+ private boolean inTagSet;
+ private Builder<InternetGateway> gateways = ImmutableSet.builder();
+
+ @Inject
+ public DescribeInternetGatewaysResponseHandler(InternetGatewayHandler
gatewayHandler) {
Reduce the injection constructor visibility by removing the public modifier.
> +import org.xml.sax.Attributes;
+
+/**
+ * @see <a
href="http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_InternetGateway.html";>InternetGateway
docs</a>
+ */
+public class InternetGatewayHandler extends
ParseSax.HandlerForGeneratedRequestWithResult<InternetGateway> {
+ private StringBuilder currentText = new StringBuilder();
+ private InternetGateway.Builder builder = InternetGateway.builder();
+ private final TagSetHandler tagSetHandler;
+ private final InternetGatewayAttachmentSetHandler attachmentSetHandler;
+ private boolean inTagSet;
+ private boolean inAttachmentSet;
+
+
+ @Inject
+ public InternetGatewayHandler(TagSetHandler tagSetHandler,
InternetGatewayAttachmentSetHandler attachmentHandler) {
Remove the public modifier.
> +import org.jclouds.ec2.features.TagApi;
+import org.jclouds.logging.Logger;
+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 InternetGatewayApi}
+ */
+@Test(groups = "live", singleThreaded = true)
+public class InternetGatewayApiLiveTest extends
BaseComputeServiceContextLiveTest {
+
+ private Logger logger = Logger.CONSOLE;
Test classes are not instantiated by Guice so the jclouds logger won't be
properly initialized. In tests we use the
`java.util.logging.Logger.getAnonymousLogger()`.
> +import org.jclouds.aws.ec2.options.InternetGatewayOptions;
+import org.jclouds.compute.internal.BaseComputeServiceContextLiveTest;
+import org.jclouds.ec2.features.TagApi;
+import org.jclouds.logging.Logger;
+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 InternetGatewayApi}
+ */
+@Test(groups = "live", singleThreaded = true)
+public class InternetGatewayApiLiveTest extends
BaseComputeServiceContextLiveTest {
[minor] If you don't need access to the portable ComputeService, better extend
the `BaseApiLiveTest`.
> +import org.jclouds.aws.ec2.options.CreateVpcOptions;
+import org.jclouds.aws.ec2.options.InternetGatewayOptions;
+import org.jclouds.compute.internal.BaseComputeServiceContextLiveTest;
+import org.jclouds.ec2.features.TagApi;
+import org.jclouds.logging.Logger;
+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 InternetGatewayApi}
+ */
+@Test(groups = "live", singleThreaded = true)
I think we can remove the single threaded flag. All tests are linked anyway.
> +
+ public void deleteInternetGateway() throws Exception {
+ enqueueRegions(DEFAULT_REGION);
+ enqueueXml(DEFAULT_REGION, "/delete_internet_gateway.xml");
+
+ final boolean deleted =
gatewayApi().deleteInternetGateway(DEFAULT_REGION, "igw-fada7c9c");
+ assertTrue(deleted);
+
+ assertPosted(DEFAULT_REGION, "Action=DescribeRegions");
+ assertPosted(DEFAULT_REGION,
"Action=DeleteInternetGateway&InternetGatewayId=igw-fada7c9c");
+ }
+
+ private InternetGatewayApi gatewayApi() {
+ return api().getInternetGatewayApi().get();
+ }
+}
Add the tests that exercise the fallbacks defined in the API methods by
enqueuing 404 responses.
--
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/1097#pullrequestreview-37216892
