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/22 14:49:06 UTC
[jclouds-labs] Profitbricks REST - NIC API (#249)
@jasminSPC
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/249
-- Commit Summary --
* Profitbricks REST - NIC API
-- File Changes --
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/ProfitBricksApi.java (4)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/BaseProfitBricksRequestBinder.java (4)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/image/UpdateImageRequestBinder.java (28)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/nic/CreateNicRequestBinder.java (85)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/nic/UpdateNicRequestBinder.java (73)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/server/AttachCdromRequestBinder.java (4)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/server/AttachVolumeRequestBinder.java (4)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/server/CreateServerRequestBinder.java (6)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/server/UpdateServerRequestBinder.java (16)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/snapshot/UpdateSnapshotRequestBinder.java (30)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/volume/CreateVolumeRequestBinder.java (4)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/volume/UpdateVolumeRequestBinder.java (8)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Nic.java (115)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/features/NicApi.java (126)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/NicApiLiveTest.java (153)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/NicApiMockTest.java (151)
A profitbricks-rest/src/test/resources/nic/get.json (31)
A profitbricks-rest/src/test/resources/nic/list.json (38)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/249.patch
https://github.com/jclouds/jclouds-labs/pull/249.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/249
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
> + assertNicRemoved(testNic);
> + }
> +
> + private void assertNicAvailable(Nic nic) {
> + assertPredicate(new Predicate<String>() {
> + @Override
> + public boolean apply(String args) {
> + String[] params = args.split(",");
> + Nic nic = nicApi().getNic(params[0], params[1], params[2]);
> +
> + if (nic == null || nic.metadata() == null)
> + return false;
> +
> + return nic.metadata().state() == State.AVAILABLE;
> + }
> + }, complexId(nic.dataCenterId(), testServer.id(), nic.id()));
Why assembling this into a String to just split it immediately? Why not passing directly the Nic object to the predicate?
---
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/249/files/a17eac2160481358b8735aab30ecf12d672b341b#r56996964
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.apache.jclouds.profitbricks.rest.domain.Server;
> +import org.apache.jclouds.profitbricks.rest.ids.ServerRef;
> +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 = "NicApiLiveTest")
> +public class NicApiLiveTest extends BaseProfitBricksLiveTest {
> +
> + DataCenter dataCenter;
> + Server testServer;
> + Nic testNic;
Make all these variables private unless there is a real need to have it package-visible.
---
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/249/files/a17eac2160481358b8735aab30ecf12d672b341b#r56995718
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by mirza-spc <no...@github.com>.
Commits are squashed!
---
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/249#issuecomment-202295931
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [e95de6a7](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/e95de6a7). 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/249#issuecomment-202432926
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
> + @PATCH
> + @Path("/{nicId}")
> + @MapBinder(UpdateNicRequestBinder.class)
> + @ResponseParser(NicApi.NicParser.class)
> + @Produces("application/vnd.profitbricks.partial-properties+json")
> + Nic updateNic(@PayloadParam("nic") Nic.Request.UpdatePayload payload);
> +
> + @Named("nic:delete")
> + @DELETE
> + @Path("/{nicId}")
> + @Fallback(Fallbacks.VoidOnNotFoundOr404.class)
> + void deleteNic(@PathParam("dataCenterId") String dataCenterId, @PathParam("serverId") String serverId, @PathParam("nicId") String nicId);
> +
> + static final class NicParser extends ParseJson<Nic> {
> +
> + final ParseId parseService;
Make this 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/249/files/a17eac2160481358b8735aab30ecf12d672b341b#r56995629
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + if (payload.dhcp() != null)
> + properties.put("dhcp", payload.dhcp());
> +
> + if (payload.firewallActive() != null)
> + properties.put("firewallActive", payload.firewallActive());
> +
> + if (payload.firewallrules() != null) {
> + Map<String, Object> entities = new HashMap<String, Object>();
> + entities.put("firewallrules", payload.firewallrules());
> + properties.put("entities", entities);
> + }
> +
> + requestBuilder.put("properties", properties);
> +
> + return (new Gson()).toJson(requestBuilder);
Use the injected `Json`.
---
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/249/files/a17eac2160481358b8735aab30ecf12d672b341b#r56994139
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by mirza-spc <no...@github.com>.
> + .dataCenterId(testNic.dataCenterId())
> + .serverId(testServer.id())
> + .id(testNic.id())
> + .name("apache-nic")
> + .build());
> +
> + assertNicAvailable(testNic);
> +
> + Nic nic = nicApi().getNic(dataCenter.id(), testServer.id(), testNic.id());
> +
> + assertEquals(nic.properties().name(), "apache-nic");
> + }
> +
> +
> + @Test(dependsOnMethods = "testUpdateNic")
> + public void testDeleteNic() {
But if the update test fails, we don't consider the tests passing? Also I'd still have to wait since I don't really want to update and delete at the same time.
---
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/249/files/a17eac2160481358b8735aab30ecf12d672b341b#r56998364
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
> + .dataCenterId(testNic.dataCenterId())
> + .serverId(testServer.id())
> + .id(testNic.id())
> + .name("apache-nic")
> + .build());
> +
> + assertNicAvailable(testNic);
> +
> + Nic nic = nicApi().getNic(dataCenter.id(), testServer.id(), testNic.id());
> +
> + assertEquals(nic.properties().name(), "apache-nic");
> + }
> +
> +
> + @Test(dependsOnMethods = "testUpdateNic")
> + public void testDeleteNic() {
We should be able to test this even if the update test fails.
---
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/249/files/a17eac2160481358b8735aab30ecf12d672b341b#r56996018
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
> + .dataCenterId(testNic.dataCenterId())
> + .serverId(testServer.id())
> + .id(testNic.id())
> + .name("apache-nic")
> + .build());
> +
> + assertNicAvailable(testNic);
> +
> + Nic nic = nicApi().getNic(dataCenter.id(), testServer.id(), testNic.id());
> +
> + assertEquals(nic.properties().name(), "apache-nic");
> + }
> +
> +
> + @Test(dependsOnMethods = "testUpdateNic")
> + public void testDeleteNic() {
Tests don't pass but we need to know which pass and which don't. Just add an `alwaysRun = true` to the test annotation in this method.
---
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/249/files/a17eac2160481358b8735aab30ecf12d672b341b#r57001960
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
Just one comment left. Thanks for the quick fixes!
---
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/249#issuecomment-200276340
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
> + if (nic == null || nic.metadata() == null)
> + return false;
> +
> + return nic.metadata().state() == State.AVAILABLE;
> + }
> + }, nic);
> + }
> +
> + private void assertNicRemoved(Nic nic) {
> + assertPredicate(new Predicate<String>() {
> + @Override
> + public boolean apply(String args) {
> + String[] params = args.split(",");
> + return nicApi().get(params[0], params[1], params[2]) == null;
> + }
> + }, complexId(nic.dataCenterId(), testServer.id(), nic.id()));
Use the Nic for the predicate here too.
---
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/249/files/9aae5625aabf5770d221f4af46ed8f1293c5c38b#r57133650
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
Thanks! Can you squash the commits before merging?
---
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/249#issuecomment-200744377
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.rest.annotations.PATCH;
> +import org.jclouds.rest.annotations.PayloadParam;
> +import org.jclouds.rest.annotations.RequestFilters;
> +import org.jclouds.rest.annotations.ResponseParser;
> +import org.jclouds.rest.annotations.SelectJson;
> +import org.jclouds.util.Strings2;
> +
> +@Path("/datacenters/{dataCenterId}/servers/{serverId}/nics")
> +@RequestFilters(BasicAuthentication.class)
> +public interface NicApi extends Closeable {
> +
> + @Named("nic:list")
> + @GET
> + @SelectJson("items")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<Nic> getList(@PathParam("dataCenterId") String dataCenterId, @PathParam("serverId") String serverId);
This was already discussed. Let's start using a consistent naming in new APIs ()feel free to modify the existing ones if you want). Rename this to just "list", the get operations to just "get", etc. This is the NicApi, so there is no need for the method names to include the context of what entity is being managed.
---
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/249/files/a17eac2160481358b8735aab30ecf12d672b341b#r56995302
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
Closed #249.
---
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/249#event-605322849
Re: [jclouds-labs] Profitbricks REST - NIC API (#249)
Posted by Ignasi Barrera <no...@github.com>.
> + dataCenterId = payload.dataCenterId();
> + serverId = payload.serverId();
> + nicId = payload.id();
> +
> + requestBuilder.put("lan", payload.lan());
> +
> + if (payload.name() != null)
> + requestBuilder.put("name", payload.name());
> +
> + if (payload.ips() != null && !payload.ips().isEmpty())
> + requestBuilder.put("ips", payload.ips());
> +
> + if (payload.dhcp() != null)
> + requestBuilder.put("dhcp", payload.dhcp());
> +
> + return (new Gson()).toJson(requestBuilder);
Same here; use the injected one.
---
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/249/files/a17eac2160481358b8735aab30ecf12d672b341b#r56994321