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/26 22:38:16 UTC
[jclouds-labs] Profitbricks IpBlock API (#141)
Profitbricks IpBlock API - Cleaned up
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/141
-- Commit Summary --
* Profitbricks IpBlock API
-- File Changes --
M profitbricks/src/main/java/org/jclouds/profitbricks/ProfitBricksApi.java (5)
A profitbricks/src/main/java/org/jclouds/profitbricks/domain/IpBlock.java (125)
A profitbricks/src/main/java/org/jclouds/profitbricks/features/IpBlockApi.java (80)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/ipblock/BaseIpBlockResponseHandler.java (70)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/ipblock/IpBlockListResponseHandler.java (62)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/ipblock/IpBlockResponseHandler.java (64)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/publicip/BasePublicIpResponseHandler.java (40)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/publicip/PublicIpListResponseHandler.java (51)
A profitbricks/src/test/java/org/jclouds/profitbricks/features/IpBlockApiLiveTest.java (92)
A profitbricks/src/test/java/org/jclouds/profitbricks/features/IpBlockApiMockTest.java (181)
A profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/ipblock/IpBlockListResponseHandlerTest.java (77)
A profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/ipblock/IpBlockResponseHandlerTest.java (63)
A profitbricks/src/test/resources/ipblock/ipblock-addtonic.xml (12)
A profitbricks/src/test/resources/ipblock/ipblock-release.xml (10)
A profitbricks/src/test/resources/ipblock/ipblock-removefromnic.xml (12)
A profitbricks/src/test/resources/ipblock/ipblock-reserve.xml (13)
A profitbricks/src/test/resources/ipblock/ipblock.xml (18)
A profitbricks/src/test/resources/ipblock/ipblocks.xml (30)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/141.patch
https://github.com/jclouds/jclouds-labs/pull/141.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.common.collect.Iterables;
> +import java.util.List;
> +import org.jclouds.profitbricks.BaseProfitBricksLiveTest;
> +import org.jclouds.profitbricks.domain.IpBlock;
> +import org.jclouds.profitbricks.domain.Location;
> +import org.jclouds.profitbricks.domain.Nic;
> +import static org.testng.Assert.assertFalse;
> +import static org.testng.Assert.assertNotNull;
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "live", testName = "IpBlockApiLiveTest", singleThreaded = true)
> +public class IpBlockApiLiveTest extends BaseProfitBricksLiveTest {
> +
> + String nicid;
> + IpBlock newIpBlock;
> + String ip;
Any reason why these variables are package private and not private?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25603710
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
> + public void testGetAllIpBlock() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setBody(payloadFromResource("/ipblock/ipblocks.xml")));
> +
> + ProfitBricksApi pbApi = api(server.getUrl(rootUrl));
> + IpBlockApi api = pbApi.ipBlockApi();
> +
> + try {
> + List<IpBlock> ipBlocks = api.getAllIpBlock();
> + assertRequestHasCommonProperties(server.takeRequest());
> + assertNotNull(ipBlocks);
> + } finally {
> + pbApi.close();
> + server.shutdown();
> + }
> + }
The get all blocks method also defines a 404 fallback. Add the corresponding test.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25603960
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Test
> + public void testReservePublicIpBlock() {
> + newIpBlock = api.ipBlockApi().reservePublicIpBlock("2", Location.US_LAS.value());
> +
> + assertNotNull(newIpBlock);
> + assertNotNull(newIpBlock.ips());
> + assertFalse(newIpBlock.ips().isEmpty());
> + }
> +
> + @Test
> + public void testGetAllIpBlocks() {
> + List<IpBlock> ipBlocks = api.ipBlockApi().getAllIpBlock();
> +
> + assertNotNull(ipBlocks);
The jclouds fallback always returns a list (or an empty one). Should we better add some assertion to verify that the list has the expected elements?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25603854
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by jasminSPC <no...@github.com>.
> + if ("publicIps".equals(qName))
> + publicIps.add(builder.build());
> + clearTextBuffer();
> + }
> +
> + @Override
> + public void reset() {
> + this.publicIps = Lists.newArrayList();
> + }
> +
> + @Override
> + public List<PublicIp> getResult() {
> + return publicIps;
> + }
> +
> +}
In our IRC conversation we said to ignore this one.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25722875
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by jasminSPC <no...@github.com>.
> + if ("publicIps".equals(qName))
> + publicIps.add(builder.build());
> + clearTextBuffer();
> + }
> +
> + @Override
> + public void reset() {
> + this.publicIps = Lists.newArrayList();
> + }
> +
> + @Override
> + public List<PublicIp> getResult() {
> + return publicIps;
> + }
> +
> +}
This is the same case like this one Firewall.Rule
https://github.com/jclouds/jclouds-labs/blob/master/profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/firewall/rule/FirewallRuleListResponseHandler.java
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25618713
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.profitbricks.http.parser.publicip;
> +
> +import com.google.inject.Inject;
> +import org.jclouds.profitbricks.domain.IpBlock;
> +import org.jclouds.profitbricks.http.parser.BaseProfitBricksResponseHandler;
> +
> +public abstract class BasePublicIpResponseHandler<T> extends BaseProfitBricksResponseHandler<T> {
> +
> + protected IpBlock.PublicIp.Builder builder;
> +
> + @Inject
This has no effect here. Remove it and delegate the configuration to the subclasses.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25603654
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import com.google.auto.value.AutoValue;
> +import com.google.common.collect.ImmutableList;
> +import java.util.List;
> +import org.jclouds.javax.annotation.Nullable;
> +
> +@AutoValue
> +public abstract class IpBlock {
> +
> + public abstract String id();
> +
> + public abstract Location location();
> +
> + public abstract List<PublicIp> publicIps();
> +
> + @Nullable
This is no longer nullable since the factory method sets it to an empty list if the input parameter is null. Remove the annotation or keep the null value when the input is null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25729423
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Matt Baldwin <no...@github.com>.
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "live", testName = "IpBlockApiLiveTest", singleThreaded = true)
> +public class IpBlockApiLiveTest extends BaseProfitBricksLiveTest {
> +
> + String nicid;
> + IpBlock newIpBlock;
> + String ip;
> +
> + @Override
> + public void initialize() {
> + super.initialize();
> +
> + List<Nic> nics = api.nicApi().getAllNics();
> +
> + assertFalse(nics.isEmpty(), "At least one NIC is requred to test IpBlocks");
@nacx Thanks for reviewing all of this. This live test assumes the user has already built out their environment via API or UI with a datacenter, server, volume, and NIC. So, this is a precondition that will be met by default. This is very similar to the Server live tests where a datacenter is required before running the test.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25618414
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @jasminSPC! Just one comment remaining. Feel free to squash the commits into a single one when addressing it, and I'll merge the PR. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141#issuecomment-77041850
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
> import org.jclouds.profitbricks.features.NicApi;
> import org.jclouds.profitbricks.features.ServerApi;
> import org.jclouds.profitbricks.features.StorageApi;
> import org.jclouds.rest.annotations.Delegate;
> -
Add this line back
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25603370
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by jasminSPC <no...@github.com>.
This has been squashed and ready for merge.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141#issuecomment-77446805
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.javax.annotation.Nullable;
> +
> +@AutoValue
> +public abstract class IpBlock {
> +
> + public abstract String id();
> +
> + public abstract Location location();
> +
> + public abstract List<PublicIp> publicIps();
> +
> + @Nullable
> + public abstract List<String> ips();
> +
> + public static IpBlock create(String id, Location location, List<PublicIp> publicIps, List<String> ips) {
> + return new AutoValue_IpBlock(id, location, publicIps, ips);
To enforce the immutability of the list, if not null you should pass an `ImmutableList.copyOf(ips)` to the auto value constructor.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25603429
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
> + if ("publicIps".equals(qName))
> + publicIps.add(builder.build());
> + clearTextBuffer();
> + }
> +
> + @Override
> + public void reset() {
> + this.publicIps = Lists.newArrayList();
> + }
> +
> + @Override
> + public List<PublicIp> getResult() {
> + return publicIps;
> + }
> +
> +}
Add a unit test for this class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25604055
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [f5605dd4](https://git1-us-west.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=f5605dd4). Thanks @jasminSPC!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141#issuecomment-77772144
Re: [jclouds-labs] Profitbricks IpBlock API (#141)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.testng.annotations.Test;
> +
> +@Test(groups = "live", testName = "IpBlockApiLiveTest", singleThreaded = true)
> +public class IpBlockApiLiveTest extends BaseProfitBricksLiveTest {
> +
> + String nicid;
> + IpBlock newIpBlock;
> + String ip;
> +
> + @Override
> + public void initialize() {
> + super.initialize();
> +
> + List<Nic> nics = api.nicApi().getAllNics();
> +
> + assertFalse(nics.isEmpty(), "At least one NIC is requred to test IpBlocks");
Is this something that can be setup or is it a precondition that will be met by default?
I mean, a user with a default and clean account, will need to perform a manual configuration to be able to run thses live tests?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/141/files#r25603794