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/04/07 10:11:15 UTC
[jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
@jasminSPC
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/255
-- Commit Summary --
* Profitbricks REST - Firewall 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/firewall/CreateFirewallRuleRequestBinder.java (92)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/firewall/UpdateFirewallRuleRequestBinder.java (88)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/lan/CreateLanRequestBinder.java (4)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/binder/lan/UpdateLanRequestBinder.java (5)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/FirewallRule.java (148)
A profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/features/FirewallApi.java (130)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/binder/firewall/CreateFirewallRuleRequestBinderTest.java (71)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/binder/firewall/UpdateFirewallRuleRequestBinderTest.java (63)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/FirewallApiLiveTest.java (185)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/FirewallApiMockTest.java (155)
A profitbricks-rest/src/test/resources/firewall/get.json (24)
A profitbricks-rest/src/test/resources/firewall/list.json (31)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/255.patch
https://github.com/jclouds/jclouds-labs/pull/255.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/255
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
> +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 = "FirewallApiLiveTest")
> +public class FirewallApiLiveTest extends BaseProfitBricksLiveTest {
> +
> + DataCenter dataCenter;
> + Server testServer;
> + Nic testNic;
> + FirewallRule testFirewallRule;
Make the 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/255/files/a468fa433e15575616a062912756bb7e8a6f4dd0#r59285888
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
>
> - public abstract int portRangeEnd();
> + public abstract Integer portRangeEnd();
Any reason to convert to Integer if it is not marked as `@Nullable`?
---
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/255/files/a468fa433e15575616a062912756bb7e8a6f4dd0#r59285035
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @mirza-spc!
Most of the comments are exactly the same that were made in the previous LAN api pull request. Please, take your time to review and understand them in order to avoid repeating those patterns in upcoming pull requests.
---
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/255#issuecomment-208575081
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Matt Baldwin <no...@github.com>.
hey @nacx let me know if you need anything from our end to merge these PRs on into the project. We'd like to wrap them up soon :)
---
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/255#issuecomment-208412293
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by mirza-spc <no...@github.com>.
Sorry about that, these are basically still the PRs that were completed in december last year and sometimes things get overlooked while applying all of the changes because it's all a bit of mechanical repetition.
> On Apr 11, 2016, at 11:43 PM, Ignasi Barrera <no...@github.com> wrote:
>
> Thanks @mirza-spc!
> Most of the comments are exactly the same that were made in the previous LAN api pull request. Please, take your time to review and understand them in order to avoid repeating those patterns in upcoming pull requests.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly or view it on GitHub
>
---
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/255#issuecomment-208712026
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
> + assertEquals(firewallRule.properties().name(), "apache-firewall");
> +
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/datacenters/datacenter-id/servers/server-id/nics/nic-id/firewallrules/some-id?depth=3");
> + }
> +
> + public void testGetFirewallRuleWith404() throws InterruptedException {
> + server.enqueue(response404());
> +
> + FirewallRule firewallRule = firewallApi().get("datacenter-id", "server-id", "nic-id", "some-id");
> +
> + assertEquals(firewallRule, null);
> +
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/datacenters/datacenter-id/servers/server-id/nics/nic-id/firewallrules/some-id");
> + }
Still missing a test for the get firewall with depth and 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/255/files/958590fa1cb0101355e6cfee1893c43112c09253#r59632411
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
> + assertNotNull(list);
> + assertEquals(list.size(), 1);
> + assertEquals(list.get(0).properties().name(), "apache-firewall");
> +
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/datacenters/datacenter-id/servers/server-id/nics/nic-id/firewallrules?depth=3");
> + }
> +
> + @Test
> + public void testGetRuleListWith404() throws InterruptedException {
> + server.enqueue(response404());
> + List<FirewallRule> list = firewallApi().list("datacenter-id", "server-id", "nic-id", new DepthOptions().depth(1));
> + assertTrue(list.isEmpty());
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/datacenters/datacenter-id/servers/server-id/nics/nic-id/firewallrules?depth=1");
> + }
* Change the name to reflect that this tests the "withDepth" method.
* Still missing a test that verifies the list without depth and 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/255/files/958590fa1cb0101355e6cfee1893c43112c09253#r59632353
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
> + FirewallRule firewallRule = firewallApi().getFirewallRule(dataCenter.id(), testServer.id(), testNic.id(), testFirewallRule.id());
> +
> + assertNotNull(firewallRule);
> + assertEquals(firewallRule.id(), testFirewallRule.id());
> + }
> +
> + @Test(dependsOnMethods = "testCreateFirewallRule")
> + public void testListRules() {
> + List<FirewallRule> firewalls = firewallApi().getRuleList(dataCenter.id(), testServer.id(), testNic.id());
> +
> + assertNotNull(firewalls);
> + assertFalse(firewalls.isEmpty());
> + assertEquals(firewalls.size(), 1);
> + }
> +
> + @Test(dependsOnMethods = "testGetFirewallRule")
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/255/files/a468fa433e15575616a062912756bb7e8a6f4dd0#r59286316
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
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/{nicId}/firewallrules")
> +@RequestFilters(BasicAuthentication.class)
> +public interface FirewallApi extends Closeable {
> +
> + @Named("firewallRule:list")
> + @GET
> + @SelectJson("items")
> + @Fallback(EmptyListOnNotFoundOr404.class)
> + List<FirewallRule> getRuleList(@PathParam("dataCenterId") String dataCenterId, @PathParam("serverId") String serverId, @PathParam("nicId") String nicId);
Apply the same naming conventions (list, get, etc) used in the other APIs.
---
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/255/files/a468fa433e15575616a062912756bb7e8a6f4dd0#r59285570
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
> + @Test
> + public void testGetFirewallRule() throws InterruptedException {
> + MockResponse response = new MockResponse();
> + response.setBody(stringFromResource("/firewall/get.json"));
> + response.setHeader("Content-Type", "application/vnd.profitbricks.resource+json");
> +
> + server.enqueue(response);
> +
> + FirewallRule firewallRule = firewallApi().getFirewallRule("datacenter-id", "server-id", "nic-id", "some-id");
> +
> + assertNotNull(firewallRule);
> + assertEquals(firewallRule.properties().name(), "apache-firewall");
> +
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "GET", "/datacenters/datacenter-id/servers/server-id/nics/nic-id/firewallrules/some-id");
> + }
Add the missing get and list tests.
---
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/255/files/a468fa433e15575616a062912756bb7e8a6f4dd0#r59286691
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
> + requestBuilder.put("sourceIp", payload.sourceIp());
> +
> + if (payload.targetIp() != null)
> + requestBuilder.put("targetIp", payload.targetIp());
> +
> + if (payload.icmpCode() != null)
> + requestBuilder.put("icmpCode", payload.icmpCode());
> +
> + if (payload.icmpType() != null)
> + requestBuilder.put("icmpType", payload.icmpType());
> +
> + if (payload.portRangeStart() != null)
> + requestBuilder.put("portRangeStart", payload.portRangeStart());
> +
> + if (payload.portRangeEnd() != null)
> + requestBuilder.put("portRangeEnd", payload.portRangeEnd());
This pattern is everywhere and introduces quite verbosity to the code. Consider making a method int he base class `puIfPresent` or so, to remove the boilerplate.
---
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/255/files/a468fa433e15575616a062912756bb7e8a6f4dd0#r59285288
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
Squashed, rebased, and pushed to master as [5b757ee4](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/5b757ee4). Thanks!
---
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/255#issuecomment-211972052
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Test(dependsOnMethods = "testCreateFirewallRule")
> + public void testGetFirewallRule() {
> + FirewallRule firewallRule = firewallApi().getFirewallRule(dataCenter.id(), testServer.id(), testNic.id(), testFirewallRule.id());
> +
> + assertNotNull(firewallRule);
> + assertEquals(firewallRule.id(), testFirewallRule.id());
> + }
> +
> + @Test(dependsOnMethods = "testCreateFirewallRule")
> + public void testListRules() {
> + List<FirewallRule> firewalls = firewallApi().getRuleList(dataCenter.id(), testServer.id(), testNic.id());
> +
> + assertNotNull(firewalls);
> + assertFalse(firewalls.isEmpty());
> + assertEquals(firewalls.size(), 1);
Assert that the list contains the created element, instead of verifying its size.
---
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/255/files/a468fa433e15575616a062912756bb7e8a6f4dd0#r59286126
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + private void assertFirewallRuleRemoved(FirewallRule firewallRule) {
> + assertPredicate(new Predicate<FirewallRule>() {
> + @Override
> + public boolean apply(FirewallRule testRule) {
> + return firewallApi().getFirewallRule(testRule.dataCenterId(), testRule.serverId(), testRule.nicId(), testRule.id()) == null;
> + }
> + }, firewallRule);
> + }
> +
> + private FirewallApi firewallApi() {
> + return api.firewallApi();
> + }
> +
> + private void assertNicAvailable(Nic nic) {
The nic api live test already defines this. Please, reuse the code.
---
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/255/files/a468fa433e15575616a062912756bb7e8a6f4dd0#r59286577
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
>
> - public abstract int portRangeEnd();
> + public abstract Integer portRangeEnd();
Ping?
---
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/255/files/a468fa433e15575616a062912756bb7e8a6f4dd0#r60055801
Re: [jclouds/jclouds-labs] Profitbricks REST - Firewall API (#255)
Posted by Ignasi Barrera <no...@github.com>.
Closed #255.
---
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/255#event-633092929