You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Jeremy Daggett <no...@github.com> on 2014/06/04 18:27:28 UTC
[jclouds] Adds get() method to KeyPairApi (#391)
This PR adds the `get()` method to the OpenStack Nova Key Pair Extension API aka `os-keypairs`.
JIRA: https://issues.apache.org/jira/browse/JCLOUDS-455
You can merge this Pull Request by running:
git pull https://github.com/rackspace/jclouds fix-455
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/391
-- Commit Summary --
* JCLOUDS-455: Add get() method to KeyPairApi
-- File Changes --
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/extensions/KeyPairApi.java (8)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/extensions/KeyPairAsyncApi.java (10)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/extensions/KeyPairApiExpectTest.java (40)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/extensions/KeyPairApiLiveTest.java (26)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/parse/ParseKeyPairTest.java (5)
R apis/openstack-nova/src/test/resources/keypair_details.json (0)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/391.patch
https://github.com/jclouds/jclouds/pull/391.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Jeremy Daggett <no...@github.com>.
@nacx Thanks for the review! I have addressed all of your comments in 1e18660
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-45227870
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1232](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1232/) FAILURE
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/391#issuecomment-46205141
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1183](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1183/) 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/391#issuecomment-45121765
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -81,6 +82,15 @@
> @Payload("%7B\"keypair\":%7B\"name\":\"{name}\"%7D%7D")
> ListenableFuture<? extends KeyPair> create(@PayloadParam("name") String name);
>
> + @Named("keypair:get")
> + @GET
> + @Path("/os-keypairs/{name}")
> + @SelectJson("keypair")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Produces(MediaType.APPLICATION_JSON)
This request does not produce a body. Is this annotation really needed?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13406729
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Jeremy Daggett <no...@github.com>.
@demobox ping! Any other comments here? @everett-toews @zack-shoylev Can you please review?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-46612915
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Andrew Phillips <no...@github.com>.
>
> + public void testGetKeyPair() throws Exception {
Would it make sense to interleave this test with the `createDelete` test above? So we would do create - get - delete instead?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13473426
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1258](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1258/) 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/391#issuecomment-46709676
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Zack Shoylev <no...@github.com>.
Thanks @jdaggett
Is this going to be backported?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-46830444
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Jeremy Daggett <no...@github.com>.
Closed #391.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#event-133631452
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1255](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1255/) FAILURE
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/391#issuecomment-46638312
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #915](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/915/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-46639681
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1332](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1332/) 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/391#issuecomment-45237041
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Andrew Phillips <no...@github.com>.
>
> + public void testGetKeyPair() throws Exception {
> for (String zoneId : api.getConfiguredZones()) {
> KeyPairApi keyPairApi = api.getKeyPairExtensionForZone(zoneId).get();
> KeyPair keyPair = null;
> try {
> keyPair = keyPairApi.createWithPublicKey(KEYPAIR_NAME, PUBLIC_KEY);
> assertNotNull(keyPair);
Remove this assertion - not relevant to this test? Or fail at this stage with "unable to prepare test" or so?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13473416
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1387](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1387/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-46708566
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1188](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1188/) 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/391#issuecomment-45233943
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #1256](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/1256/) 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/391#issuecomment-46640703
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #896](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/896/) FAILURE
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/391#issuecomment-46203347
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #914](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/914/) FAILURE
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/391#issuecomment-46637574
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Zack Shoylev <no...@github.com>.
+1
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-46708892
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1367](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1367/) FAILURE
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/391#issuecomment-46203420
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #855](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/855/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-45123432
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Andrew Phillips <no...@github.com>.
> @@ -49,6 +48,16 @@
> KeyPair create(String name);
>
> /**
> + * Gets a specific {@link KeyPair} by name.
> + *
> + * @param name
> + * name of the Key Pair
> + *
> + * @return the specified {@link KeyPair}
"...or `null` if no such KeyPair exists"? And/or add `@Nullable`? I'm assuming this can be null based on the fallback?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13473345
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Jeremy Daggett <no...@github.com>.
Merged to master `da50cd6`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-46709241
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #861](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/861/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-45235781
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Ignasi Barrera <no...@github.com>.
Changes LGTM :) Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-45230373
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @jdaggett!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-45143363
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Ignasi Barrera <no...@github.com>.
> for (String zoneId : api.getConfiguredZones()) {
> KeyPairApi keyPairApi = api.getKeyPairExtensionForZone(zoneId).get();
> KeyPair keyPair = null;
> try {
> keyPair = keyPairApi.createWithPublicKey(KEYPAIR_NAME, PUBLIC_KEY);
> assertNotNull(keyPair);
> + keyPair = keyPairApi.get(KEYPAIR_NAME);
> + assertNotNull(keyPair);
Could you add an assertion to verify the name of the key pair too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13406979
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Zack Shoylev <no...@github.com>.
> try {
> - keyPair = keyPairApi.create(KEYPAIR_NAME);
> - assertNotNull(keyPair);
> + createdKeyPair = keyPairApi.create(KEYPAIR_NAME);
> + assertNotNull(createdKeyPair);
> +
> + KeyPair keyPair = keyPairApi.get(KEYPAIR_NAME);
Either that or break it up into multiple tests. But I don't think breaking it up will be better...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13993027
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Jeremy Daggett <no...@github.com>.
@zack-shoylev Yes, I was planning on backporting this fix.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-46859836
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1385](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1385/) FAILURE
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/391#issuecomment-46637557
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Zack Shoylev <no...@github.com>.
> @@ -81,6 +76,15 @@
> @Payload("%7B\"keypair\":%7B\"name\":\"{name}\"%7D%7D")
> ListenableFuture<? extends KeyPair> create(@PayloadParam("name") String name);
>
> + @Named("keypair:get")
> + @GET
> + @Path("/os-keypairs/{name}")
> + @SelectJson("keypair")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Fallback(NullOnNotFoundOr404.class)
> + @Nullable
> + ListenableFuture<? extends KeyPair> get(@PathParam("name") String name);
Is the bounded wildcard needed? I am not sure the create needs it either.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13992615
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1326](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1326/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-45124114
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Jeremy Daggett <no...@github.com>.
> @@ -81,6 +76,15 @@
> @Payload("%7B\"keypair\":%7B\"name\":\"{name}\"%7D%7D")
> ListenableFuture<? extends KeyPair> create(@PayloadParam("name") String name);
>
> + @Named("keypair:get")
> + @GET
> + @Path("/os-keypairs/{name}")
> + @SelectJson("keypair")
> + @Consumes(MediaType.APPLICATION_JSON)
> + @Fallback(NullOnNotFoundOr404.class)
> + @Nullable
> + ListenableFuture<? extends KeyPair> get(@PathParam("name") String name);
Good point!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13992955
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Jeremy Daggett <no...@github.com>.
@demobox Feedback reviewed and addressed in `dc7e13e`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-46203255
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -139,4 +139,38 @@ public void testDeleteKeyPair() throws Exception {
>
> assertTrue(apiWhenServersExist.getKeyPairExtensionForZone("az-1.region-a.geo-1").get().delete("testkeypair"));
> }
> +
> + public void testGetKeyPairWhenResponseIs2xx() throws Exception {
> + HttpRequest get = HttpRequest
> + .builder()
> + .method("GET")
> + .endpoint("https://az-1.region-a.geo-1.compute.hpcloudsvc.com/v1.1/3456/os-keypairs/testkeypair")
> + .addHeader("Accept", "application/json")
> + .addHeader("X-Auth-Token", authToken).build();
> +
> + HttpResponse getResponse = HttpResponse.builder().statusCode(200)
> + .payload(payloadFromResource("/keypair_details.json")).build();
> +
> + NovaApi apiWhenServersExist = requestsSendResponses(keystoneAuthWithUsernameAndPasswordAndTenantName,
Rename the variable to `apiWhenKeyPairExist`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13406802
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -49,6 +48,13 @@
> KeyPair create(String name);
>
> /**
> + * Get a Key Pair.
> + *
> + * @return a Key Pair
Specify which key pair is being returned?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13406694
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Andrew Phillips <no...@github.com>.
> @@ -57,15 +58,30 @@ public void testCreateAndDeleteKeyPair() throws Exception {
> }
>
> public void testCreateAndDeleteKeyPairWithPublicKey() throws Exception {
> - final String KEYPAIR_NAME = "testkp";
> - final String PUBLIC_KEY = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAAgQCrrBREFxz3002l1HuXz0+UOdJQ/mOYD5DiJwwB/TOybwIKQJPOxJWA9gBoo4k9dthTKBTaEYbzrll7iZcp59E80S6mNiAr3mUgi+x5Y8uyXeJ2Ws+h6peVyFVUu9epkwpcTd1GVfdcVWsTajwDz9+lxCDhl0RZKDFoT0scTxbj/w== nova@nv-aw2az2-api0002";
> + for (String zoneId : api.getConfiguredZones()) {
> + KeyPairApi keyPairApi = api.getKeyPairExtensionForZone(zoneId).get();
> + KeyPair keyPair = null;
> + try {
> + keyPair = keyPairApi.createWithPublicKey(KEYPAIR_NAME, PUBLIC_KEY);
> + assertNotNull(keyPair);
> + } finally {
> + if (keyPair != null) {
> + keyPairApi.delete(KEYPAIR_NAME);
Ah, interesting...delete would fail if a key with `KEYPAIR_NAME` did not exist?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13473377
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Andrew Phillips <no...@github.com>.
> jclouds-java-7-pull-requests #1332 UNSTABLE
Unrelated [test failure](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/org.apache.jclouds$jclouds-compute/1332/testReport/junit/org.jclouds.compute.callables/BlockUntilInitScriptStatusIsZeroThenReturnOutputTest/testloopUntilTrueOrThrowCancellationExceptionReturnsWhenPredicateIsTrueSecondTimeWhileNotCancelled/)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-45294428
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #916](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/916/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-46708254
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Jeremy Daggett <no...@github.com>.
> try {
> - keyPair = keyPairApi.create(KEYPAIR_NAME);
> - assertNotNull(keyPair);
> + createdKeyPair = keyPairApi.create(KEYPAIR_NAME);
> + assertNotNull(createdKeyPair);
> +
> + KeyPair keyPair = keyPairApi.get(KEYPAIR_NAME);
I had considered that but the language didn't make sense to me at the time. LOL I can change it...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13992965
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #1386](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/1386/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391#issuecomment-46639724
Re: [jclouds] Adds get() method to KeyPairApi (#391)
Posted by Zack Shoylev <no...@github.com>.
> try {
> - keyPair = keyPairApi.create(KEYPAIR_NAME);
> - assertNotNull(keyPair);
> + createdKeyPair = keyPairApi.create(KEYPAIR_NAME);
> + assertNotNull(createdKeyPair);
> +
> + KeyPair keyPair = keyPairApi.get(KEYPAIR_NAME);
Perhaps rename test to CreateAndGetAndDelete :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/391/files#r13992783