nacx requested changes on this pull request.
Thanks @andreaturli! This looks like a great start.
Apart from the comments, we should add:
* Pagination
* The adapter for PublicKey.
> + @Override
+ public Builder toBuilder() {
+ return new Builder().fromApiMetadata(this);
+ }
+
+ public PacketApiMetadata() {
+ this(new Builder());
+ }
+
+ protected PacketApiMetadata(Builder builder) {
+ super(builder);
+ }
+
+ public static Properties defaultProperties() {
+ Properties properties = BaseHttpApiMetadata.defaultProperties();
+ properties.put(TEMPLATE,
"osFamily=UBUNTU,os64Bit=true,osVersionMatches=14.*");
Ccan we default to a more modern version?
> + super(builder);
+ }
+
+ public static Properties defaultProperties() {
+ Properties properties = BaseHttpApiMetadata.defaultProperties();
+ properties.put(TEMPLATE,
"osFamily=UBUNTU,os64Bit=true,osVersionMatches=14.*");
+ properties.put(TIMEOUT_NODE_RUNNING, 300000); // 5 mins
+ return properties;
+ }
+
+ public static class Builder extends BaseHttpApiMetadata.Builder<PacketApi,
Builder> {
+
+ protected Builder() {
+ id("packet")
+ .name("Packet API")
+ .identityName("We can use project Id")
Let's say what needs to be put as the identity. Users will read this to know
what to put, so let's be concrete and pick one option.
> + properties.put(TEMPLATE,
> "osFamily=UBUNTU,os64Bit=true,osVersionMatches=14.*");
+ properties.put(TIMEOUT_NODE_RUNNING, 300000); // 5 mins
+ return properties;
+ }
+
+ public static class Builder extends BaseHttpApiMetadata.Builder<PacketApi,
Builder> {
+
+ protected Builder() {
+ id("packet")
+ .name("Packet API")
+ .identityName("We can use project Id")
+ .credentialName("Must be Token")
+
.documentation(URI.create("https://www.packet.net/help/api/#";))
+ .defaultEndpoint("https://api.packet.net";)
+ .defaultProperties(PacketApiMetadata.defaultProperties())
+ .view(typeToken(ComputeServiceContext.class))
Comment this out until the ComputeService interface is implemented.
> +
+ public static Properties defaultProperties() {
+ final Properties properties = PacketApiMetadata.defaultProperties();
+ return properties;
+ }
+
+ public static class Builder extends BaseProviderMetadata.Builder {
+
+ protected Builder() {
+ id("packet")
+ .name("Packet Compute Services")
+ .apiMetadata(new PacketApiMetadata())
+ .homepage(URI.create("https://www.packet.net/";))
+ .console(URI.create("https://app.packet.net/portal";))
+ .endpoint("https://api.packet.net";)
+
.defaultProperties(PacketProviderMetadata.defaultProperties());
Do we know which regions are available so we can put the iso codes here?
> +import org.jclouds.location.suppliers.implicit.FirstRegion;
+import org.jclouds.packet.PacketApi;
+import org.jclouds.packet.handlers.PacketErrorHandler;
+import org.jclouds.rest.ConfiguresHttpApi;
+import org.jclouds.rest.config.HttpApiModule;
+
+import com.google.inject.Scopes;
+
+@ConfiguresHttpApi
+public class PacketHttpApiModule extends HttpApiModule<PacketApi> {
+
+ @Override
+ protected void configure() {
+ install(new PacketComputeParserModule());
+ super.configure();
+ }
We could remove this if we add the parser module to the list of modules in the
api metadata. Better to have the complete list there?
> + @Override
+ protected void configure() {
+ install(new PacketComputeParserModule());
+ super.configure();
+ }
+
+ @Override
+ protected void bindErrorHandlers() {
+
bind(HttpErrorHandler.class).annotatedWith(Redirection.class).to(PacketErrorHandler.class);
+
bind(HttpErrorHandler.class).annotatedWith(ClientError.class).to(PacketErrorHandler.class);
+
bind(HttpErrorHandler.class).annotatedWith(ServerError.class).to(PacketErrorHandler.class);
+ }
+
+ @Override
+ protected void bindRetryHandlers() {
+
bind(HttpRetryHandler.class).annotatedWith(ClientError.class).to(BackoffLimitedRetryHandler.class);
Hmmmm, this means we are going to retry 4xx errors. Is that what we want? Those
usually are bad requests that will always fail. Retrying should be done by
default just for server errors.
> + @Override
+ protected void bindErrorHandlers() {
+
bind(HttpErrorHandler.class).annotatedWith(Redirection.class).to(PacketErrorHandler.class);
+
bind(HttpErrorHandler.class).annotatedWith(ClientError.class).to(PacketErrorHandler.class);
+
bind(HttpErrorHandler.class).annotatedWith(ServerError.class).to(PacketErrorHandler.class);
+ }
+
+ @Override
+ protected void bindRetryHandlers() {
+
bind(HttpRetryHandler.class).annotatedWith(ClientError.class).to(BackoffLimitedRetryHandler.class);
+ }
+
+ @Override
+ protected void installLocations() {
+ install(new LocationModule());
+
bind(ImplicitLocationSupplier.class).to(FirstRegion.class).in(Scopes.SINGLETON);
Move the binding to the configure method and remove this override?
> +import org.jclouds.rest.annotations.RequestFilters;
+import org.jclouds.rest.annotations.SelectJson;
+
+@Path("/projects")
+@Consumes(MediaType.APPLICATION_JSON)
+@RequestFilters(AddXAuthTokenToRequest.class)
+public interface ProjectApi {
+
+ /**
+ * List all projects
+ */
+ @Named("project:list")
+ @GET
+ @SelectJson("projects")
+ @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
+ List<Project> list();
Since Packet supports pagination, we should add it as soon as possible, to set
the foundation for all the APIs. I'd configure this list to support pagination
right in this PR.
Looking at the docs, it works very very similar to the DigitalOcean2 one, so
you could take it as an example (for example the
[ActionApi](https://github.com/jclouds/jclouds/blob/master/providers/digitalocean2/src/main/java/org/jclouds/digitalocean2/features/ActionApi.java)
class, which is a small one) and get it working soon.
> +import org.jclouds.http.HttpException;
+import org.jclouds.http.HttpRequest;
+import org.jclouds.http.HttpRequestFilter;
+import org.jclouds.location.Provider;
+
+import com.google.common.base.Supplier;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+@Singleton
+public class AddXAuthTokenToRequest implements HttpRequestFilter {
+
+ private final Supplier<Credentials> creds;
+
+ @Inject
+ public AddXAuthTokenToRequest(@Provider Supplier<Credentials> creds) {
Remove the public modifier.
> +import org.jclouds.http.HttpRequest;
+import org.jclouds.http.HttpRequestFilter;
+import org.jclouds.location.Provider;
+
+import com.google.common.base.Supplier;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+@Singleton
+public class AddXAuthTokenToRequest implements HttpRequestFilter {
+
+ private final Supplier<Credentials> creds;
+
+ @Inject
+ public AddXAuthTokenToRequest(@Provider Supplier<Credentials> creds) {
+ this.creds = checkNotNull(creds, "creds");
The check is redundant. Injection will fail if it is not present.
> + */
+@Singleton
+public class PacketErrorHandler implements HttpErrorHandler {
+
+ public void handleError(HttpCommand command, HttpResponse response) {
+ // it is important to always read fully and close streams
+ byte[] data = closeClientButKeepContentStream(response);
+ String message = data != null ? new String(data) : null;
+
+ Exception exception = message != null ? new
HttpResponseException(command, response, message)
+ : new HttpResponseException(command, response);
+ message = message != null ? message : String.format("%s -> %s",
command.getCurrentRequest().getRequestLine(),
+ response.getStatusLine());
+ switch (response.getStatusCode()) {
+ case 400:
+ break;
400 errors are Bad Request errors which are usually populated as
IllegalArgumentException errors.
--
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/340#pullrequestreview-15923697
