You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by alibazlamit <no...@github.com> on 2016/08/04 10:33:49 UTC
[jclouds/jclouds-labs] JCLOUDS-1149 oneandone-firewallpolicies-api
(#306)
@nacx ready for review
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/306
-- Commit Summary --
* JCLOUDS-1149 oneandone-firewallpolicies-api
-- File Changes --
M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/OneAndOneApi.java (4)
A oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/FirewallPolicy.java (190)
M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/Types.java (14)
A oneandone/src/main/java/org/apache/jclouds/oneandone/rest/features/FirewallPolicyApi.java (124)
A oneandone/src/test/java/org/apache/jclouds/oneandone/rest/features/FirewallPolicyApiLiveTest.java (188)
A oneandone/src/test/java/org/apache/jclouds/oneandone/rest/features/FirewallPolicyApiMockTest.java (348)
M oneandone/src/test/java/org/apache/jclouds/oneandone/rest/internal/BaseOneAndOneLiveTest.java (13)
A oneandone/src/test/resources/firewallpolicies/get.json (26)
A oneandone/src/test/resources/firewallpolicies/get.rule.json (7)
A oneandone/src/test/resources/firewallpolicies/get.serverip.json (5)
A oneandone/src/test/resources/firewallpolicies/list.json (54)
A oneandone/src/test/resources/firewallpolicies/list.options.json (54)
A oneandone/src/test/resources/firewallpolicies/list.rules.json (16)
A oneandone/src/test/resources/firewallpolicies/list.serverips.json (7)
A oneandone/src/test/resources/sharedStorage/get.json (24)
A oneandone/src/test/resources/sharedStorage/list.access.json (24)
A oneandone/src/test/resources/sharedStorage/list.json (62)
A oneandone/src/test/resources/sharedStorage/list.options.json (62)
A oneandone/src/test/resources/sharedStorage/server.get.json (5)
A oneandone/src/test/resources/sharedStorage/servers.list.json (12)
M pom.xml (1)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/306.patch
https://github.com/jclouds/jclouds-labs/pull/306.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/306
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by alibazlamit <no...@github.com>.
@alibazlamit pushed 1 commit.
0ed6bdc checkstyle removed
---
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/306/files/c59e1941c2c2c1d5d00dfc0d537995c3c0f1ee68..0ed6bdcd21cec17efdca5e0aa61be88c33d22f32
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public abstract String serverName();
> +
> + @SerializedNames({"id", "ip", "server_name"})
> + public static ServerIp create(String id, String ip, String serverName) {
> + return new AutoValue_FirewallPolicy_ServerIp(id, ip, serverName);
> + }
> +
> + @AutoValue
> + public abstract static class CreateServerIp {
> +
> + public abstract List<String> serverIps();
> +
> + @SerializedNames({"server_ips"})
> + public static CreateServerIp create(final List<String> serverIps) {
> + return new AutoValue_FirewallPolicy_ServerIp_CreateServerIp(serverIps == null ? ImmutableList.<String>of() : serverIps);
Same here. Use `copyOf` for lists when present to enforce immutability. Apply this pattern to all domain classes.
---
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/306/files/c59e1941c2c2c1d5d00dfc0d537995c3c0f1ee68#r73499646
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + assertNotNull(updateResult);
> + assertEquals(updateResult.name(), updatedName);
> + }
> +
> + @Test(dependsOnMethods = "testUpdate")
> + public void testAssignServerIp() {
> + assertNodeAvailable(currentServer);
> + currentServer = updateServerStatus(currentServer);
> + List<String> ips = new ArrayList<String>();
> + ips.add(currentServer.ips().get(0).id());
> + FirewallPolicy.ServerIp.CreateServerIp toAdd = FirewallPolicy.ServerIp.CreateServerIp.create(ips);
> +
> + FirewallPolicy updateResult = firewallPolicyApi().assignServerIp(currentFirewallPolicy.id(), toAdd);
> +
> + assertNotNull(updateResult);
Is there a more solid way to test that the result of the operation is the expected 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/306/files/c59e1941c2c2c1d5d00dfc0d537995c3c0f1ee68#r73499996
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by alibazlamit <no...@github.com>.
@nacx Done, 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/306#issuecomment-237869450
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by alibazlamit <no...@github.com>.
@alibazlamit pushed 1 commit.
b56bbcf Minor changes updated
---
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/306/files/0ed6bdcd21cec17efdca5e0aa61be88c33d22f32..b56bbcf958782163465e1ab683f47a394a4b541f
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by alibazlamit <no...@github.com>.
@alibazlamit pushed 1 commit.
6b48f4d Fixed the thread sleep issue
---
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/306/files/e71ff59d3b71afca0b0e3a128a6ef786aa84134e..6b48f4d5158feae40eea2af4241058c3e8d39c2e
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> + @Nullable
> + public abstract String creationDate();
> +
> + @Nullable
> + public abstract String defaultState();
> +
> + public abstract List<Rule> rules();
> +
> + public abstract List<ServerIp> serverIps();
> +
> + @Nullable
> + public abstract String cloudpanelId();
> +
> + @SerializedNames({"id", "name", "description", "state", "creation_date", "default", "rules", "server_ips", "cloudpanel_id"})
> + public static FirewallPolicy create(String id, String name, String description, String state, String creationDate, String defaultState, List<Rule> rules, List<ServerIp> serverIps, String cloudpanelId) {
> + return new AutoValue_FirewallPolicy(id, name, description, state, creationDate, defaultState, rules == null ? ImmutableList.<Rule>of() : rules, serverIps == null ? ImmutableList.<ServerIp>of() : serverIps, cloudpanelId);
Use `copyOf` for lists when present to enforce immutability.
---
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/306/files/c59e1941c2c2c1d5d00dfc0d537995c3c0f1ee68#r73499530
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> + @PUT
> + @Path("/{firewallPolicyId}")
> + FirewallPolicy update(@PathParam("firewallPolicyId") String firewallPolicyId, @BinderParam(BindToJsonPayload.class) FirewallPolicy.UpdateFirewallPolicy firewallPolicy);
> +
> + @Named("firewallpolicies:delete")
> + @DELETE
> + @Path("/{firewallPolicyId}")
> + @MapBinder(BindToJsonPayload.class)
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> + FirewallPolicy delete(@PathParam("firewallPolicyId") String firewallPolicyId);
> +
> + @Named("firewallpolicies:serverips:list")
> + @GET
> + @Path("/{firewallPolicyId}/server_ips")
> + @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
> + List<FirewallPolicy.ServerIp> getServerIps(@PathParam("firewallPolicyId") String firewallPolicyId);
Rename to `listServerIps`.
---
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/306/files/c59e1941c2c2c1d5d00dfc0d537995c3c0f1ee68#r73499788
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by alibazlamit <no...@github.com>.
> @@ -102,6 +105,16 @@ protected Server createServer(String serverName) {
> .powerOn(Boolean.TRUE).build());
> }
>
> + protected Server updateServerStatus(Server server) {
This is just to get the latest state of the Server from the API its more like getServer nothing is being blocked except that sometimes users need the latest Server state, what do you think?
---
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/306/files/0ed6bdcd21cec17efdca5e0aa61be88c33d22f32#r73529066
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -102,6 +105,16 @@ protected Server createServer(String serverName) {
> .powerOn(Boolean.TRUE).build());
> }
>
> + protected Server updateServerStatus(Server server) {
That's fine, my concern is to have a method that encapsulates that "sleep". Is this need to wait a common pattern users will have to deal with? If the answer is yes, we should consider providing a predicate users can easily use to wait until a server is ready / in the desired state.
---
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/306/files/0ed6bdcd21cec17efdca5e0aa61be88c33d22f32#r73560285
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @alibazlamit! Mind squashing the commits so I can cleanly merge the PR?
---
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/306#issuecomment-237860488
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -102,6 +105,16 @@ protected Server createServer(String serverName) {
> .powerOn(Boolean.TRUE).build());
> }
>
> + protected Server updateServerStatus(Server server) {
Why do we need to use this? Is this a common pattern users will need to use when consuming this api? If this is the case, we should consider providing predicates that block until the resource is ready.
---
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/306/files/0ed6bdcd21cec17efdca5e0aa61be88c33d22f32#r73500736
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Test(dependsOnMethods = "testAddRules")
> + public void testGetRule() {
> + FirewallPolicy.Rule result = firewallPolicyApi().getRule(currentFirewallPolicy.id(), currentFirewallPolicy.rules().get(0).id());
> +
> + assertNotNull(result);
> + }
> +
> + @Test(dependsOnMethods = "testGetRule")
> + public void testRemoveRule() {
> + FirewallPolicy result = firewallPolicyApi().removeRule(currentFirewallPolicy.id(), currentFirewallPolicy.rules().get(0).id());
> +
> + assertNotNull(result);
> + assertEquals(result.id(), currentFirewallPolicy.id());
> + }
Same comments for the assign/remove rule. Is there a stronger way to test that the operation produced the desired results?
---
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/306/files/0ed6bdcd21cec17efdca5e0aa61be88c33d22f32#r73500196
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by alibazlamit <no...@github.com>.
> @@ -102,6 +105,16 @@ protected Server createServer(String serverName) {
> .powerOn(Boolean.TRUE).build());
> }
>
> + protected Server updateServerStatus(Server server) {
i have replaced the sleep with the required wait operations.
---
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/306/files/0ed6bdcd21cec17efdca5e0aa61be88c33d22f32#r73683177
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @Named("firewallpolicies:rules:create")
> + @POST
> + @Path("/{firewallPolicyId}/rules")
> + FirewallPolicy addRules(@PathParam("firewallPolicyId") String firewallPolicyId, @BinderParam(BindToJsonPayload.class) FirewallPolicy.Rule.AddRule rule);
> +
> + @Named("firewallpolicies:rules:get")
> + @GET
> + @Path("/{firewallPolicyId}/rules/{ruleId}")
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
> + FirewallPolicy.Rule getRule(@PathParam("firewallPolicyId") String firewallPolicyId, @PathParam("ruleId") String ruleId);
> +
> + @Named("firewallpolicies:rules:delete")
> + @DELETE
> + @Path("/{firewallPolicyId}/rules/{ruleId}")
> + @Fallback(Fallbacks.NullOnNotFoundOr404.class)
Fallback here is ambiguous, Better remove 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/306/files/c59e1941c2c2c1d5d00dfc0d537995c3c0f1ee68#r73499859
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @alibazlamit! I've already done an early review :)
---
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/306#issuecomment-237520149
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> + Assert.assertTrue(servers.size() > 0);
> + }
> +
> + @Test(dependsOnMethods = "testListServerIps")
> + public void testServerIpGet() {
> + FirewallPolicy.ServerIp result = firewallPolicyApi().getServerIp(currentFirewallPolicy.id(), currentServer.ips().get(0).id());
> +
> + assertNotNull(result);
> + }
> +
> + @Test(dependsOnMethods = "testServerIpGet")
> + public void testUnassignServer() {
> + FirewallPolicy result = firewallPolicyApi().unassignServerIp(currentFirewallPolicy.id(), currentServer.ips().get(0).id());
> +
> + assertNotNull(result);
> + assertEquals(result.id(), currentFirewallPolicy.id());
Also verify that the unassigned ip is no longer there?
---
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/306/files/c59e1941c2c2c1d5d00dfc0d537995c3c0f1ee68#r73500056
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> + return new AutoValue_FirewallPolicy(id, name, description, state, creationDate, defaultState, rules == null ? ImmutableList.<Rule>of() : rules, serverIps == null ? ImmutableList.<ServerIp>of() : serverIps, cloudpanelId);
> + }
> +
> + @AutoValue
> + public abstract static class CreateFirewallPolicy {
> +
> + public abstract String name();
> +
> + @Nullable
> + public abstract String description();
> +
> + public abstract List<Rule.CreatePayload> rules();
> +
> + @SerializedNames({"name", "description", "rules"})
> + public static CreateFirewallPolicy create(final String name, final String description, List<Rule.CreatePayload> rules) {
> + return new AutoValue_FirewallPolicy_CreateFirewallPolicy(name, description, rules == null ? ImmutableList.<Rule.CreatePayload>of() : rules);
Same here. Use `copyOf` for lists when present to enforce immutability.
---
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/306/files/c59e1941c2c2c1d5d00dfc0d537995c3c0f1ee68#r73499585
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by alibazlamit <no...@github.com>.
Thanks @nacx for the early one, everything is changed except for the comments i left there.
---
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/306#issuecomment-237582658
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
Closed #306.
---
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/306#event-748009141
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
> + assertNotNull(result);
> + }
> +
> + @Test(dependsOnMethods = "testGetRule")
> + public void testRemoveRule() {
> + FirewallPolicy result = firewallPolicyApi().removeRule(currentFirewallPolicy.id(), currentFirewallPolicy.rules().get(0).id());
> +
> + assertNotNull(result);
> + assertEquals(result.id(), currentFirewallPolicy.id());
> + }
> +
> + @AfterClass(alwaysRun = true)
> + public void teardownTest() throws InterruptedException {
> + //give time for operations to finish
> + firewallPolicyApi().delete(currentFirewallPolicy.id());
> + Thread.sleep(15000);
Doing this in a live test is a symptom of something that users will probably have to do themselves. better use the build-in `Predicates2.retry` construct to wait until the server is deleted and consider binding it to the guice context so it can be reused where needed.
---
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/306/files/0ed6bdcd21cec17efdca5e0aa61be88c33d22f32#r73500368
Re: [jclouds/jclouds-labs] JCLOUDS-1149
oneandone-firewallpolicies-api (#306)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [50744595](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/50744595). Thanks @alibazlamit!
---
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/306#issuecomment-238159993