You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by mirza-spc <no...@github.com> on 2016/03/04 09:37:19 UTC
[jclouds-labs] Profitbricks REST - Snapshot API (#244)
@jasminSPC
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/244
-- Commit Summary --
* Profitbricks REST - Snapshot API
-- File Changes --
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/snapshot/UpdateSnapshotRequestBinder.java (97)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/volume/CreateVolumeRequestBinder.java (2)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Snapshot.java (163)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/features/SnapshotApi.java (47)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/SnapshotApiLiveTest.java (165)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/SnapshotApiMockTest.java (162)
M profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/VolumeApiLiveTest.java (1)
A profitbricks-rest/src/test/resources/snapshot/get-depth-5.json (30)
A profitbricks-rest/src/test/resources/snapshot/get.json (30)
A profitbricks-rest/src/test/resources/snapshot/list-depth-5.json (97)
A profitbricks-rest/src/test/resources/snapshot/list.json (277)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/244.patch
https://github.com/jclouds/jclouds-labs/pull/244.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> + api.snapshotApi().updateSnapshot(
> + Snapshot.Request.updatingBuilder()
> + .id("some-id")
> + .name("new-snapshot-name")
> + .description("description...")
> + .build());
> +
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "PATCH", "/rest/snapshots/some-id", "{\"name\": \"new-snapshot-name\", \"description\": \"description...\"}");
> + }
> +
> + @Test
> + public void testDelete() throws InterruptedException {
> + server.enqueue(
> + new MockResponse().setBody("")
> + );
Instead of an empty body, wouldn't it be better to configure a response with the 204 status code?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55018161
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> + assertEquals(list.size(), 3);
> + assertEquals(list.get(0).properties().name(), "test snapshot");
> +
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/snapshots?depth=5");
> + }
> +
> +
> + @Test
> + public void testGetListWith404() throws InterruptedException {
> + server.enqueue(response404());
> + List<Snapshot> list = snapshotApi().getList(new DepthOptions().depth(1));
> + assertTrue(list.isEmpty());
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/snapshots?depth=1");
> + }
Add a test for the 404 fallback for the other `getList` too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55018225
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + assertSnapshotAvailable(testSnapshot);
> + }
> +
> + @AfterClass(alwaysRun = true)
> + public void teardownTest() {
> + if (dataCenter != null)
> + deleteDataCenter(dataCenter.id());
> + }
> +
> + @Test
> + public void testList() {
> + List<Snapshot> snapshots = snapshotApi().getList();
> +
> + assertNotNull(snapshots);
> + assertFalse(snapshots.isEmpty());
Assert that it actually contains the just created snapshot?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55017463
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by mirza-spc <no...@github.com>.
> + */
> +package org.apache.jclouds.profitbricks.rest.binder.snapshot;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import com.google.inject.Inject;
> +import java.util.HashMap;
> +import java.util.Map;
> +import org.apache.jclouds.profitbricks.rest.binder.BaseProfitBricksRequestBinder;
> +import org.apache.jclouds.profitbricks.rest.domain.Snapshot;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.json.Json;
> +
> +public class UpdateSnapshotRequestBinder extends BaseProfitBricksRequestBinder<Snapshot.Request.UpdatePayload> {
> +
> + final Map<String, Object> requestBuilder;
> + final Json jsonBinder;
I've refactored it out into the base class. Good thing for noticing it. I think they are only two binders that build the form encoded data, all others are using json.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55352860
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> + .name("jclouds-volume")
> + .size(3)
> + .licenceType(LicenceType.LINUX)
> + .build()
> + );
> +
> + assertNotNull(testVolume);
> + assertVolumeAvailable(testVolume);
> +
> + testSnapshot = api.volumeApi().createSnapshot(
> + Volume.Request.createSnapshotBuilder()
> + .dataCenterId(testVolume.dataCenterId())
> + .volumeId(testVolume.id())
> + .name("test-snapshot")
> + .description("snapshot desc...")
> + .build());
Move this better to a `createSnapthot` test method.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55017420
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by mirza-spc <no...@github.com>.
>
> @Path("/snapshots")
> @RequestFilters(BasicAuthentication.class)
> public interface SnapshotApi extends Closeable {
>
> + @Named("snapshot:list")
> + @GET
> + @SelectJson("items")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<Snapshot> getList();
this should mean that all of the methods of all of the apis in profitbricks rest should be updated to the convention format?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55205994
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> + */
> +package org.apache.jclouds.profitbricks.rest.binder.snapshot;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import com.google.inject.Inject;
> +import java.util.HashMap;
> +import java.util.Map;
> +import org.apache.jclouds.profitbricks.rest.binder.BaseProfitBricksRequestBinder;
> +import org.apache.jclouds.profitbricks.rest.domain.Snapshot;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.json.Json;
> +
> +public class UpdateSnapshotRequestBinder extends BaseProfitBricksRequestBinder<Snapshot.Request.UpdatePayload> {
> +
> + final Map<String, Object> requestBuilder;
> + final Json jsonBinder;
What about this comment? Something that makes sense for a separate PR? Also I've noticed some binders declare the variables in the default scope. If there is no good reason for that, they should be made private. We could address both in a new PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55286102
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.apache.jclouds.profitbricks.rest.domain.Volume;
> +import org.apache.jclouds.profitbricks.rest.ids.VolumeRef;
> +import org.apache.jclouds.profitbricks.rest.internal.BaseProfitBricksLiveTest;
> +import static org.testng.Assert.assertEquals;
> +import org.testng.annotations.Test;
> +import static org.testng.Assert.assertFalse;
> +import static org.testng.Assert.assertNotNull;
> +import org.testng.annotations.AfterClass;
> +import org.testng.annotations.BeforeClass;
> +
> +@Test(groups = "live", testName = "SnapshotApiLiveTest")
> +public class SnapshotApiLiveTest extends BaseProfitBricksLiveTest {
> +
> + DataCenter dataCenter;
> + Volume testVolume;
> + Snapshot testSnapshot;
Make these variables private
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55017344
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
>
> @Path("/snapshots")
> @RequestFilters(BasicAuthentication.class)
> public interface SnapshotApi extends Closeable {
>
> + @Named("snapshot:list")
> + @GET
> + @SelectJson("items")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<Snapshot> getList();
I've just noticed this pattern. In other jclouds APIs, the common name for list methods is just `list` (instead of getList). Mind doing the change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55018368
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [ca57d90d](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/ca57d90d). Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244#issuecomment-194537268
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by mirza-spc <no...@github.com>.
>
> @Path("/snapshots")
> @RequestFilters(BasicAuthentication.class)
> public interface SnapshotApi extends Closeable {
>
> + @Named("snapshot:list")
> + @GET
> + @SelectJson("items")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<Snapshot> getList();
ok but you still have getSnapshot, should it be just get? or createSnapshot => create? that would seem along the lines of such a convention. or is it just that the list method is a special case?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55169058
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Test
> + public void testList() {
> + List<Snapshot> snapshots = snapshotApi().list();
> +
> + assertNotNull(snapshots);
> + assertFalse(snapshots.isEmpty());
> +
> + Snapshot newSnapshot = null;
> +
> + for (Snapshot snapshot : snapshots) {
> + if (snapshot.id().equals(testSnapshot.id())) {
> + newSnapshot = snapshot;
> + break;
> + }
> + }
Better use:
```java
assertTrue(Iterables.any(snapshots, new Predicate<Snapshot>() {
@Override public boolean apply(Snapshot input) {
return input.id().equals(testSnapshot.id());
}
}));
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55285927
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
Closed #244.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244#event-584025482
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by mirza-spc <no...@github.com>.
> + .name("jclouds-volume")
> + .size(3)
> + .licenceType(LicenceType.LINUX)
> + .build()
> + );
> +
> + assertNotNull(testVolume);
> + assertVolumeAvailable(testVolume);
> +
> + testSnapshot = api.volumeApi().createSnapshot(
> + Volume.Request.createSnapshotBuilder()
> + .dataCenterId(testVolume.dataCenterId())
> + .volumeId(testVolume.id())
> + .name("test-snapshot")
> + .description("snapshot desc...")
> + .build());
That is already tested in volume api, there is not create snapshot in snapshot api as per docs.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55022061
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @mirza-spc! This looks pretty good; all comments are minors.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244#issuecomment-192237645
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + assertEquals(this.server.getRequestCount(), 1);
> + assertSent(this.server, "GET", "/snapshots/some-id?depth=5");
> + }
> +
> +
> + public void testGetSnapshotWith404() throws InterruptedException {
> + server.enqueue(response404());
> +
> + Snapshot snapshot = snapshotApi().getSnapshot("some-id");
> +
> + assertEquals(snapshot, null);
> +
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/snapshots/some-id");
> + }
Add a test for the 404 fallback for the other `getSnapshot` too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55018274
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> + List<Snapshot> snapshots = snapshotApi().getList();
> +
> + assertNotNull(snapshots);
> + assertFalse(snapshots.isEmpty());
> + }
> +
> + @Test
> + public void testGetSnapshot() {
> + Snapshot snapshot = snapshotApi().getSnapshot(testSnapshot.id());
> +
> + assertNotNull(snapshot);
> + assertEquals(snapshot.id(), testSnapshot.id());
> + assertEquals(snapshot.properties().name(), "test-snapshot");
> + }
> +
> + @Test(dependsOnMethods = "testGetSnapshot")
Consider changing this to depend on the create snapshot test once it exists.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55017544
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> + */
> +package org.apache.jclouds.profitbricks.rest.binder.snapshot;
> +
> +import static com.google.common.base.Preconditions.checkNotNull;
> +import com.google.inject.Inject;
> +import java.util.HashMap;
> +import java.util.Map;
> +import org.apache.jclouds.profitbricks.rest.binder.BaseProfitBricksRequestBinder;
> +import org.apache.jclouds.profitbricks.rest.domain.Snapshot;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.json.Json;
> +
> +public class UpdateSnapshotRequestBinder extends BaseProfitBricksRequestBinder<Snapshot.Request.UpdatePayload> {
> +
> + final Map<String, Object> requestBuilder;
> + final Json jsonBinder;
I've noticed these properties are used in all binders. Would it make sense to move them tot he parent class? Also, is there any reason why they're not private?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55016821
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
>
> @Path("/snapshots")
> @RequestFilters(BasicAuthentication.class)
> public interface SnapshotApi extends Closeable {
>
> + @Named("snapshot:list")
> + @GET
> + @SelectJson("items")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<Snapshot> getList();
Yes, but let's leave that for another PR and just amend the ones in the snapshot api in this one.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55209868
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
>
> @Path("/snapshots")
> @RequestFilters(BasicAuthentication.class)
> public interface SnapshotApi extends Closeable {
>
> + @Named("snapshot:list")
> + @GET
> + @SelectJson("items")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<Snapshot> getList();
Yes, that's the convention we try to use now, so it applies to all mehtods.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55172195
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> + .name("jclouds-volume")
> + .size(3)
> + .licenceType(LicenceType.LINUX)
> + .build()
> + );
> +
> + assertNotNull(testVolume);
> + assertVolumeAvailable(testVolume);
> +
> + testSnapshot = api.volumeApi().createSnapshot(
> + Volume.Request.createSnapshotBuilder()
> + .dataCenterId(testVolume.dataCenterId())
> + .volumeId(testVolume.id())
> + .name("test-snapshot")
> + .description("snapshot desc...")
> + .build());
Oh, you're right. Dismiss this comment then! :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55040291
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Test(dependsOnMethods = "testUpdateSnapshot")
> + public void testDeleteSnapshot() {
> + api.volumeApi().deleteVolume(testVolume.dataCenterId(), testVolume.id());
> + assertVolumeRemoved(testVolume);
> + snapshotApi().deleteSnapshot(testSnapshot.id());
> + assertSnapshotRemoved(testSnapshot);
> + }
> +
> + private SnapshotApi snapshotApi() {
> + return api.snapshotApi();
> + }
> +
> + private void assertVolumeAvailable(Volume volume) {
> + assertRandom(new Predicate<VolumeRef>() {
Just seen this... Would it be more convenient for this method to be named `assertPredicate`? The current name seems to mean that it will perform arbitrary assertions...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244/files#r55017838
Re: [jclouds-labs] Profitbricks REST - Snapshot API (#244)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @mirza-spc! Mind squashing the commits to prepare the merge?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/244#issuecomment-193988297