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/29 09:59:12 UTC
[jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
@jasminSPC
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/251
-- Commit Summary --
* Profitbricks REST - LAN API
-- File Changes --
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/ProfitBricksApi.java (4)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/lan/CreateLanRequestBinder.java (68)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/lan/UpdateLanRequestBinder.java (55)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/nic/CreateNicRequestBinder.java (5)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/nic/UpdateNicRequestBinder.java (3)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Lan.java (77)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/features/LanApi.java (123)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/binder/lan/CreateLanRequestBinderTest.java (63)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/binder/lan/UpdateLanRequestBinderTest.java (61)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/binder/nic/CreateNicRequestBinderTest.java (66)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/binder/nic/UpdateNicRequestBinderTest.java (62)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/LanApiLiveTest.java (137)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/LanApiMockTest.java (148)
M profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/NicApiMockTest.java (4)
A profitbricks-rest/src/test/resources/lan/get.json (57)
A profitbricks-rest/src/test/resources/lan/list.json (328)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/251.patch
https://github.com/jclouds/jclouds-labs/pull/251.diff
---
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/251
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Test(dependsOnMethods = "testCreateLan")
> + public void testList() {
> + List<Lan> lans = lanApi().list(dataCenter.id());
> +
> + assertNotNull(lans);
> + assertFalse(lans.isEmpty());
> + assertTrue(Iterables.any(lans, new Predicate<Lan>() {
> + @Override public boolean apply(Lan input) {
> + return input.id().equals(testLan.id());
> + }
> + }));
> + }
> +
> + @Test(dependsOnMethods = "testCreateLan", alwaysRun = true)
We really don't want to execute this if the lan was not created, so let's remove the `alwaysRun = true` from this test.
---
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/251/files/a1ca155092ab4da85662a17c2476185327dcc644#r58029671
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by jasminSPC <no...@github.com>.
Got it, thanks
Jasmin Gacić
On Apr 6, 2016 22:09, "Ignasi Barrera" <no...@github.com> wrote:
> BTW, that's why I always put a link to the commit in the Apache git repo.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub
> <https://github.com/jclouds/jclouds-labs/pull/251#issuecomment-206538019>
>
---
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/251#issuecomment-206538692
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
Closed #251.
---
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/251#event-617323595
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.apache.jclouds.profitbricks.rest.domain.State;
> +import org.apache.jclouds.profitbricks.rest.domain.Lan;
> +import org.apache.jclouds.profitbricks.rest.internal.BaseProfitBricksLiveTest;
> +import org.testng.annotations.AfterClass;
> +import org.testng.annotations.BeforeClass;
> +import org.testng.annotations.Test;
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertFalse;
> +import static org.testng.Assert.assertNotNull;
> +
> +@Test(groups = "live", testName = "LanApiLiveTest")
> +public class LanApiLiveTest extends BaseProfitBricksLiveTest {
> +
> + DataCenter dataCenter;
> + Lan testLan;
> + Snapshot testSnapshot;
Make these variables private
---
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/251/files/dade9518e8e726d8c8040077a65ae77d8866db6d#r57851469
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
It is OK. I don't "git merge" contributions for two reasons:
* To avoid unnecessary merge commits.
* To properly preserve authorship, so the commit properly reflects the author and the committer (when using git merge the committer is set tot he author even when he's not the one that checked in the code).
For this reason I usually merge contributions with a cherry-pick (which changes the commit hash), and that's why Github does not detect it as merged.
Also, commits are pushed to the Apache Git repos and mirrored to GitHub, so it might be a while until commits appear here, but don't worry, everything is OK. If in doubt, please, double-check and compare the commit that is in master with the one in your branch!
---
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/251#issuecomment-206537268
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
BTW, that's why I always put a link to the commit in the Apache git repo.
---
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/251#issuecomment-206538019
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
> + Lan lan = lanApi().get(dataCenter.id(), testLan.id());
> +
> + assertNotNull(lan);
> + assertEquals(lan.id(), testLan.id());
> + }
> +
> + @Test(dependsOnMethods = "testCreateLan")
> + public void testList() {
> + List<Lan> lans = lanApi().list(dataCenter.id());
> +
> + assertNotNull(lans);
> + assertFalse(lans.isEmpty());
> + assertEquals(lans.size(), 1);
> + }
> +
> + @Test(dependsOnMethods = "testGetLan")
Can this just depend on the create test? This will reduce the chances this test is skipped.
---
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/251/files/dade9518e8e726d8c8040077a65ae77d8866db6d#r57851744
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + api.lanApi().update(
> + Lan.Request.updatingBuilder()
> + .dataCenterId(testLan.dataCenterId())
> + .id(testLan.id())
> + .isPublic(false)
> + .build());
> +
> + assertLanAvailable(testLan);
> +
> + Lan lan = lanApi().get(dataCenter.id(), testLan.id());
> +
> + assertEquals(lan.properties().isPublic(), false);
> + }
> +
> + @Test(dependsOnMethods = "testUpdateLan")
Is an `alwaysRun = True` needed here, or the tear down that removes the datacenter is enough?
---
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/251/files/dade9518e8e726d8c8040077a65ae77d8866db6d#r57851950
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Test(dependsOnMethods = "testCreateLan")
> + public void testGetLan() {
> + Lan lan = lanApi().get(dataCenter.id(), testLan.id());
> +
> + assertNotNull(lan);
> + assertEquals(lan.id(), testLan.id());
> + }
> +
> + @Test(dependsOnMethods = "testCreateLan")
> + public void testList() {
> + List<Lan> lans = lanApi().list(dataCenter.id());
> +
> + assertNotNull(lans);
> + assertFalse(lans.isEmpty());
> + assertEquals(lans.size(), 1);
A more robust assertion would be to check that the list *contains* the created Lan. Just to avoid race conditions if at some point we create something else in this datacenter.
---
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/251/files/dade9518e8e726d8c8040077a65ae77d8866db6d#r57851626
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import com.squareup.okhttp.mockwebserver.MockResponse;
> +import java.util.List;
> +import org.apache.jclouds.profitbricks.rest.domain.Lan;
> +import org.apache.jclouds.profitbricks.rest.domain.options.DepthOptions;
> +import org.apache.jclouds.profitbricks.rest.internal.BaseProfitBricksApiMockTest;
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +import static org.testng.Assert.assertTrue;
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "unit", testName = "LanApiMockTest", singleThreaded = true)
> +public class LanApiMockTest extends BaseProfitBricksApiMockTest {
> +
> + @Test
> + public void testGetList() throws InterruptedException {
Rename tests to `testList`
---
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/251/files/dade9518e8e726d8c8040077a65ae77d8866db6d#r57852149
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
Just one last comment. Mind fixing it and directly squash the commits to prepare the merge? Thx!
---
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/251#issuecomment-203863240
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by mirza-spc <no...@github.com>.
> +
> + api.lanApi().update(
> + Lan.Request.updatingBuilder()
> + .dataCenterId(testLan.dataCenterId())
> + .id(testLan.id())
> + .isPublic(false)
> + .build());
> +
> + assertLanAvailable(testLan);
> +
> + Lan lan = lanApi().get(dataCenter.id(), testLan.id());
> +
> + assertEquals(lan.properties().isPublic(), false);
> + }
> +
> + @Test(dependsOnMethods = "testUpdateLan")
I would say so, because all the things in the datacenter get removed with it.
---
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/251/files/dade9518e8e726d8c8040077a65ae77d8866db6d#r57856149
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by jasminSPC <no...@github.com>.
@nacx I see this This pull request is closed, but the StackPointCloud:pb-lan-api **branch has unmerged commits.** is this how it is supposed to be?
---
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/251#issuecomment-206521707
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
> + assertNotNull(list);
> + assertEquals(list.size(), 4);
> + assertEquals(list.get(0).properties().name(), "Ex lan 1");
> +
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/datacenters/datacenter-id/lans");
> + }
> +
> + @Test
> + public void testGetListWith404() throws InterruptedException {
> + server.enqueue(response404());
> + List<Lan> list = lanApi().list("datacenter-id", new DepthOptions().depth(1));
> + assertTrue(list.isEmpty());
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/datacenters/datacenter-id/lans?depth=1");
> + }
You need to test each method independently. Missing tests:
* List with options when response is 200.
* List without options when response is 404.
* Get with options when response is 200.
* Get with options when response is 404.
---
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/251/files/dade9518e8e726d8c8040077a65ae77d8866db6d#r57852263
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [ff028de1](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/ff028de1). Thanks @mirza-spc!
---
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/251#issuecomment-206521418
Re: [jclouds/jclouds-labs] Profitbricks REST - LAN API (#251)
Posted by Ignasi Barrera <no...@github.com>.
> + return false;
> +
> + return lan.metadata().state() == State.AVAILABLE;
> + }
> + }, complexId(lan.dataCenterId(), lan.id()));
> + }
> +
> + private void assertLanRemoved(Lan lan) {
> + assertPredicate(new Predicate<String>() {
> + @Override
> + public boolean apply(String args) {
> + String[] params = args.split(",");
> + return lanApi().get(params[0], params[1]) == null;
> + }
> + }, complexId(lan.dataCenterId(), lan.id()));
> + }
Same thing here about this ugly complex id thing. Get rid of this pattern, please.
---
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/251/files/dade9518e8e726d8c8040077a65ae77d8866db6d#r57851857