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/07/12 20:24:06 UTC

[jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

@baldwinspc @nacx 
Image api PR ready for review, i have applied all the comments from the previous PR so this one should more ready to merge.

Thanks.
You can view, comment on, or merge this pull request online at:

  https://github.com/jclouds/jclouds-labs/pull/299

-- Commit Summary --

  * JCLOUDS-1138 oneandone-image-api

-- File Changes --

    M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/OneAndOneApi.java (7)
    M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/DataCenter.java (15)
    M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/Dvd.java (13)
    M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/FixedInstanceHardware.java (1)
    M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/Hardware.java (1)
    M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/HardwareFlavour.java (1)
    M oneandone/src/main/java/org/apache/jclouds/oneandone/rest/domain/Image.java (71)
    A oneandone/src/main/java/org/apache/jclouds/oneandone/rest/features/ImageApi.java (77)
    A oneandone/src/test/java/org/apache/jclouds/oneandone/rest/features/ImageApiLiveTest.java (109)
    A oneandone/src/test/java/org/apache/jclouds/oneandone/rest/features/ImageApiMockTest.java (189)
    M oneandone/src/test/java/org/apache/jclouds/oneandone/rest/internal/BaseOneAndOneApiMockTest.java (1)
    A oneandone/src/test/resources/image/get.json (31)
    A oneandone/src/test/resources/image/list.json (63)
    A oneandone/src/test/resources/image/list.options.json (33)
    M pom.xml (1)

-- Patch Links --

https://github.com/jclouds/jclouds-labs/pull/299.patch
https://github.com/jclouds/jclouds-labs/pull/299.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/299

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by Ignasi Barrera <no...@github.com>.
> @@ -38,6 +38,7 @@
>  
>    <properties>
>      <jclouds.version>2.0.0-SNAPSHOT</jclouds.version>
> +    <checkstyle.skip>true</checkstyle.skip>

Remove this :)

---
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/299/files/16efc99645240d8683449a3f2f77254ad0e91a1b#r70532050

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by Ignasi Barrera <no...@github.com>.
Pushed to master as [f997b061](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/f997b061). Apologies for the delay @alibazlamit. I missed the notification for your previous comment. Thanks for pinging back! :)

---
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/299#issuecomment-234476953

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by Ignasi Barrera <no...@github.com>.
>  import java.io.Closeable;
>  import org.apache.jclouds.oneandone.rest.features.ImageApi;
>  import org.apache.jclouds.oneandone.rest.features.ServerApi;
>  import org.jclouds.rest.annotations.Delegate;
>  
> -@Beta

You can keep this, but taking care of using the right import. Just `com.google.common.common.annotations.Beta`.

---
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/299/files/bf8d8c4370014ca206a6b4a6e1479e13f0ca7c1c..80c2fc4d9375cb9061507009887c338d2a517f31#r70707068

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by alibazlamit <no...@github.com>.
Changes 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/299#issuecomment-232483182

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by alibazlamit <no...@github.com>.
>  import java.io.Closeable;
>  import org.apache.jclouds.oneandone.rest.features.ImageApi;
>  import org.apache.jclouds.oneandone.rest.features.ServerApi;
>  import org.jclouds.rest.annotations.Delegate;
>  
> -@Beta

What is it used for, do we really need 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/299/files/bf8d8c4370014ca206a6b4a6e1479e13f0ca7c1c..80c2fc4d9375cb9061507009887c338d2a517f31#r70708370

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by alibazlamit <no...@github.com>.
@alibazlamit pushed 1 commit.

80c2fc4  Removed chekstyle error


---
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/299/files/bf8d8c4370014ca206a6b4a6e1479e13f0ca7c1c..80c2fc4d9375cb9061507009887c338d2a517f31

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by Ignasi Barrera <no...@github.com>.
Closed #299.

