Re: [jclouds] JCLOUDS-492 (#318)

[Everett Toews]

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)

[jaiganeshm]

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
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
> .
>

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37970184

Re: [jclouds] JCLOUDS-492 (#318)

[Andrew Phillips]

> +
> +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)

[Andrew Phillips]

> + *   Options to specify metadata such as architecture or 
> secondary volumes to be
> + *   associated with this image.
> + * @return imageId
> + *
> + * @see #describeImages
> + * @see #deregisterImage
> + * @see  + *  
> "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)

[Andrew Phillips]

> @@ -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)

[Andrew Phillips]

> @@ -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)

[Andrew Phillips]

> @@ -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)

[Andrew Phillips]

> @@ -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)

[Andrew Phillips]

> 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)

[jaiganeshm]

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 wrote:

> Thanks @jaiganeshm ! It seems an spurious
> test failure.
>
> --
> Reply to this email directly or view it on 
> GitHub
> .
>

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/318#issuecomment-37840950

Re: [jclouds] JCLOUDS-492 (#318)

[Ignasi Barrera]

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)

[Ignasi Barrera]

> @@ -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)

[Ignasi Barrera]

> @@ -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)

[Ignasi Barrera]

> @@ -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)

[CloudBees pull request builder plugin]

[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

Re: [jclouds] JCLOUDS-492 (#318)

[CloudBees pull request builder plugin]

[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)

[BuildHive]

[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)

[CloudBees pull request builder plugin]

[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)

[CloudBees pull request builder plugin]

[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)

[BuildHive]

[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)

[CloudBees pull request builder plugin]

[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)

[CloudBees pull request builder plugin]

[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)

[BuildHive]

[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