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/13 22:13:51 UTC
[jclouds-labs-google] Adding SSD support (#58)
Adds the ability to specify a disk type when creating a disk.
Impact: users are able to use SSDs.
You can merge this Pull Request by running:
git pull https://github.com/GoogleCloudPlatform/jclouds-labs-google ssd_support
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-google/pull/58
-- Commit Summary --
* Adding SSD support
-- File Changes --
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/domain/Disk.java (31)
M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/features/DiskApi.java (50)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskApiExpectTest.java (22)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/DiskApiLiveTest.java (29)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/internal/BaseGoogleComputeEngineApiLiveTest.java (7)
M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/parse/ParseDiskTest.java (1)
M google-compute-engine/src/test/resources/disk_get.json (3)
A google-compute-engine/src/test/resources/disk_insert_ssd.json (1)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-google/pull/58.patch
https://github.com/jclouds/jclouds-labs-google/pull/58.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1493](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1493/) 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/58#issuecomment-59810142
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #158](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/158/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-58948347
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1503](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1503/) 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/58#issuecomment-59963384
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by Ignasi Barrera <no...@github.com>.
Just one last minor in the test. Mind removing that line, squashing and rebasing to the latest master? Then it will be ready to merge Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59992753
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1469](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1469/) 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/58#issuecomment-58948787
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #170](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/170/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-60004512
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #169](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/169/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59962843
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by danbroudy <no...@github.com>.
Done. Thank you for the timely feedback!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-60004487
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #166](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/166/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59809652
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1499](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1499/) 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/58#issuecomment-59865901
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #168](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/168/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59865758
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by Ignasi Barrera <no...@github.com>.
Good! Just a couple comments:
* Regarding the options object, it allows to configure the optional parameters but also the mandatory ones, such as the `name`, that are also provided as method arguments. It is unlikely to happen, but that would allow users to configure a name in the method parameter and a different one in the options. Which one should be considered in that case? A defensive approach to prevent this would be to remove those "duplicated" variables form the options object. You could then create a private inner class with all fields inside the binder that is just used to bind the values to the JSON. But it's just an idea. WDYT?
* Also, could you add a unit test for the binder? It should be pretty straightforward. You can take the [BindToJsonPayloadTest](https://github.com/jclouds/jclouds/blob/master/core/src/test/java/org/jclouds/rest/binders/BindToJsonPayloadTest.java) as an example.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59838582
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by danbroudy <no...@github.com>.
I agree that it would be worth making a DiskCreationOptions. I will look into doing that.
As for the diskType field, that was the motivation to implementing the DiskType Api in #56. The DiskType Api makes it possible to get the URI by looking at the selfLink attribute of a DiskType which you can get from calling DiskType:list or DiskType:get.
As for the last segment of the path changing, I like to assume as little as possible.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59401999
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1506](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1506/) 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/58#issuecomment-60004964
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by danbroudy <no...@github.com>.
Added DiskCreationOptions and DiskCreationBinder.
Will gladly rebase when applicable.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59809795
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by danbroudy <no...@github.com>.
Removed duplicated variables in options object and wrote binder unit test.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59865712
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @danbroudy! Just a couple questions:
Regarding the `createDisk` methods, now there are several different methods to create a disk. Would it make sense to just leave two methods:
```java
Operation createInZone(String diskName, int sizeGb, String zone);
Operation createInZone(String diskName, int sizeGb, String zone, DiskCreationOptions options);
```
And encapsulate the image, type, and other parameters in the `options` object? That may require to create a custom binder instead of using the default MapBinder, but could be an option to simplify the API. It's just asking if this could make sense here, as other jclouds APIs use this options pattern for similar use cases.
And regarding the `diskType` field, it is an URI. How can a user get that URI? I've seen in the json test file that it is relative to the project, so it can't be hardcoded. Does the last segment of the path change? If it is fixed, could we have an enum with the possible values instead, and compute the right URI at runtime, so jclouds users just have to pick a type from the enum, and forget about the URIs?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59377423
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by Ignasi Barrera <no...@github.com>.
> +import javax.inject.Inject;
> +
> +import org.jclouds.googlecomputeengine.options.DiskCreationOptions;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.rest.MapBinder;
> +import org.jclouds.rest.binders.BindToJsonPayload;
> +
> +public class DiskCreationBinder implements MapBinder {
> +
> + @Inject
> + private BindToJsonPayload jsonBinder;
> +
> + public DiskCreationBinder SetBinder(BindToJsonPayload jsonBinder){
> + this.jsonBinder = jsonBinder;
> + return this;
> + }
Instead of having the `jsonBinder` variable and exposing this public method, would it make sense to directly extend the `BindToJsonPayload` class?
This way we can get rid of this variable and public setter, and we should only have to implement the method that accepts the `Map<String, Object>`; the other one could be removed, as the `BindToJsonPayload` already implements it the right way.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58/files#r19136914
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-google #1497](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-google/1497/) 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/58#issuecomment-59862887
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-google-pull-requests #167](https://jclouds.ci.cloudbees.com/job/jclouds-labs-google-pull-requests/167/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59862473
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by danbroudy <no...@github.com>.
I think the defensive approach is a better one. The way it is now it would overwrite with the method parameter at the time of binding. It was also based on the way it was done in the FirewallOptions/FirewallBinder/FirewallApi flow.
I will get right on the unit test. I guess I missed that test because the other binders are not tested.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58#issuecomment-59845405
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by danbroudy <no...@github.com>.
> +import javax.inject.Inject;
> +
> +import org.jclouds.googlecomputeengine.options.DiskCreationOptions;
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.rest.MapBinder;
> +import org.jclouds.rest.binders.BindToJsonPayload;
> +
> +public class DiskCreationBinder implements MapBinder {
> +
> + @Inject
> + private BindToJsonPayload jsonBinder;
> +
> + public DiskCreationBinder SetBinder(BindToJsonPayload jsonBinder){
> + this.jsonBinder = jsonBinder;
> + return this;
> + }
That is a much better idea.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58/files#r19162758
Re: [jclouds-labs-google] Adding SSD support (#58)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + private static final String FAKE_SOURCE_IMAGE = "https://www.googleapis.com/compute/v1/projects/" +
> + "debian-cloud/global/images/backports-debian-7-wheezy-v20141017";
> +
> + Json json = new GsonWrapper(new Gson());
> +
> + @Test
> + public void testMap() throws SecurityException, NoSuchMethodException {
> + DiskCreationBinder binder = new DiskCreationBinder(json);
> + DiskCreationOptions diskCreationOptions = new DiskCreationOptions().sourceImage(URI.create(FAKE_SOURCE_IMAGE));
> +
> + HttpRequest request = HttpRequest.builder().method("GET").endpoint("http://momma").build();
> + Map<String, Object> postParams = ImmutableMap.of("name", "testName", "sizeGb", 15, "options", diskCreationOptions);
> +
> + HttpRequest newRequest = binder.bindToRequest(request, postParams);
> + System.out.println(newRequest.getPayload().getRawContent());
Remove the `System.out` line.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/58/files#r19176426