You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by danbroudy <no...@github.com> on 2014/10/15 22:51:22 UTC
[jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Users can now create images from persistent disks.
When images are made from PD the rawDisk attribute is not present.
This changes Image.java to treat it as an Optional<RawDisk> instead.
You can merge this Pull Request by running:
git pull https://github.com/GoogleCloudPlatform/jclouds-labs-google imagecreation
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-google/pull/59
-- Commit Summary --
* JCLOUDS-649: added image creation from pd and made Image rawDisk Optional<T>
-- File Changes --
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/domain/Image.java (10)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/ImageApi.java (23)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskApiLiveTest.java (2)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ImageApiExpectTest.java (42)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/ImageApiLiveTest.java (47)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/internal/BaseGoogleComputeEngineApiLiveTest.java (6)
A google-compute-engine/src/test/resources/image_insert_from_pd.json (1)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-google/pull/59.patch
https://github.com/jclouds/jclouds-labs-google/pull/59.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1476](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1476/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59#issuecomment-59398652
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -70,5 +86,36 @@ private void assertImageEquals(Image result, Image expected) {
> assertEquals(result.getName(), expected.getName());
> }
>
> + @Test(groups = "live")
> + public void testInsertDisk() {
> + assertZoneOperationDoneSucessfully(diskApi().createInZone(DISK_NAME, sizeGb, DEFAULT_ZONE_NAME), TIME_WAIT);
> + Disk disk = diskApi().getInZone(DEFAULT_ZONE_NAME, DISK_NAME);
> + diskURI = disk.getSelfLink();
> + System.out.println("diskURI: " + diskURI.toString());
I just meant to remove the `System.out` line :) Seeing that in the output (without context) might not provide any value when running the entire test suite?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59/files#r18941854
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by danbroudy <no...@github.com>.
> + /**
> + * Creates an image resource in the specified project from the provided persistent disk.
> + *
> + * @param imageName the name of the created image
> + * @param sourceDisk fully qualified URL for the persistent disk to create the image from
> + * @return an Operation resource. To check on the status of an operation, poll the Operations resource returned to
> + * you, and look for the status field.
> + */
> + @Named("Images:insert")
> + @POST
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Produces(MediaType.APPLICATION_JSON)
> + @Path("/global/images")
> + @OAuthScopes(COMPUTE_SCOPE)
> + @MapBinder(BindToJsonPayload.class)
> + @Fallback(NullOnNotFoundOr404.class)
Sounds good to me. I was wondering how to handle these responses, I noticed that some similar methods did not have a fallback such as Disk:insert but then others did such as Disks:createSnapshot. I see that is addressed in PR#40.
I will remove the @Fallback(NullOnNotFoundOr404.class) line and the corresponding expect test
testCreateImageFromPdResponseIs4xx().
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59/files#r18929730
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by Ignasi Barrera <no...@github.com>.
> + /**
> + * Creates an image resource in the specified project from the provided persistent disk.
> + *
> + * @param imageName the name of the created image
> + * @param sourceDisk fully qualified URL for the persistent disk to create the image from
> + * @return an Operation resource. To check on the status of an operation, poll the Operations resource returned to
> + * you, and look for the status field.
> + */
> + @Named("Images:insert")
> + @POST
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Produces(MediaType.APPLICATION_JSON)
> + @Path("/global/images")
> + @OAuthScopes(COMPUTE_SCOPE)
> + @MapBinder(BindToJsonPayload.class)
> + @Fallback(NullOnNotFoundOr404.class)
This kind of fallbacks shouldn't be used in PUT or POST operations, as the returned type won't be clear (see https://github.com/jclouds/jclouds-labs-google/pull/40). *Write* operations on a resource should assume that the resource already exists or fail otherwise. Using this fallback would silently return `null`, which has no clear meaning to the user: did the operation fail? Did it complete and the response had no body? Others?
Just remove the fallback, and a `ResourceNotFoundException` will be automatically thrown by jclouds if a 404 response is returned.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59/files#r18926849
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #159](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/159/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59#issuecomment-59274346
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by danbroudy <no...@github.com>.
> @@ -70,5 +86,36 @@ private void assertImageEquals(Image result, Image expected) {
> assertEquals(result.getName(), expected.getName());
> }
>
> + @Test(groups = "live")
> + public void testInsertDisk() {
> + assertZoneOperationDoneSucessfully(diskApi().createInZone(DISK_NAME, sizeGb, DEFAULT_ZONE_NAME), TIME_WAIT);
> + Disk disk = diskApi().getInZone(DEFAULT_ZONE_NAME, DISK_NAME);
> + diskURI = disk.getSelfLink();
> + System.out.println("diskURI: " + diskURI.toString());
Woups, nice catch, ill remove that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59/files#r18968984
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by Chris Custine <no...@github.com>.
@danbroudy Yeah it is perfectly fine to change the return type here in a labs project. If we had released this before the PR, then we would have to wait until 2.0, so this worked out nicely. I don't have time to review this in detail but I'll be back with a review in a couple of hours.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59#issuecomment-59300182
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @danbroudy! Just a couple comments. Apart from that, lgtm!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59#issuecomment-59285602
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1474](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1474/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59#issuecomment-59274805
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by danbroudy <no...@github.com>.
I did have a question about how to handle backwards compatibility with Image.getRawDisk().
Is it ok that I have changed the return value from a RawDisk to an Optional<RawDisk>?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59#issuecomment-59291700
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #160](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/160/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59#issuecomment-59293469
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #161](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/161/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59#issuecomment-59398078
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1475](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1475/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59#issuecomment-59293730
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -70,5 +86,36 @@ private void assertImageEquals(Image result, Image expected) {
> assertEquals(result.getName(), expected.getName());
> }
>
> + @Test(groups = "live")
> + public void testInsertDisk() {
> + assertZoneOperationDoneSucessfully(diskApi().createInZone(DISK_NAME, sizeGb, DEFAULT_ZONE_NAME), TIME_WAIT);
> + Disk disk = diskApi().getInZone(DEFAULT_ZONE_NAME, DISK_NAME);
> + diskURI = disk.getSelfLink();
> + System.out.println("diskURI: " + diskURI.toString());
Can this be removed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59/files#r18926936
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by Chris Custine <no...@github.com>.
@danbroudy Aside from the comment from @nacx about the System.out in that test, everything LGTM. I think if you fix that, rebase and squash to a single commit, I will +1 and merge this in.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59#issuecomment-59376447
Re: [jclouds-labs-google] JCLOUDS-649: Added image creation from pd,
changed rawDisk to Optional (#59)
Posted by danbroudy <no...@github.com>.
> @@ -70,5 +86,36 @@ private void assertImageEquals(Image result, Image expected) {
> assertEquals(result.getName(), expected.getName());
> }
>
> + @Test(groups = "live")
> + public void testInsertDisk() {
> + assertZoneOperationDoneSucessfully(diskApi().createInZone(DISK_NAME, sizeGb, DEFAULT_ZONE_NAME), TIME_WAIT);
> + Disk disk = diskApi().getInZone(DEFAULT_ZONE_NAME, DISK_NAME);
> + diskURI = disk.getSelfLink();
> + System.out.println("diskURI: " + diskURI.toString());
I don't think we can get rid of it because in the tests below I need the disk to create an image from.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/59/files#r18929352