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/02/29 12:14:03 UTC
[jclouds-labs] Profitbricks REST - Image API (#242)
@jasminSPC
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/242
-- Commit Summary --
* Profitbricks REST - Image 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/image/UpdateImageRequestBinder.java (98)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/domain/Image.java (81)
M profitbricks-rest/src/main/java/org/apache/jclouds/profitbricks/rest/features/ImageApi.java (58)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/binder/image/UpdateImageRequestBinderTest.java (65)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/ImageApiLiveTest.java (55)
A profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/ImageApiMockTest.java (95)
A profitbricks-rest/src/test/resources/image/get.json (32)
A profitbricks-rest/src/test/resources/image/list.json (423)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/242.patch
https://github.com/jclouds/jclouds-labs/pull/242.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Reijhanniel Jearl Campos <no...@github.com>.
> + assertEquals(image.id(), testImageId);
> + assertEquals(image.properties().name(), "ubuntu-14.04.3-server-amd64.iso");
> + }
> +
> + @Test
> + public void testList() {
> + List<Image> images = imageApi().getList();
> +
> + assertNotNull(images);
> + assertFalse(images.isEmpty());
> + assertTrue(images.size() > 1);
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
He might be pertaining to methods with the `DepthOptions` param (getList, getImage). :)
About the *update* and *delete* operations: Since the PB API doesn't have an endpoint to actually upload images (was this via FTP, IIRC?), would it make sense if we just remove these operations, and just leave the `GET` requests to keep it simple? This is the case for the [soap counterpart](https://github.com/jclouds/jclouds/blob/master/providers/profitbricks/src/main/java/org/jclouds/profitbricks/features/ImageApi.java#L42). Although I'm not aware as to how many users would actually depend on this feature, so... thoughts?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54403984
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Ignasi Barrera <no...@github.com>.
Closed #242.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242#event-575385879
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Test
> + public void testDelete() throws InterruptedException {
> + server.enqueue(
> + new MockResponse().setBody("")
> + );
> +
> + imageApi().deleteImage("some-id");
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "DELETE", "/images/some-id");
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
To avoid confusion, jclouds provides a set of default fallbacks that will be executed if no fallback is defined on a method (typically these fallbacks throw an appropriate exception).
* There should be **no tests** for fallbacks in methods that don't explicitly declare a fallback, as we don't want to test what jclouds does by default; it is already tested in jclouds core.
* There **must** be tests for fallbacks in those methods where you explicitly declare one, as that method won't use the jclouds default and we want to make sure the right fallback takes place upon failed requests.
Rule of thumb is: if your method has a `@Fallback` annotation, it needs a test. Otherwise, fallback test is not needed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54411128
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Test
> + public void testDelete() throws InterruptedException {
> + server.enqueue(
> + new MockResponse().setBody("")
> + );
> +
> + imageApi().deleteImage("some-id");
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "DELETE", "/images/some-id");
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
Add the missing tests for all the methods in the image api, and tests for the fallbacks for those methods that define them.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54398374
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by jasminSPC <no...@github.com>.
> + assertEquals(image.id(), testImageId);
> + assertEquals(image.properties().name(), "ubuntu-14.04.3-server-amd64.iso");
> + }
> +
> + @Test
> + public void testList() {
> + List<Image> images = imageApi().getList();
> +
> + assertNotNull(images);
> + assertFalse(images.isEmpty());
> + assertTrue(images.size() > 1);
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
https://devops.profitbricks.com/api/rest/#images
Update and delete can be done only on private images, and to create private you have to upload them using PB control panel. This wouldn't make sense in Live tests
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54401247
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Reijhanniel Jearl Campos <no...@github.com>.
> + }
> +
> + @Test
> + public void testDelete() throws InterruptedException {
> + server.enqueue(
> + new MockResponse().setBody("")
> + );
> +
> + imageApi().deleteImage("some-id");
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "DELETE", "/images/some-id");
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
Hi @mirza-spc ! You're right, there's no need to test for 404 fallbacks. :)
In this case, @nacx might've meant, the tests for the methods with `DepthOptions` param, like for getList and getImage.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54403419
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by mirza-spc <no...@github.com>.
> + assertEquals(image.id(), testImageId);
> + assertEquals(image.properties().name(), "ubuntu-14.04.3-server-amd64.iso");
> + }
> +
> + @Test
> + public void testList() {
> + List<Image> images = imageApi().getList();
> +
> + assertNotNull(images);
> + assertFalse(images.isEmpty());
> + assertTrue(images.size() > 1);
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
Remaining methods are Update and Delete. I'm not quite sure if PB has a test image for that purpose? Esp. since there is no Create call.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54400641
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by jasminSPC <no...@github.com>.
> + assertEquals(image.id(), testImageId);
> + assertEquals(image.properties().name(), "ubuntu-14.04.3-server-amd64.iso");
> + }
> +
> + @Test
> + public void testList() {
> + List<Image> images = imageApi().getList();
> +
> + assertNotNull(images);
> + assertFalse(images.isEmpty());
> + assertTrue(images.size() > 1);
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
That makes sense
Jasmin Gacić
On Feb 29, 2016 13:36, "Reijhanniel Jearl Campos" <no...@github.com>
wrote:
> In
> profitbricks-rest/src/test/java/org/apache/jclouds/profitbricks/rest/features/ImageApiLiveTest.java
> <https://github.com/jclouds/jclouds-labs/pull/242#discussion_r54403984>:
>
> > + assertEquals(image.id(), testImageId);
> > + assertEquals(image.properties().name(), "ubuntu-14.04.3-server-amd64.iso");
> > + }
> > +
> > + @Test
> > + public void testList() {
> > + List<Image> images = imageApi().getList();
> > +
> > + assertNotNull(images);
> > + assertFalse(images.isEmpty());
> > + assertTrue(images.size() > 1);
> > + }
> > +
> > + private ImageApi imageApi() {
> > + return api.imageApi();
> > + }
>
> He might be pertaining to methods with the DepthOptions param (getList,
> getImage). :)
>
> About the *update* and *delete* operations: Since the PB API doesn't have
> an endpoint to actually upload images (was this via FTP, IIRC?), would it
> make sense if we just remove these operations, and just leave the GET
> requests to keep it simple? This is the case for the soap counterpart
> <https://github.com/jclouds/jclouds/blob/master/providers/profitbricks/src/main/java/org/jclouds/profitbricks/features/ImageApi.java#L42>.
> Although I'm not aware as to how many users would actually depend on this
> feature, so... thoughts?
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/jclouds/jclouds-labs/pull/242/files#r54403984>.
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54406011
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Reijhanniel Jearl Campos <no...@github.com>.
I have no numbers to determine how many users are currently using/will use those image UD operations via jclouds (if this argument make sense).
If this were a vote, I'd say remove the update and delete methods, because there's no way we can write an *acceptable* live test for 'em anyway. Even if a REST endpoint for upload image is added, I don't think waiting for the upload live test of a [50MB](http://www.damnsmalllinux.org/) image (smallest I can find) is feasible. :smile:
With that said, now that the missing mock tests were added, this already LGTM, if you decide to just keep only getImage/s operations.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242#issuecomment-190767165
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by mirza-spc <no...@github.com>.
I've added the missing tests for mock tests. I guess we still haven't decided the course for the live tests?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242#issuecomment-190599931
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by mirza-spc <no...@github.com>.
> + }
> +
> + @Test
> + public void testDelete() throws InterruptedException {
> + server.enqueue(
> + new MockResponse().setBody("")
> + );
> +
> + imageApi().deleteImage("some-id");
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "DELETE", "/images/some-id");
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
All the methods in the Image API? As far as I can see they are already there List, Get, Update, Delete. By fallbacks do you mean 404s? Because @devcsrj commented
`You may remove the test for 404 fallback, as this is already tested in the core.
Add a line note
`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54401257
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Ignasi Barrera <no...@github.com>.
I'd say remove them too. The primary objective here should be to add the minimum required methods to make it possible to implement the ComputeService interface.
Any other methods are low priority for jclouds, as we are all about cloud portability. What will make this a first class provider in jclouds is not implementing the entire ProfitBricks API, but properly implementing the jclouds interfaces, and that should be the focus. Let's forget about anything that doesn't help reach that goal until the provider is out of labs.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242#issuecomment-190821592
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [f588d6b6](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/f588d6b6). Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242#issuecomment-191455574
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Ignasi Barrera <no...@github.com>.
> + assertEquals(image.id(), testImageId);
> + assertEquals(image.properties().name(), "ubuntu-14.04.3-server-amd64.iso");
> + }
> +
> + @Test
> + public void testList() {
> + List<Image> images = imageApi().getList();
> +
> + assertNotNull(images);
> + assertFalse(images.isEmpty());
> + assertTrue(images.size() > 1);
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
Add live tests for the other methods in the image api
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54398330
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Ignasi Barrera <no...@github.com>.
lgtm. Thanks! Can you squash the commits into a single one to prepare the merge?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242#issuecomment-191279810
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Reijhanniel Jearl Campos <no...@github.com>.
> + }
> +
> + @Test
> + public void testDelete() throws InterruptedException {
> + server.enqueue(
> + new MockResponse().setBody("")
> + );
> +
> + imageApi().deleteImage("some-id");
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "DELETE", "/images/some-id");
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
> Rule of thumb is: if your method has a @Fallback annotation, it needs a test. Otherwise, fallback test is not needed.
+1.
So in this case: a 404/401 test won't be needed for `updateImage`, because it doesn't have a declared fallback (and jclouds will propagate an appropriate exception), but we need one for `getImage`, so that we can verify that the method's behavior indeed returns an empty list, instead of a `ResourceNotFoundException`/`AuthorizationException`.
I got confused with this as well. Sorry for the confusion, @mirza-spc !
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54416331
Re: [jclouds-labs] Profitbricks REST - Image API (#242)
Posted by Ignasi Barrera <no...@github.com>.
> + }
> +
> + @Test
> + public void testDelete() throws InterruptedException {
> + server.enqueue(
> + new MockResponse().setBody("")
> + );
> +
> + imageApi().deleteImage("some-id");
> + assertEquals(server.getRequestCount(), 1);
> + assertSent(server, "DELETE", "/images/some-id");
> + }
> +
> + private ImageApi imageApi() {
> + return api.imageApi();
> + }
> In this case, @nacx might've meant, the missing tests for the methods with DepthOptions param, like for getList and getImage.
That's also right. **Every** method needs a mock test.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/242/files#r54411270