jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] add remaining features to support the abstraction (#345)
Date Wed, 18 Jan 2017 20:25:36 GMT
nacx requested changes on this pull request.

Thanks @andreaturli! This is taking shape! :)

> + * 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.packet.compute.utils;
+
+import java.net.URI;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class URIs {
+
+    public static String toId(URI uri) {
+        checkNotNull(uri, "uri");
+        String path = uri.getPath();
+        return path.substring(path.lastIndexOf('/') + 1);

What about URIs Just like `http://packet.net`? Let's better validate the input to propagate
an appropriate error and add a unit test class for this.

> +            @Override
+            protected IterableWithMarker<Device> fetchPageUsingOptions(ListOptions
options, Optional<Object> arg0) {
+                return api.deviceApi((String) arg0.get()).list(options);
+            }
+        }
+    }
+
+    @Named("device:create")
+    @POST
+    @Produces(MediaType.APPLICATION_JSON)
+    @MapBinder(BindToJsonPayload.class)
+    @ResponseParser(URIParser.class)
+    URI create(@PayloadParam("hostname") String hostname, @PayloadParam("plan") String plan,
+               @PayloadParam("billing_cycle") String billingCycle, @PayloadParam("facility")
String facility,
+               @PayloadParam("features") Map<String, String> features, @PayloadParam("operating_system")
String operatingSystem,
+               @PayloadParam("locked") boolean locked, @PayloadParam("userdata") String userdata,
@PayloadParam("tags") Set<String> tags);

For methods with such an amount of parameters, consider creating an object instead.

> +            @ConstructorProperties({ "devices", "meta" })
+            public Devices(List<Device> items, Meta meta) {
+                super(items, meta);
+            }
+        }
+
+        private static class ToPagedIterable extends BaseToPagedIterable<Device, ListOptions>
{
+
+            @Inject ToPagedIterable(PacketApi api, Function<Href, ListOptions> linkToOptions)
{
+                super(api, linkToOptions);
+            }
+
+           @Override
+           protected List<Object> getArgs(GeneratedHttpRequest request) {
+              return request.getCaller().get().getArgs();
+           }

Do we need to override this?

> +import org.jclouds.rest.annotations.PayloadParam;
+import org.jclouds.rest.annotations.RequestFilters;
+import org.jclouds.rest.annotations.ResponseParser;
+import org.jclouds.rest.annotations.Transform;
+import org.jclouds.rest.binders.BindToJsonPayload;
+
+import com.google.common.base.Function;
+import com.google.common.base.Optional;
+import com.google.inject.TypeLiteral;
+
+@Path("/ssh-keys")
+@Consumes(MediaType.APPLICATION_JSON)
+@RequestFilters({ AddXAuthTokenToRequest.class, AddApiVersionToRequest.class} )
+public interface SshKeyApi {
+
+    @Named("sshKey:list")

Use lowercase naming like in the other apis.

> +           protected List<Object> getArgs(GeneratedHttpRequest request) {
+              return request.getCaller().get().getArgs();
+           }
+
+            @Override
+            protected IterableWithMarker<Device> fetchPageUsingOptions(ListOptions
options, Optional<Object> arg0) {
+                return api.deviceApi((String) arg0.get()).list(options);
+            }
+        }
+    }
+
+    @Named("device:create")
+    @POST
+    @Produces(MediaType.APPLICATION_JSON)
+    @MapBinder(BindToJsonPayload.class)
+    @ResponseParser(URIParser.class)

Use the existing [ParseURIFromListOrLocationHeaderIf20x](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/functions/ParseURIFromListOrLocationHeaderIf20x.java)
instead?

> +      assertEquals(size(devices), 5);
+      assertEquals(server.getRequestCount(), 1);
+
+      assertSent(server, "GET", "/projects/93907f48-adfe-43ed-ad89-0e6e83721a54/devices?page=1&per_page=5");
+   }
+
+   public void testListDevicesWithOptionsReturns404() throws InterruptedException {
+      server.enqueue(response404());
+
+      Iterable<Device> actions = api.deviceApi("93907f48-adfe-43ed-ad89-0e6e83721a54").list(page(1).perPage(5));
+
+      assertTrue(isEmpty(actions));
+
+      assertEquals(server.getRequestCount(), 1);
+      assertSent(server, "GET", "/projects/93907f48-adfe-43ed-ad89-0e6e83721a54/devices?page=1&per_page=5");
+   }

Add the mock tests for the remaining methods in the DeviceApi.

> +import static org.testng.util.Strings.isNullOrEmpty;
+
+@Test(groups = "live", testName = "SshKeyApiLiveTest")
+public class SshKeyApiLiveTest extends BasePacketApiLiveTest {
+
+   public void testListSshKeys() {
+      final AtomicInteger found = new AtomicInteger(0);
+      assertTrue(Iterables.all(api().list().concat(), new Predicate<SshKey>() {
+         @Override
+         public boolean apply(SshKey input) {
+            found.incrementAndGet();
+            return !isNullOrEmpty(input.id());
+         }
+      }), "All ssh keys must have the 'id' field populated");
+      assertTrue(found.get() > 0, "Expected some ssh keys to be returned");
+   }

Add the live tests for the create, get and delete operations.

> +      assertEquals(size(actions), 5);
+      assertEquals(server.getRequestCount(), 1);
+
+      assertSent(server, "GET", "/ssh-keys?page=1&per_page=5");
+   }
+
+   public void testListSshKeysWithOptionsReturns404() throws InterruptedException {
+      server.enqueue(response404());
+
+      Iterable<SshKey> actions = api.sshKeyApi().list(page(1).perPage(5));
+
+      assertTrue(isEmpty(actions));
+
+      assertEquals(server.getRequestCount(), 1);
+      assertSent(server, "GET", "/ssh-keys?page=1&per_page=5");
+   }

Add the tests for the remaining methods in the api.

-- 
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/345#pullrequestreview-17313562
Mime
View raw message