You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by jaiganeshm <no...@github.com> on 2014/03/16 23:56:41 UTC
[jclouds] JCLOUDS-492 (#318)
New API to allow users to pass root device name in the registerImage from snapshot api.
Deprecated older api which fixed the root device name and accepted multiple options objects as parameters.
You can merge this Pull Request by running:
git pull https://github.com/jaiganeshm/jclouds 1.7.x
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/318
-- Commit Summary --
* JCLOUDS-492
-- File Changes --
M apis/ec2/src/main/java/org/jclouds/ec2/features/AMIApi.java (73)
M apis/ec2/src/main/java/org/jclouds/ec2/options/RegisterImageBackedByEbsOptions.java (23)
M apis/ec2/src/test/java/org/jclouds/ec2/features/AMIApiLiveTest.java (2)
M apis/ec2/src/test/java/org/jclouds/ec2/features/AMIApiTest.java (75)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/318.patch
https://github.com/jclouds/jclouds/pull/318.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318
Re: [jclouds] JCLOUDS-492 (#318)
Posted by jaiganeshm <no...@github.com>.
Thanks Andrew for the clarification. That is a relief to know that the main
repo is not touched :)
Rgds
Jai
On Tue, Mar 18, 2014 at 9:54 AM, Andrew Phillips
<no...@github.com>wrote:
> The test failures was a bummer. I thought a commit to Github will only
> update my fork branch and I can send it for a review.
>
> You don't have to worry about the test failures. The Jenkins builds are
> automatically adding your diff to a copy of the code and running tests -
> nothing is changed in the main repo.
>
> You can, of course, always run tests locally, but you don't need to be
> concerned about failures here ;-)
>
> --
> Reply to this email directly or view it on GitHub<https://github.com/jclouds/jclouds/pull/318#issuecomment-37957698>
> .
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37970184
Re: [jclouds] JCLOUDS-492 (#318)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #665](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/665/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37780973
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Everett Toews <no...@github.com>.
It's release time in jclouds and that means we like to do a little house cleaning by sweeping out the pull request queue. This PR hasn't been touched in over 6 months. Please give us a status update.
If we don't hear anything by the end of the week, we'll close this pull request.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-59872211
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Andrew Phillips <no...@github.com>.
> @@ -130,6 +135,22 @@ public RegisterImageBackedByEbsOptions addNewBlockDevice(String deviceName,
> return this;
> }
>
> + /**
> + *
> + * sets the ROOT device name to the specified name.
"Sets" (capital 'S"), and please remove the blank line above. I know "adds" above is also lowercase, but we don't have to continue that trend ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318/files#r10712949
Re: [jclouds] JCLOUDS-492 (#318)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #919](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/919/) 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/pull/318#issuecomment-37780775
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Andrew Phillips <no...@github.com>.
> @@ -130,6 +135,22 @@ public RegisterImageBackedByEbsOptions addNewBlockDevice(String deviceName,
> return this;
> }
>
> + /**
> + *
> + * sets the ROOT device name to the specified name.
> + *
> + * @param name
> + * The device name (e.g., /dev/sdh).
> + *
> + */
> + public RegisterImageBackedByEbsOptions setRootDeviceName(String deviceName) {
> + formParameters.removeAll("BlockDeviceMapping.0.DeviceName");
> + formParameters.removeAll("RootDeviceName");
Agree with @nacx's comment above - we don't really want setters removing items.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318/files#r10715428
Re: [jclouds] JCLOUDS-492 (#318)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1133](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1133/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37776263
Re: [jclouds] JCLOUDS-492 (#318)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #664](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/664/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37779436
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -44,15 +35,17 @@
> import org.jclouds.ec2.xml.PermissionHandler;
> import org.jclouds.javax.annotation.Nullable;
> import org.jclouds.location.functions.RegionToEndpointOrProviderIfNull;
> -import org.jclouds.rest.annotations.BinderParam;
> -import org.jclouds.rest.annotations.EndpointParam;
> -import org.jclouds.rest.annotations.Fallback;
> -import org.jclouds.rest.annotations.FormParams;
> -import org.jclouds.rest.annotations.RequestFilters;
> -import org.jclouds.rest.annotations.VirtualHost;
> -import org.jclouds.rest.annotations.XMLResponseParser;
> +import org.jclouds.rest.annotations.*;
Don't use wildcard imports
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318/files#r10648322
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Andrew Phillips <no...@github.com>.
> +
> + request = (GeneratedHttpRequest) request.getFilters().get(0).filter(request);
> +
> + assertRequestLineEquals(request, "POST https://ec2.us-east-1.amazonaws.com/ HTTP/1.1");
> + assertNonPayloadHeadersEqual(request, "Host: ec2.us-east-1.amazonaws.com\n");
> + assertPayloadEquals(request, actualRegisterImgBackedByEBSOptions.getPayload().getRawContent().toString(),
> + "application/x-www-form-urlencoded", false);
> +
> + assertResponseParserClassEquals(method, request, ParseSax.class);
> + assertSaxResponseParserClassEquals(method, ImageIdHandler.class);
> + assertFallbackClassEquals(method, null);
> +
> + checkFilters(request);
> + }
> +
> + HttpRequest getBlockDeviceMappingsForImage = HttpRequest.builder().method("POST")
Indents are incorrect here - jclouds uses a 3-space indent
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318/files#r10715507
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Andrew Phillips <no...@github.com>.
> + * Options to specify metadata such as architecture or secondary volumes to be
> + * associated with this image.
> + * @return imageId
> + *
> + * @see #describeImages
> + * @see #deregisterImage
> + * @see <a href=
> + * "http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/ApiReference-query-RegisterImage.html"
> + * />
> + */
> + @Named("RegisterImageWithOptions")
> + @POST
> + @Path("/")
> + @FormParams(keys = { ACTION }, values = { "RegisterImage"})
> + @XMLResponseParser(ImageIdHandler.class)
> + String registerUnixImgBackedByEbsInRegion(
Rename to registerUnixImageBackedByEbsInRegion - see @nacx's comment above
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318/files#r10715467
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -241,6 +234,7 @@ String registerImageFromManifestInRegion(
> * "http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/ApiReference-query-RegisterImage.html"
> * />
> */
> + @Deprecated
Instead of deprecating this method, just remove the `options` parameter from it (and remove the deprecation annotation). Existing integrations won't break, as you are providing the new method with the options parameter, and we don't allow more than one `HttpRequestOptions` subclass parameter in methods (there is a runtime check for that), so we already know no one is actually using the varargs parameter with more than one value.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318/files#r10648372
Re: [jclouds] JCLOUDS-492 (#318)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #663](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/663/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37776120
Re: [jclouds] JCLOUDS-492 (#318)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1134](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1134/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37779587
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @jaiganeshm! It seems an spurious test failure.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37794070
Re: [jclouds] JCLOUDS-492 (#318)
Posted by jaiganeshm <no...@github.com>.
Hi Ignasi,
Thanks for there review comments. I will work on incorporating them.
The test failures was a bummer. I thought a commit to Github will only
update my fork branch and I can send it for a review.
Is there a way to just hold it locally in github account and get it
reviewed before it gets part of the build ?
Rgds
Jai
On Mon, Mar 17, 2014 at 1:42 AM, Ignasi Barrera <no...@github.com>wrote:
> Thanks @jaiganeshm <https://github.com/jaiganeshm>! It seems an spurious
> test failure.
>
> --
> Reply to this email directly or view it on GitHub<https://github.com/jclouds/jclouds/pull/318#issuecomment-37794070>
> .
>
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37840950
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -44,7 +44,12 @@
>
> private int deviceIndex = 1;
>
> - /**
> + public RegisterImageBackedByEbsOptions() {
> + formParameters.put("BlockDeviceMapping.0.DeviceName", "/dev/sda1");
> + formParameters.put("RootDeviceName","/dev/sda1");
Can we remove this from here and move the logic to the build() method? This way we can avoid removing the entries later if user specifies them.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318/files#r10648389
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Andrew Phillips <no...@github.com>.
> @@ -130,6 +135,22 @@ public RegisterImageBackedByEbsOptions addNewBlockDevice(String deviceName,
> return this;
> }
>
> + /**
> + *
> + * sets the ROOT device name to the specified name.
> + *
> + * @param name
> + * The device name (e.g., /dev/sdh).
Simply "The device name, e.g. /dev/sdh"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318/files#r10712968
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Andrew Phillips <no...@github.com>.
> @@ -44,7 +44,12 @@
>
> private int deviceIndex = 1;
>
> - /**
> + public RegisterImageBackedByEbsOptions() {
> + formParameters.put("BlockDeviceMapping.0.DeviceName", "/dev/sda1");
> + formParameters.put("RootDeviceName","/dev/sda1");
And a minor comment: missing space before `"/dev/sda1"`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318/files#r10712892
Re: [jclouds] JCLOUDS-492 (#318)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #917](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/917/) UNSTABLE
Looks like there's a problem with this pull request
[(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/pull/318#issuecomment-37775669
Re: [jclouds] JCLOUDS-492 (#318)
Posted by Andrew Phillips <no...@github.com>.
> The test failures was a bummer. I thought a commit to Github will only
> update my fork branch and I can send it for a review.
You don't have to worry about the test failures. The Jenkins builds are automatically adding your diff to a copy of the code and running tests - nothing is changed in the main repo.
You can, of course, always run tests locally, but you don't need to be concerned about failures here ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37957698
Re: [jclouds] JCLOUDS-492 (#318)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #918](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/918/) UNSTABLE
Looks like there's a problem with this pull request
[(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/pull/318#issuecomment-37779119
Re: [jclouds] JCLOUDS-492 (#318)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1135](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1135/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37781144