You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by jasminSPC <no...@github.com> on 2015/02/27 23:30:30 UTC
[jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
JClouds Profitbricks provider - LoadBalancer API
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/143
-- Commit Summary --
* JClouds Profitbricks provider - LoadBalancer API
-- File Changes --
M profitbricks/src/main/java/org/jclouds/profitbricks/ProfitBricksApi.java (4)
A profitbricks/src/main/java/org/jclouds/profitbricks/binder/loadbalancer/CreateLoadBalancerRequestBinder.java (50)
A profitbricks/src/main/java/org/jclouds/profitbricks/binder/loadbalancer/DeregisterLoadBalancerRequestBinder.java (45)
A profitbricks/src/main/java/org/jclouds/profitbricks/binder/loadbalancer/RegisterLoadBalancerRequestBinder.java (47)
A profitbricks/src/main/java/org/jclouds/profitbricks/binder/loadbalancer/UpdateLoadBalancerRequestBinder.java (44)
A profitbricks/src/main/java/org/jclouds/profitbricks/domain/LoadBalancer.java (395)
M profitbricks/src/main/java/org/jclouds/profitbricks/domain/Server.java (27)
A profitbricks/src/main/java/org/jclouds/profitbricks/features/LoadBalancerApi.java (89)
M profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/firewall/FirewallListResponseHandler.java (9)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/loadbalancer/BaseLoadBalancerResponseHandler.java (102)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/loadbalancer/LoadBalancerListResponseHandler.java (70)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/loadbalancer/LoadBalancerResponseHandler.java (65)
M profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/server/BaseServerResponseHandler.java (8)
M profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/server/ServerListResponseHandler.java (2)
A profitbricks/src/test/java/org/jclouds/profitbricks/binder/loadbalancer/CreateLoadBalancerRequestBinderTest.java (54)
A profitbricks/src/test/java/org/jclouds/profitbricks/binder/loadbalancer/RegisterLoadBalancerRequestBinderTest.java (52)
A profitbricks/src/test/java/org/jclouds/profitbricks/features/LoadbalancerApiLiveTest.java (139)
A profitbricks/src/test/java/org/jclouds/profitbricks/features/LoadbalancerApiMockTest.java (243)
A profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/loadbalancer/LoadBalancerListResponseHandlerTest.java (75)
A profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/loadbalancer/LoadBalancerResponseHandlerTest.java (93)
A profitbricks/src/test/resources/loadbalancer/loadbalancer-create.xml (17)
A profitbricks/src/test/resources/loadbalancer/loadbalancer-delete.xml (12)
A profitbricks/src/test/resources/loadbalancer/loadbalancer-deregister.xml (14)
A profitbricks/src/test/resources/loadbalancer/loadbalancer-register.xml (13)
A profitbricks/src/test/resources/loadbalancer/loadbalancer-update.xml (15)
A profitbricks/src/test/resources/loadbalancer/loadbalancer.xml (32)
A profitbricks/src/test/resources/loadbalancer/loadbalancers.xml (57)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/143.patch
https://github.com/jclouds/jclouds-labs/pull/143.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by jasminSPC <no...@github.com>.
> + public abstract String lanId();
> +
> + @Nullable
> + public abstract ProvisioningState state();
> +
> + @Nullable
> + public abstract Date creationTime();
> +
> + @Nullable
> + public abstract Date lastModificationTime();
> +
> + @Nullable
> + public abstract List<Server> balancedServers();
> +
> + @Nullable
> + public abstract List<Firewall> firewalls();
I use the same object for as a response when creating https://github.com/StackPointCloud/jclouds-labs/commit/42a6251088b7aa403407676e90ee70cf65a97503#diff-bf57b5d122ac037379a25c1825cf7badR65
Do you think I should change this to return RequestId only?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26167186
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Matt Baldwin <no...@github.com>.
@nacx No worries. Thanks. Good luck with the 1.9.0 release!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-85589488
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + }
> +
> + @Test
> + public void testGetAllLoadBalancers() {
> + List<LoadBalancer> loadBalancers = api.loadBalancerApi().getAllLoadBalancers();
> +
> + assertNotNull(loadBalancers);
> + assertFalse(loadBalancers.isEmpty());
> + }
> +
> + @Test(dependsOnMethods = "testCreateLoadBalancer")
> + public void testGetLoadBalancer() {
> + LoadBalancer loadBalancer = api.loadBalancerApi().getLoadBalancer(loadBalancerID);
> +
> + System.out.println(loadBalancer);
Remove this
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26055065
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.profitbricks.http.parser.server.ServerListResponseHandler;
> +import org.xml.sax.Attributes;
> +import org.xml.sax.SAXException;
> +
> +public abstract class BaseLoadBalancerResponseHandler<T> extends BaseProfitBricksResponseHandler<T> {
> +
> + protected final ServerListResponseHandler balancedServerResponseHandler;
> + protected final FirewallListResponseHandler firewallListResponseHandler;
> +
> + protected LoadBalancer.Builder builder;
> + protected final DateCodec dateCodec;
> +
> + protected boolean useBalancedServerParser = false;
> + protected boolean useFirewallParser = false;
> +
> + protected BaseLoadBalancerResponseHandler(DateCodecFactory dateCodec, ServerListResponseHandler balancedServerResponseHandler, FirewallListResponseHandler firewallResponseHandler) {
Add null checks for the mandatory parameters. Although subclasses use injection, this class does not know how subclasses are built, so we'd better check the input parameters here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26054244
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> + public abstract String lanId();
> +
> + @Nullable
> + public abstract ProvisioningState state();
> +
> + @Nullable
> + public abstract Date creationTime();
> +
> + @Nullable
> + public abstract Date lastModificationTime();
> +
> + @Nullable
> + public abstract List<Server> balancedServers();
> +
> + @Nullable
> + public abstract List<Firewall> firewalls();
Yes, I would change that. Think about how a user will interact with that API. What can be done with the returned object after it is created? Nothing. Then we'd better return the request id and let the user get the right object.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26168761
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
Add the missing tests for the `DeregisterLoadBalancerRequestBinder` and `UpdateLoadBalancerRequestBinder`, and update the Firewall and Server list handler tests to properly test the new elements introduced in this commit.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-77899841
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
Hi! 1.9.0 is out and we're back to reviewing PRs. I've seen there are several files with Windows line endings. Mind fixing that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-88236951
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.profitbricks.http.parser.server.ServerListResponseHandler;
> +import org.xml.sax.Attributes;
> +import org.xml.sax.SAXException;
> +
> +public abstract class BaseLoadBalancerResponseHandler<T> extends BaseProfitBricksResponseHandler<T> {
> +
> + protected final ServerListResponseHandler balancedServerResponseHandler;
> + protected final FirewallListResponseHandler firewallListResponseHandler;
> +
> + protected LoadBalancer.Builder builder;
> + protected final DateCodec dateCodec;
> +
> + protected boolean useBalancedServerParser = false;
> + protected boolean useFirewallParser = false;
> +
> + protected BaseLoadBalancerResponseHandler(DateCodecFactory dateCodec, ServerListResponseHandler balancedServerResponseHandler, FirewallListResponseHandler firewallResponseHandler) {
Yes, I'd do it in both places, and I'd remove the `@Inject` annotation from the one you reference.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26169600
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> + .lanId("1")
> + .serverIds(serverIds)
> + .build();
> +
> + LoadBalancer loadBalancer = api.loadBalancerApi().createLoadBalancer(payload);
> +
> + System.out.println(loadBalancer);
> + assertNotNull(loadBalancer);
> +
> + }
> +
> + @Test
> + public void testGetAllLoadBalancers() {
> + List<LoadBalancer> loadBalancers = api.loadBalancerApi().getAllLoadBalancers();
> +
> + assertNotNull(loadBalancers);
The API should never return null lists. Remove this.
Also, should this be executed after creating the load balancer?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26055052
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
Looked at it again and it was only the Server object.
Pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/823df74f) and [1.9.x](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/c607a19a). Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-92297167
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
Apologies for the absence of feedback. We are about to release jclouds 1.9.0, and the master branch are under code freeze. We are fixing an issue spotted while testing the first release candidate, and expect to have the release out by the end of this week. Once that happens I'll be back to the PR backlog. Apologies for the delay!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-85204656
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Matt Baldwin <no...@github.com>.
@nacx Just following up on this PR. Is there any outstanding work on our end we should do?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-85134772
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import autovalue.shaded.com.google.common.common.collect.Lists;
> +import com.google.inject.Inject;
> +import java.util.List;
> +import org.jclouds.date.DateCodecFactory;
> +import org.jclouds.profitbricks.domain.LoadBalancer;
> +import org.jclouds.profitbricks.http.parser.firewall.FirewallListResponseHandler;
> +import org.jclouds.profitbricks.http.parser.server.ServerListResponseHandler;
> +import org.xml.sax.SAXException;
> +
> +public class LoadBalancerListResponseHandler extends BaseLoadBalancerResponseHandler<List<LoadBalancer>> {
> +
> + private final List<LoadBalancer> loadBalancers;
> +
> + @Inject
> + protected LoadBalancerListResponseHandler(DateCodecFactory dateCodec, ServerListResponseHandler balancedServerResponseHandler, FirewallListResponseHandler firewallListResponseHandler) {
Make this package private.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26054286
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> + super("loadbalancer");
> + this.requestBuilder = new StringBuilder(128 * 4);
> + }
> +
> + @Override
> + protected String createPayload(LoadBalancer.Request.DeregisterPayload payload) {
> + requestBuilder.append("<ws:deregisterServersOnLoadBalancer>")
> + .append("<request>");
> + for (String s : payload.serverIds()) {
> + requestBuilder.append(format("<serverIds>%s</serverIds>", s));
> + }
> + requestBuilder.append(format("<loadBalancerId>%s</loadBalancerId>", payload.id()))
> + .append("</request>")
> + .append("</ws:deregisterServersOnLoadBalancer>");
> +
> + return requestBuilder.toString().replaceAll("\\s+", "");
Is this really needed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26053349
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> + public abstract String id();
> +
> + @Nullable
> + public abstract String name();
> +
> + @Nullable
> + public abstract LoadbalancerAlgorithm loadBalancerAlgorithm();
> +
> + @Nullable
> + public abstract String dataCenterId();
> +
> + @Nullable
> + public abstract String dataCenterVersion();
> +
> + @Nullable
> + public abstract boolean internetAccess();
Primitive types can't be nullable
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26053652
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +@Test(groups = "unit", testName = "LoadbalancerApiMockTest")
> +public class LoadbalancerApiMockTest extends BaseProfitBricksMockTest {
> +
> + @Test
> + public void testGetAllLoadBalancers() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setBody(payloadFromResource("/loadbalancer/loadbalancers.xml")));
> +
> + ProfitBricksApi pbApi = api(server.getUrl(rootUrl));
> + LoadBalancerApi api = pbApi.loadBalancerApi();
> +
> + try {
> + List<LoadBalancer> loadBalancerList = api.getAllLoadBalancers();
> +
> + assertRequestHasCommonProperties(server.takeRequest());
Verify the body of the request too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26055259
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> + public abstract String lanId();
> +
> + @Nullable
> + public abstract ProvisioningState state();
> +
> + @Nullable
> + public abstract Date creationTime();
> +
> + @Nullable
> + public abstract Date lastModificationTime();
> +
> + @Nullable
> + public abstract List<Server> balancedServers();
> +
> + @Nullable
> + public abstract List<Firewall> firewalls();
Are **all** fields of the LoadBalancer really nullable? Is it possible that there is a load balancer without name, without id, algorithm, etc? If not, configure `@Nullable` annoations properly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26053809
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @AutoValue
> + public abstract static class CreatePayload {
> +
> + public abstract String dataCenterId();
> +
> + public abstract String loadBalancerName();
> +
> + public abstract LoadbalancerAlgorithm loadBalancerAlgorithm();
> +
> + public abstract String ip();
> +
> + @Nullable
> + public abstract String lanId();
> +
> + @Nullable
Avoid null lists and make sure they're immutable.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26053960
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by jasminSPC <no...@github.com>.
> +import org.jclouds.profitbricks.http.parser.server.ServerListResponseHandler;
> +import org.xml.sax.Attributes;
> +import org.xml.sax.SAXException;
> +
> +public abstract class BaseLoadBalancerResponseHandler<T> extends BaseProfitBricksResponseHandler<T> {
> +
> + protected final ServerListResponseHandler balancedServerResponseHandler;
> + protected final FirewallListResponseHandler firewallListResponseHandler;
> +
> + protected LoadBalancer.Builder builder;
> + protected final DateCodec dateCodec;
> +
> + protected boolean useBalancedServerParser = false;
> + protected boolean useFirewallParser = false;
> +
> + protected BaseLoadBalancerResponseHandler(DateCodecFactory dateCodec, ServerListResponseHandler balancedServerResponseHandler, FirewallListResponseHandler firewallResponseHandler) {
Should I throw exception if null? I don't see that we are doing the same here https://github.com/StackPointCloud/jclouds-labs/blob/master/profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/server/BaseServerResponseHandler.java#L50
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26169335
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public LoadbalancerAlgorithm loadBalancerAlgorithm;
> +
> + public String dataCenterId;
> +
> + public String dataCenterVersion;
> +
> + public boolean internetAccess;
> +
> + public String ip;
> +
> + public String lanId;
> +
> + public ProvisioningState state;
> +
> + @Nullable
Don't use this in the builder. Builder properties are not meant to be read.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26053875
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> and update the Firewall and Server list handler tests to properly test the new elements introduced in this commit
The server object now seems to have nested load balancers, and the handler was updated to handle that. Update the handler tests to exercise that path too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-91041159
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
Just that minor and it will be ready to be merged. Apologies for the delay in the reviews!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-91041514
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by jasminSPC <no...@github.com>.
Ping
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-77446964
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +import java.util.List;
> +
> +@AutoValue
> +public abstract class LoadBalancer {
> +
> + public enum LoadbalancerAlgorithm {
> +
> + ROUND_ROBIN, UNRECOGNIZED;
> +
> + public static LoadbalancerAlgorithm
> + fromValue(String value) {
> + try {
> + return valueOf(value);
> + } catch (IllegalArgumentException e) {
> + return UNRECOGNIZED;
> + }
[nit] There is also a cleaner approach using Guava:
```java
return Enums.getIfPresent(LoadbalancerAlgorithm.class, value).or(UNRECOGNIZED);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26053629
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.profitbricks.domain.DataCenter;
> +import org.jclouds.profitbricks.domain.LoadBalancer;
> +import org.jclouds.profitbricks.domain.LoadBalancer.LoadbalancerAlgorithm;
> +import org.jclouds.profitbricks.domain.Server;
> +import org.testng.Assert;
> +import static org.testng.Assert.assertFalse;
> +import static org.testng.Assert.assertNotNull;
> +import org.testng.annotations.AfterClass;
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "unit", testName = "LoadbalancerApiLiveTest")
> +public class LoadbalancerApiLiveTest extends BaseProfitBricksLiveTest {
> +
> + String dataCenterId;
> + String loadBalancerID;
> + String serverId;
Any reason why these are not private?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26054352
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
Closed #143.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#event-279601647
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.common.collect.ImmutableList;
> +import java.util.List;
> +import org.jclouds.date.DateCodec;
> +import org.jclouds.date.DateCodecFactory;
> +import org.jclouds.http.functions.ParseSax;
> +import org.jclouds.profitbricks.domain.Firewall;
> +import org.jclouds.profitbricks.domain.LoadBalancer;
> +import org.jclouds.profitbricks.domain.LoadBalancer.LoadbalancerAlgorithm;
> +import org.jclouds.profitbricks.domain.ProvisioningState;
> +import org.jclouds.profitbricks.domain.Server;
> +import org.jclouds.profitbricks.http.parser.BaseResponseHandlerTest;
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "unit", testName = "LoadbalancerListResponseHandlerTest")
[nit] "LoadBalancer"
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26055335
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -84,14 +85,20 @@ public static Status fromValue(String v) {
> @Nullable
> public abstract List<Nic> nics();
>
> + @Nullable
> + public abstract String balancedNicId();
> +
> + @Nullable
> + public abstract boolean activate();
Primitives can't be null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26054024
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by jasminSPC <no...@github.com>.
@nacx I don't see any changes on Firewalls that I have made. I only used Firewalls as nested objects in LoadBalancers. Am I missing something?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-91501083
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
This PR has several commits, including a merge commit that will make it difficult to merge. Before we start reviewing (and adding commits that address the review comments), could you cleanup the PR so it has only the necessary commits, and does not have any merge commit? This will facilitate the subsequent review commits/squashes, etc.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-76727342
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> + LoadBalancerApi api = pbApi.loadBalancerApi();
> +
> + String id = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee";
> +
> + String content = "<ws:getLoadBalancer><loadBalancerId>" + id + "</loadBalancerId></ws:getLoadBalancer>";
> + try {
> + LoadBalancer loadBalancer = api.getLoadBalancer(id);
> +
> + assertRequestHasCommonProperties(server.takeRequest(), content);
> + assertNotNull(loadBalancer);
> + assertEquals(loadBalancer.id(), id);
> + } finally {
> + pbApi.close();
> + server.shutdown();
> + }
> + }
Both get methods define a fallback. Add the corresponding tests.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26055169
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by jasminSPC <no...@github.com>.
@nacx Last week I fixed those line endings. Is there something I missed or should I squash it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143#issuecomment-90199315
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public Builder serverIds(List<String> serverIds) {
> + this.serverIds = serverIds;
> + return this;
> + }
> +
> + public CreatePayload build() {
> + return CreatePayload.create(dataCenterId, loadBalancerName, loadBalancerAlgorithm, ip, lanId, serverIds);
> + }
> + }
> + }
> +
> + @AutoValue
> + public abstract static class RegisterPayload {
> +
> + public abstract List<String> serverIds();
Same comment about immutable lists.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26053979
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> + public void testCreateLoadBalancer() {
> + List<String> serverIds = com.google.common.collect.Lists.newArrayList();
> + serverIds.add("server-ids");
> +
> + LoadBalancer.Request.CreatePayload payload = LoadBalancer.Request.creatingBuilder()
> + .dataCenterId(dataCenterId)
> + .loadBalancerName("testName")
> + .loadBalancerAlgorithm(LoadbalancerAlgorithm.ROUND_ROBIN)
> + .ip("0.0.0.1")
> + .lanId("1")
> + .serverIds(serverIds)
> + .build();
> +
> + LoadBalancer loadBalancer = api.loadBalancerApi().createLoadBalancer(payload);
> +
> + System.out.println(loadBalancer);
Remove this
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26055074
Re: [jclouds-labs] JClouds Profitbricks provider - LoadBalancer API
(#143)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public Builder id(String id) {
> + this.id = id;
> + return this;
> + }
> +
> + public RegisterPayload build() {
> + return RegisterPayload.create(serverIds, id);
> + }
> + }
> + }
> +
> + @AutoValue
> + public abstract static class DeregisterPayload {
> +
> + public abstract List<String> serverIds();
Same here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/143/files#r26053995