nacx commented on this pull request.
First review done (not checked the tests).
I'll go through the tests review once the current comments are addressed.
> + * 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.aliyun.ecs.domain;
+
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Objects;
+
+import java.beans.ConstructorProperties;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class KeyPairRequest extends Request {
Use auto-value
> + public static Permission create(String sourceCidrIp, String destCidrIp,
> String description, String nicType,
+ String destGroupName, String portRange,
String destGroupId, String direction, String priority,
+ String ipProtocol, String
sourceGroupOwnerAccount, String policy, Date createTime, String sourceGroupId,
+ String destGroupOwnerAccount, String
sourceGroupName) {
+ return new AutoValue_Permission(sourceCidrIp, destCidrIp, description,
nicType, destGroupName, portRange,
+ destGroupId, direction, priority, ipProtocol,
sourceGroupOwnerAccount, policy, createTime, sourceGroupId,
+ destGroupOwnerAccount, sourceGroupName);
+ }
+
+ public abstract String sourceCidrIp();
+
+ public abstract String destCidrIp();
+
+ public abstract String description();
+
+ public abstract String nicType();
Looks like a candidate for an enum?
> +
+ public abstract String sourceCidrIp();
+
+ public abstract String destCidrIp();
+
+ public abstract String description();
+
+ public abstract String nicType();
+
+ public abstract String destGroupName();
+
+ public abstract String portRange();
+
+ public abstract String destGroupId();
+
+ public abstract String direction();
Same here, does this have a finite set of possible values?
> +
+ public abstract String description();
+
+ public abstract String nicType();
+
+ public abstract String destGroupName();
+
+ public abstract String portRange();
+
+ public abstract String destGroupId();
+
+ public abstract String direction();
+
+ public abstract String priority();
+
+ public abstract String ipProtocol();
Same here about enums
> + * 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.aliyun.ecs.domain;
+
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Objects;
+
+import java.beans.ConstructorProperties;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class Request {
Use auto-value
> +import com.google.auto.value.AutoValue;
+import org.jclouds.json.SerializedNames;
+
+@AutoValue
+public abstract class SecurityGroup {
+
+ SecurityGroup() {
+ }
+
+ @SerializedNames({ "SecurityGroupId", "Description", "SecurityGroupName",
"VpcId" })
+ public static SecurityGroup create(String securityGroupId, String
description, String securityGroupName,
+ String vpcId) {
+ return new AutoValue_SecurityGroup(securityGroupId, description,
securityGroupName, vpcId);
+ }
+
+ public abstract String securityGroupId();
In general, for all classes, remove the "securityGroup" prefix. Use just "id",
"name". This class is already a Security group, so this kind of prefixes don't
make sense.
> + * 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.aliyun.ecs.domain;
+
+import com.google.common.base.MoreObjects;
+import com.google.common.base.Objects;
+
+import java.beans.ConstructorProperties;
+
+public class SecurityGroupRequest extends Request {
Why not auto-value?
> +import org.jclouds.http.options.BaseHttpRequestOptions;
+
+public class TagOptions extends BaseHttpRequestOptions {
+
+ public static final String TAG_1_KEY_PARAM = "Tag.1.Key";
+ public static final String TAG_2_KEY_PARAM = "Tag.2.Key";
+ public static final String TAG_3_KEY_PARAM = "Tag.3.Key";
+ public static final String TAG_4_KEY_PARAM = "Tag.4.Key";
+ public static final String TAG_5_KEY_PARAM = "Tag.5.Key";
+ public static final String TAG_1_VALUE_PARAM = "Tag.1.Value";
+ public static final String TAG_2_VALUE_PARAM = "Tag.2.Value";
+ public static final String TAG_3_VALUE_PARAM = "Tag.3.Value";
+ public static final String TAG_4_VALUE_PARAM = "Tag.4.Value";
+ public static final String TAG_5_VALUE_PARAM = "Tag.5.Value";
+
+ public TagOptions tag1Key(String tag1Key) {
Wouldn't it be better to just provide one method like this?
```java
public TagOptions tag(int pos, String name, String value)
```
> + @Override
+ public IterableWithMarker<SecurityGroup> apply(Object input) {
+ String regionId = arg0.get().toString();
+ ListSecurityGroupsOptions listSecurityGroupsOptions =
ListSecurityGroupsOptions.Builder.paginationOptions(PaginationOptions.class.cast(input));
+ return api.securityGroupApi().list(regionId,
listSecurityGroupsOptions);
+ }
+ };
+ }
+ }
+ }
+
+ @Named("securityGroup:get")
+ @GET
+ @QueryParams(keys = "Action", values = "DescribeSecurityGroupAttribute")
+ @SelectJson("Permission")
+ List<Permission> get(@QueryParam("RegionId") String region,
@QueryParam("SecurityGroupId") String securityGroupId);
Returns a list?
> + }
+ };
+ }
+ }
+ }
+
+ @Named("securityGroup:get")
+ @GET
+ @QueryParams(keys = "Action", values = "DescribeSecurityGroupAttribute")
+ @SelectJson("Permission")
+ List<Permission> get(@QueryParam("RegionId") String region,
@QueryParam("SecurityGroupId") String securityGroupId);
+
+ @Named("securityGroup:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
Remove this fallback. Don't use 4xx fallbacks in POST/PUT
> + @Named("securityGroup:get")
+ @GET
+ @QueryParams(keys = "Action", values = "DescribeSecurityGroupAttribute")
+ @SelectJson("Permission")
+ List<Permission> get(@QueryParam("RegionId") String region,
@QueryParam("SecurityGroupId") String securityGroupId);
+
+ @Named("securityGroup:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+ SecurityGroupRequest create(@QueryParam("RegionId") String region);
+
+ @Named("securityGroup:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
Remove the fallback.
> + @GET
+ @QueryParams(keys = "Action", values = "DescribeSecurityGroupAttribute")
+ @SelectJson("Permission")
+ List<Permission> get(@QueryParam("RegionId") String region,
@QueryParam("SecurityGroupId") String securityGroupId);
+
+ @Named("securityGroup:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+ SecurityGroupRequest create(@QueryParam("RegionId") String region);
+
+ @Named("securityGroup:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+ SecurityGroupRequest create(@QueryParam("RegionId") String region,
CreateSecurityGroupOptions options);
Options will be bound to the request as query params. Is that correct for this
POST operation?
> + @Named("securityGroup:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+ SecurityGroupRequest create(@QueryParam("RegionId") String region);
+
+ @Named("securityGroup:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+ SecurityGroupRequest create(@QueryParam("RegionId") String region,
CreateSecurityGroupOptions options);
+
+ @Named("securityGroup:addInbound")
+ @POST
+ @QueryParams(keys = "Action", values = "AuthorizeSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
Remove fallback.
> + @QueryParams(keys = "Action", values = "CreateSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+ SecurityGroupRequest create(@QueryParam("RegionId") String region,
CreateSecurityGroupOptions options);
+
+ @Named("securityGroup:addInbound")
+ @POST
+ @QueryParams(keys = "Action", values = "AuthorizeSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+ Request addInboundRule(@QueryParam("RegionId") String region,
@QueryParam("SecurityGroupId") String securityGroupId,
+ @QueryParam("IpProtocol") IpProtocol ipProtocol,
@QueryParam("PortRange") String portRange,
+ @QueryParam("SourceCidrIp") String sourceCidrIp);
+
+ @Named("securityGroup:delete")
+ @POST
+ @QueryParams(keys = "Action", values = "DeleteSecurityGroup")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
This does not return a list. Fix fallback.
> + @Named("sshKeyPair:create")
+ @POST
+ @QueryParams(keys = "Action", values = "CreateKeyPair")
+ KeyPairRequest create(@QueryParam("RegionId") String region,
@QueryParam("KeyPairName") String keyPairName);
+
+ @Named("sshKeyPair:import")
+ @POST
+ @QueryParams(keys = "Action", values = "ImportKeyPair")
+ KeyPair importKeyPair(@QueryParam("RegionId") String region,
+ @QueryParam("PublicKeyBody") String publicKeyBody,
+ @QueryParam("KeyPairName") String keyPairName);
+
+ @Named("sshKeyPair:delete")
+ @POST
+ @QueryParams(keys = "Action", values = "DeleteKeyPairs")
+ Request delete(@QueryParam("RegionId") String region);
Does this delete all keypairs?
> + @POST
+ @QueryParams(keys = "Action", values = "ImportKeyPair")
+ KeyPair importKeyPair(@QueryParam("RegionId") String region,
+ @QueryParam("PublicKeyBody") String publicKeyBody,
+ @QueryParam("KeyPairName") String keyPairName);
+
+ @Named("sshKeyPair:delete")
+ @POST
+ @QueryParams(keys = "Action", values = "DeleteKeyPairs")
+ Request delete(@QueryParam("RegionId") String region);
+
+ @Named("sshKeyPair:delete")
+ @POST
+ @QueryParams(keys = "Action", values = "DeleteKeyPairs")
+ Request delete(@QueryParam("RegionId") String region, DeleteKeyPairOptions
deleteOptions);
+}
Is there a method to get a single keypair?
> + static class ToPagedIterable extends
> Arg0ToPagedIterable<SecurityGroup, ParseSecurityGroups.ToPagedIterable> {
+
+ private final ECSComputeServiceApi api;
+
+ @Inject
+ ToPagedIterable(ECSComputeServiceApi api) {
+ this.api = api;
+ }
+
+ @Override
+ protected Function<Object, IterableWithMarker<SecurityGroup>>
markerToNextForArg0(final Optional<Object> arg0) {
+ return new Function<Object, IterableWithMarker<SecurityGroup>>() {
+ @Override
+ public IterableWithMarker<SecurityGroup> apply(Object input) {
+ String regionId = arg0.get().toString();
+ ListSecurityGroupsOptions listSecurityGroupsOptions =
ListSecurityGroupsOptions.Builder.paginationOptions(PaginationOptions.class.cast(input));
You are creating a brand new options object and forgetting all other options in
the original object. You need to keep it.
Get a builder out from the input object (so it keeps all info), and just
override the pagination options, instead of creating a brand new options
object. Apply this to all API classes.
> + return new Function<Object, IterableWithMarker<Tag>>() {
+ @Override
+ public IterableWithMarker<Tag> apply(Object input) {
+ String regionId = arg0.get().toString();
+ ListTagsOptions listTagsOptions =
ListTagsOptions.Builder.paginationOptions(PaginationOptions.class.cast(input));
+ return api.tagApi().list(regionId, listTagsOptions);
+ }
+ };
+ }
+ }
+ }
+
+ @Named("tag:add")
+ @POST
+ @QueryParams(keys = "Action", values = "AddTags")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
Remove
> + }
+ }
+
+ @Named("tag:add")
+ @POST
+ @QueryParams(keys = "Action", values = "AddTags")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+ Request add(@QueryParam("RegionId") String region,
@QueryParam("ResourceId") String resourceId,
+ @QueryParam("ResourceType") String resourceType,
+ TagOptions tagOptions);
+
+ @Named("tag:remove")
+ @POST
+ @QueryParams(keys = "Action", values = "RemoveTags")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+ Request remove(@QueryParam("RegionId") String region,
Does this remove all the tags? Can a single tag be removed?
--
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/441#pullrequestreview-137945744