---
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/299#event-731422105

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by Ignasi Barrera <no...@github.com>.
> +      assertFalse(images.isEmpty());
> +      Assert.assertTrue(images.size() > 0);
> +   }
> +
> +   @Test(dependsOnMethods = "testList")
> +   public void testListWithOption() {
> +      GenericQueryOptions options = new GenericQueryOptions();
> +      options.options(0, 0, null, "jcloudsimage", null);
> +      List<Image> imageWithQuery = imageApi().list(options);
> +
> +      assertNotNull(imageWithQuery);
> +      assertFalse(imageWithQuery.isEmpty());
> +      Assert.assertTrue(imageWithQuery.size() > 0);
> +   }
> +
> +   @Test(dependsOnMethods = "testListWithOption")

Same here. There is no need for the dependency between tests. Apply the same criteria to the rest of the tests.

---
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/299/files/16efc99645240d8683449a3f2f77254ad0e91a1b#r70531728

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by Ignasi Barrera <no...@github.com>.
> +      assertNodeAvailable(currentServer);
> +      currentImage = imageApi().createImage(CreateImage.builder()
> +              .name("jcloudsimage")
> +              .numImages(1)
> +              .frequency(Types.ImageFrequency.WEEKLY)
> +              .serverId(currentServer.id())
> +              .build());
> +   }
> +
> +   @Test
> +   public void testList() {
> +      images = imageApi().list();
> +
> +      assertNotNull(images);
> +      assertFalse(images.isEmpty());
> +      Assert.assertTrue(images.size() > 0);

Better assert that the image we created exists, to make tests more robust.

---
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/299/files/16efc99645240d8683449a3f2f77254ad0e91a1b#r70531543

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by Ignasi Barrera <no...@github.com>.
This looks great @alibazlamit! Just minor comments. Could you also re-indent the code to 3 spaces?

---
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/299#issuecomment-232198022

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by Ignasi Barrera <no...@github.com>.
> +              .numImages(1)
> +              .frequency(Types.ImageFrequency.WEEKLY)
> +              .serverId(currentServer.id())
> +              .build());
> +   }
> +
> +   @Test
> +   public void testList() {
> +      images = imageApi().list();
> +
> +      assertNotNull(images);
> +      assertFalse(images.isEmpty());
> +      Assert.assertTrue(images.size() > 0);
> +   }
> +
> +   @Test(dependsOnMethods = "testList")

There is no need for this test to depend on the other one. Let's try to avoid skipping tests when there are unexpected failures.

---
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/299/files/16efc99645240d8683449a3f2f77254ad0e91a1b#r70531659

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by Ignasi Barrera <no...@github.com>.
Thanks! Mind squashing the commits so I can cleanly merge 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/299#issuecomment-232488330

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by alibazlamit <no...@github.com>.
@nacx Can we move on with merging this please.

---
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/299#issuecomment-234396328

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by alibazlamit <no...@github.com>.
@alibazlamit pushed 1 commit.

7f49123  Changes done


---
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/299/files/16efc99645240d8683449a3f2f77254ad0e91a1b..7f491232f8c7ebc807edd9fe4326f2ae83e66432

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by alibazlamit <no...@github.com>.
Done, Thanks man.

---
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/299#issuecomment-232492049

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by alibazlamit <no...@github.com>.
@alibazlamit pushed 1 commit.

bf8d8c4  removed checkstyle skip


---
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/299/files/7f491232f8c7ebc807edd9fe4326f2ae83e66432..bf8d8c4370014ca206a6b4a6e1479e13f0ca7c1c

Re: [jclouds/jclouds-labs] JCLOUDS-1138 oneandone-image-api (#299)

Posted by Ignasi Barrera <no...@github.com>.
>  import java.io.Closeable;
>  import org.apache.jclouds.oneandone.rest.features.ImageApi;
>  import org.apache.jclouds.oneandone.rest.features.ServerApi;
>  import org.jclouds.rest.annotations.Delegate;
>  
> -@Beta

No need to keep it; just commented in case you just removed it to make checkstyle happy :)
It is just an annotation to give information that the api is still in an early stage (which is redundant for providers in labs). There is no need to keep 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/299/files/bf8d8c4370014ca206a6b4a6e1479e13f0ca7c1c..80c2fc4d9375cb9061507009887c338d2a517f31#r70709755