You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Reijhanniel Jearl Campos <no...@github.com> on 2015/01/14 07:55:56 UTC
[jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server &
Image API (#119)
Hi! This PR is the implementation of ProfitBricks' API for:
* **Server** - [live test](https://gist.github.com/devcsrj/6a17a3c1347f740b0715)
* **Image** - [live test](https://gist.github.com/devcsrj/e4ab53864b75afa9655b)
As usual, looking forward to awesome code reviews!
You can merge this Pull Request by running:
git pull https://github.com/devcsrj/jclouds-labs master
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/119
-- Commit Summary --
* JCLOUDS-702: JCloud ProfitBricks provider implementation
* Merge remote-tracking branch 'upstream/master'
* Image Api
* Server api
* Merge branch 'master' of github.com:jclouds/jclouds-labs
-- File Changes --
M profitbricks/src/main/java/org/jclouds/profitbricks/ProfitBricksApi.java (8)
M profitbricks/src/main/java/org/jclouds/profitbricks/binder/BaseProfitBricksRequestBinder.java (2)
A profitbricks/src/main/java/org/jclouds/profitbricks/binder/server/CreateServerRequestBinder.java (57)
A profitbricks/src/main/java/org/jclouds/profitbricks/binder/server/UpdateServerRequestBinder.java (56)
M profitbricks/src/main/java/org/jclouds/profitbricks/compute/internal/ProvisioningStatusAware.java (2)
M profitbricks/src/main/java/org/jclouds/profitbricks/compute/internal/ProvisioningStatusPollingPredicate.java (2)
A profitbricks/src/main/java/org/jclouds/profitbricks/domain/AvailabilityZone.java (35)
A profitbricks/src/main/java/org/jclouds/profitbricks/domain/Image.java (207)
A profitbricks/src/main/java/org/jclouds/profitbricks/domain/OsType.java (30)
A profitbricks/src/main/java/org/jclouds/profitbricks/domain/Server.java (424)
A profitbricks/src/main/java/org/jclouds/profitbricks/domain/internal/ServerCommonProperties.java (49)
A profitbricks/src/main/java/org/jclouds/profitbricks/features/ImageApi.java (65)
A profitbricks/src/main/java/org/jclouds/profitbricks/features/ServerApi.java (164)
M profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/BaseProfitBricksResponseHandler.java (8)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/RequestIdOnlyResponseHandler.java (49)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/image/BaseImageResponseHandler.java (76)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/image/ImageInfoResponseHandler.java (48)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/image/ImageListResponseHandler.java (51)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/server/BaseServerResponseHandler.java (75)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/server/ServerIdOnlyResponseHandler.java (55)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/server/ServerInfoResponseHandler.java (48)
A profitbricks/src/main/java/org/jclouds/profitbricks/http/parser/server/ServerListResponseHandler.java (51)
A profitbricks/src/test/java/org/jclouds/profitbricks/binder/server/CreateServerRequestBinderTest.java (65)
A profitbricks/src/test/java/org/jclouds/profitbricks/binder/server/UpdateServerRequestBinderTest.java (64)
M profitbricks/src/test/java/org/jclouds/profitbricks/features/DataCenterApiMockTest.java (2)
A profitbricks/src/test/java/org/jclouds/profitbricks/features/ImageApiLiveTest.java (58)
A profitbricks/src/test/java/org/jclouds/profitbricks/features/ImageApiMockTest.java (114)
A profitbricks/src/test/java/org/jclouds/profitbricks/features/ServerApiLiveTest.java (139)
A profitbricks/src/test/java/org/jclouds/profitbricks/features/ServerApiMockTest.java (256)
A profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/RequestIdOnlyResponseHandlerTest.java (76)
M profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/ServiceFaultResponseHandlerTest.java (4)
M profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/datacenter/DataCenterInfoResponseHandlerTest.java (3)
A profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/image/ImageInfoResponseHandlerTest.java (65)
A profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/image/ImageListResponseHandlerTest.java (183)
A profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/server/ServerIdOnlyResponseHandlerTest.java (40)
A profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/server/ServerInfoResponseHandlerTest.java (76)
A profitbricks/src/test/java/org/jclouds/profitbricks/http/parser/server/ServerListResponseHandlerTest.java (97)
A profitbricks/src/test/resources/datacenter/datacenter-not-found.xml (17)
M profitbricks/src/test/resources/fault-404.xml (28)
A profitbricks/src/test/resources/image/image-not-found.xml (17)
A profitbricks/src/test/resources/image/image.xml (26)
A profitbricks/src/test/resources/image/images.xml (140)
A profitbricks/src/test/resources/server/server-create.xml (13)
A profitbricks/src/test/resources/server/server-delete.xml (13)
A profitbricks/src/test/resources/server/server-reset.xml (11)
A profitbricks/src/test/resources/server/server-start.xml (11)
A profitbricks/src/test/resources/server/server-stop.xml (11)
A profitbricks/src/test/resources/server/server-update.xml (13)
A profitbricks/src/test/resources/server/server.xml (57)
A profitbricks/src/test/resources/server/servers.xml (104)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/119.patch
https://github.com/jclouds/jclouds-labs/pull/119.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * 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;
> +
> +import org.jclouds.http.functions.ParseSax;
> +import static org.testng.Assert.assertEquals;
> +import org.testng.annotations.Test;
> +
> +@Test( groups = "unit", testName = "RequestIdOnlyResponseHandler" )
[minor] `testName = "RequestIdOnlyResponseHandlerTest"`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22945618
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + public abstract boolean isCpuHotPlug();
> +
> + public abstract boolean isCpuHotUnPlug();
> +
> + public abstract boolean isRamHotPlug();
> +
> + public abstract boolean isRamHotUnPlug();
> +
> + public abstract boolean isNicHotPlug();
> +
> + public abstract boolean isNicHotUnPlug();
> +
> + public abstract boolean isDiscVirtioHotPlug();
> +
> + public abstract boolean isDiscVirtioHotUnPlug();
Are all properties nullable?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22943276
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Reijhanniel Jearl Campos <no...@github.com>.
> +
> + public abstract boolean isCpuHotPlug();
> +
> + public abstract boolean isCpuHotUnPlug();
> +
> + public abstract boolean isRamHotPlug();
> +
> + public abstract boolean isRamHotUnPlug();
> +
> + public abstract boolean isNicHotPlug();
> +
> + public abstract boolean isNicHotUnPlug();
> +
> + public abstract boolean isDiscVirtioHotPlug();
> +
> + public abstract boolean isDiscVirtioHotUnPlug();
So far, the [API](https://www.profitbricks.com/apidoc/1_3/APIDocumentation.html?GetAllImages.html) is always returning values for those properties and it's docs don't say that they're nullable. I think it's safe to assume that they're not.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22982121
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
> + HDD, CDROM, UNRECOGNIZED;
> +
> + public static Type fromValue(String v) {
> + try {
> + return valueOf(v);
> + } catch (IllegalArgumentException ex) {
> + return UNRECOGNIZED;
> + }
> + }
> + }
> +
> + public abstract String id();
> +
> + public abstract String name();
> +
> + public abstract Float size(); // MB
Use the primitive type for non `@Nullable` variables.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22943139
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Reijhanniel Jearl Campos <no...@github.com>.
> +import org.jclouds.rest.annotations.XMLResponseParser;
> +
> +@RequestFilters({BasicAuthentication.class, ProfitBricksSoapMessageEnvelope.class})
> +@Consumes(MediaType.TEXT_XML)
> +@Produces(MediaType.TEXT_XML)
> +public interface ImageApi {
> +
> + /**
> + * @return Outputs a list of all HDD and/or CD-ROM/DVD images existing on or uploaded to the ProfitBricks FTP server.
> + */
> + @POST
> + @Named("image:getall")
> + @Payload("<ws:getAllImages/>")
> + @XMLResponseParser(ImageListResponseHandler.class)
> + @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
> + List<Image> getAllImages();
Nah it doesn't. Returns a <i>reaaally</i> large response containing all of the available images.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22982319
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
Thanks! Most comments are minors.
Given [this comment](https://github.com/jclouds/jclouds-labs/pull/119#discussion_r22944097), though, I realized that the mock tests are not verifying that the requests we generate have the expected body. This passed unnoticed in the previous review, but we should add those checks too.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119#issuecomment-69938287
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Reijhanniel Jearl Campos <no...@github.com>.
> +import org.jclouds.rest.annotations.Payload;
> +import org.jclouds.rest.annotations.PayloadParam;
> +import org.jclouds.rest.annotations.RequestFilters;
> +import org.jclouds.rest.annotations.XMLResponseParser;
> +
> +@RequestFilters( { BasicAuthentication.class, ProfitBricksSoapMessageEnvelope.class } )
> +@Consumes( MediaType.TEXT_XML )
> +@Produces( MediaType.TEXT_XML )
> +public interface ServerApi {
> +
> + /**
> + * @return Returns information about all virtual server, such as configuration, provisioning status, power status, etc.
> + */
> + @POST
> + @Named( "server:getall" )
> + @Payload( "<ws:getAllDataCenters/>" )
Woah nice catch. Will add tests to verify correct requests.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22981801
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Reijhanniel Jearl Campos <no...@github.com>.
Hi @nacx . I addressed the comments and is now ready for another review cycle. :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119#issuecomment-70445219
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
> + return valueOf( v );
> + } catch ( IllegalArgumentException ex ) {
> + return UNRECOGNIZED;
> + }
> + }
> + }
> +
> + @Nullable
> + public abstract String id();
> +
> + @Nullable
> + @Override
> + public abstract String name();
> +
> + @Nullable
> + public abstract boolean hasInternetAccess();
A primitive type can't be null.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22943980
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.rest.annotations.XMLResponseParser;
> +
> +@RequestFilters({BasicAuthentication.class, ProfitBricksSoapMessageEnvelope.class})
> +@Consumes(MediaType.TEXT_XML)
> +@Produces(MediaType.TEXT_XML)
> +public interface ImageApi {
> +
> + /**
> + * @return Outputs a list of all HDD and/or CD-ROM/DVD images existing on or uploaded to the ProfitBricks FTP server.
> + */
> + @POST
> + @Named("image:getall")
> + @Payload("<ws:getAllImages/>")
> + @XMLResponseParser(ImageListResponseHandler.class)
> + @Fallback(Fallbacks.EmptyListOnNotFoundOr404.class)
> + List<Image> getAllImages();
Does the Profitbricks API return paginated results?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22944957
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
Really nice test coverage @devcsrj. Thanks for the effort! Mind squashing the commits so I can merge it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119#issuecomment-70999626
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [3b6ddf0](https://git-wip-us.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=3b6ddf0d74bb7f4d5b7376b6e7f00b7336b139e8). Thanks @devcsrj!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119#issuecomment-71441228
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
> +import org.jclouds.rest.annotations.Payload;
> +import org.jclouds.rest.annotations.PayloadParam;
> +import org.jclouds.rest.annotations.RequestFilters;
> +import org.jclouds.rest.annotations.XMLResponseParser;
> +
> +@RequestFilters( { BasicAuthentication.class, ProfitBricksSoapMessageEnvelope.class } )
> +@Consumes( MediaType.TEXT_XML )
> +@Produces( MediaType.TEXT_XML )
> +public interface ServerApi {
> +
> + /**
> + * @return Returns information about all virtual server, such as configuration, provisioning status, power status, etc.
> + */
> + @POST
> + @Named( "server:getall" )
> + @Payload( "<ws:getAllDataCenters/>" )
Is this the right payload?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22944097
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
> + @Nullable
> + public abstract AvailabilityZone availabilityZone();
> +
> + @Nullable
> + public abstract Date creationTime();
> +
> + @Nullable
> + public abstract Date lastModificationTime();
> +
> +// public abstract List<Storage> storages();
> +// public abstract List<Nic> storages();
> + public static Server create( String id, String name, int cores, int ram, boolean hasInternetAccess, ProvisioningState state,
> + Status status, OsType osType, AvailabilityZone availabilityZone, Date creationTime, Date lastModificationTime, Boolean isCpuHotPlug,
> + Boolean isRamHotPlug, Boolean isNicHotPlug, Boolean isNicHotUnPlug, Boolean isDiscVirtioHotPlug, Boolean isDiscVirtioHotUnPlug ) {
> + return new AutoValue_Server( isCpuHotPlug, isRamHotPlug, isNicHotPlug, isNicHotUnPlug, isDiscVirtioHotPlug, isDiscVirtioHotUnPlug,
> + cores, ram, id, name, hasInternetAccess, state, status, osType, availabilityZone, creationTime, lastModificationTime );
Take care with inheritance, as the order here is very important, as noted in the [google auto warnings](https://github.com/google/auto/tree/master/value#warnings). Make sure we have a test to cover the proper object construction so we don't break this accidentally by changing the implemented interface.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22943820
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -56,6 +56,8 @@ protected BaseProfitBricksRequestBinder(String paramName) {
> protected abstract String createPayload(T payload);
>
> protected String formatIfNotEmpty(String pattern, Object param) {
> + if (param == null )
> + return "";
> return Strings.isNullOrEmpty(param.toString()) ? "" : String.format(pattern, param);
[minor] You could do it in a more elegant way as:
```java
return Strings.emptyToNull(param) == null ? "" : String.format(pattern, param);
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22942932
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
Closed #119.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119#event-224541848
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Ignasi Barrera <no...@github.com>.
> + try {
> + Image image = api.getImage(id);
> + assertRequestHasCommonProperties(server.takeRequest());
> + assertNotNull(image);
> + assertEquals(image.id(), id);
> + } finally {
> + pbApi.close();
> + server.shutdown();
> + }
> +
> + }
> +
> + @Test
> + public void testGetNonExistingImage() throws Exception {
> + MockWebServer server = mockWebServer();
> + server.enqueue(new MockResponse().setResponseCode(500).setBody(payloadFromResource("/image/image-not-found.xml")));
Hmmm this test depends on the `ResponseStatusFromPayloadHttpCommandExecutorService` fixing the response code. Even if the *real* provider returns a 500, I'd just configure a 404 response to make sure the API works, without coupling it to that class.
The `ResponseStatusFromPayloadHttpCommandExecutorService` tests should take care the status is properly fixed and that should be enough.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22945387
Re: [jclouds-labs] JCLOUDS-702: JCloud ProfitBricks provider - Server
& Image API (#119)
Posted by Reijhanniel Jearl Campos <no...@github.com>.
> @@ -56,6 +56,8 @@ protected BaseProfitBricksRequestBinder(String paramName) {
> protected abstract String createPayload(T payload);
>
> protected String formatIfNotEmpty(String pattern, Object param) {
> + if (param == null )
> + return "";
> return Strings.isNullOrEmpty(param.toString()) ? "" : String.format(pattern, param);
I can't exactly use it, since the param expects a String. (Doing <code>#toString()</code> won't do on null, cast to String throws a <code>ClassCastException</code> for <code>Boolean</code>) Anyway, see the change I did instead
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/119/files#r22986163