You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Christopher Dancy <no...@github.com> on 2015/03/10 21:43:05 UTC
[jclouds-labs] Jclouds 833 (#149)
https://issues.apache.org/jira/browse/JCLOUDS-833
ServiceKeys API has been fully implemented. Mock and Live tests both pass with latest Shipyard version 2.0.8.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/149
-- Commit Summary --
* ADDED: ServiceKeys API
* ADDED: mock tests for ServiceKeys API
* ADDED: live tests for ServiceKeys API.
-- File Changes --
M shipyard/README.md (34)
M shipyard/pom.xml (2)
M shipyard/src/main/java/org/jclouds/shipyard/ShipyardApi.java (4)
A shipyard/src/main/java/org/jclouds/shipyard/domain/servicekeys/CreateServiceKey.java (36)
A shipyard/src/main/java/org/jclouds/shipyard/domain/servicekeys/DeleteServiceKey.java (35)
A shipyard/src/main/java/org/jclouds/shipyard/domain/servicekeys/ServiceKey.java (38)
A shipyard/src/main/java/org/jclouds/shipyard/features/ServiceKeysApi.java (53)
M shipyard/src/main/java/org/jclouds/shipyard/handlers/ShipyardErrorHandler.java (6)
A shipyard/src/test/java/org/jclouds/shipyard/features/ServiceKeysApiLiveTest.java (77)
A shipyard/src/test/java/org/jclouds/shipyard/features/ServiceKeysApiMockTest.java (101)
A shipyard/src/test/resources/servicekey-create.json (3)
A shipyard/src/test/resources/servicekey-response.json (5)
A shipyard/src/test/resources/servicekeys.json (9)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/149.patch
https://github.com/jclouds/jclouds-labs/pull/149.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
For now I've put the new fallback tester inside ServiceKeysApiMockTest as 'testDeleteNonExistentServiceKeyWithErroneousData' as it's using the ServiceKeys "delete" call to stress that an exception is thrown upon erroneous body and return code data.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#issuecomment-78483689
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
@nacx can we merge? Release is out the door so I figure I'd start bothering again ;)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#issuecomment-89755923
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
> @@ -69,6 +70,12 @@ public void handleError(HttpCommand command, HttpResponse response) {
> if (command.getCurrentRequest().getMethod().equals("POST")) {
> if (command.getCurrentRequest().getEndpoint().getPath().endsWith("/engines")) {
> exception = new HttpResponseException("Connection refused registering docker daemon", command, response);
> + } else if (command.getCurrentRequest().getEndpoint().getPath().endsWith("/containers")) {
> + exception = new HttpResponseException(command, response, message);
> + }
> + } else if (command.getCurrentRequest().getMethod().equals("DELETE")) {
> + if (command.getCurrentRequest().getEndpoint().getPath().endsWith("/servicekeys")) {
> + exception = new ResourceNotFoundException(message, new HttpResponseException(command, response, message));
I can make a check to be safe but yes only a 500 with content "service key does not exist" pop on a DELETE at /api/servicekeys
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26326446
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
Regarding the fallback, they're used to recover upon failure and return a value. This means that the fallback should return an object compatible with the method's signature. In this case the method returns void but the fallback returns an Optional object. I'd create a custom fallback that returns void on failure.
Also it would be even better to make the fallback just return without failing if the error is a "not found" error and propagate the exception otherwise, but I don't know if given the exception in the fallback (caused by the 500 response) you can determine that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#issuecomment-78221632
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> +
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class CreateServiceKey {
> +
> + public abstract String description();
> +
> + CreateServiceKey() {
> + }
> +
> + @SerializedNames({ "description" })
> + public static CreateServiceKey create(String description) {
> + if (description == null) description = "";
Do we really want to do this instead of declaring the field `@Nullable`? What is the purpose of this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26166923
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
Oh, I see. There is an easier approach then: rempve the custom binders and binding annotations and just annotate the parameter with `@WrapWith("key")`. That will generate the expected json.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#issuecomment-78220336
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> +@Consumes(MediaType.APPLICATION_JSON)
> +@Produces(MediaType.APPLICATION_JSON)
> +@RequestFilters({ ServiceKeyAuthentication.class })
> +@Path("/api/servicekeys")
> +public interface ServiceKeysApi {
> +
> + @Named("servicekeys:list")
> + @GET
> + List<ServiceKey> listServiceKeys();
> +
> + @Named("servicekeys:create")
> + @POST
> + ServiceKey createServiceKey(@WrapWith("description") String description);
> +
> + @Named("servicekeys:delete")
> + @Fallback(VoidOnServiceKeyNotFoundAnd500.class)
WRT the comment in the other fallback, this is a write operation, but we use these fallbacks, because the delete operations are often used to release resources, and having to try/catch there is not convenient.
However, to follow the pattern used in other jclouds providers I'd change this method to return a `boolean` and change the fallback to return `false` if there is an exception. jclouds will return `true` by default if the response succeeds.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26263224
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -69,6 +70,12 @@ public void handleError(HttpCommand command, HttpResponse response) {
> if (command.getCurrentRequest().getMethod().equals("POST")) {
> if (command.getCurrentRequest().getEndpoint().getPath().endsWith("/engines")) {
> exception = new HttpResponseException("Connection refused registering docker daemon", command, response);
> + } else if (command.getCurrentRequest().getEndpoint().getPath().endsWith("/containers")) {
> + exception = new HttpResponseException(command, response, message);
> + }
> + } else if (command.getCurrentRequest().getMethod().equals("DELETE")) {
> + if (command.getCurrentRequest().getEndpoint().getPath().endsWith("/servicekeys")) {
> + exception = new ResourceNotFoundException(message, new HttpResponseException(command, response, message));
Is this `ResourceNotFoundException` the only possible error here, or should we inspect the response message to know when to throw the RNFE or the default one?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26324422
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class ServiceKey {
> +
> + public abstract String key();
> +
> + @Nullable public abstract String description();
> +
> + ServiceKey() {
> + }
> +
> + @SerializedNames({ "key", "description" })
> + public static ServiceKey create(String key, String description) {
> + return new AutoValue_ServiceKey(key, description);
> + }
I'd declare the Create and Delete service key classes as inner classes here. They don't seem to make sense outside the scope of this class, so I'd configure them as inner classes.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26167033
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
> +
> +import org.jclouds.json.SerializedNames;
> +
> +import com.google.auto.value.AutoValue;
> +
> +@AutoValue
> +public abstract class CreateServiceKey {
> +
> + public abstract String description();
> +
> + CreateServiceKey() {
> + }
> +
> + @SerializedNames({ "description" })
> + public static CreateServiceKey create(String description) {
> + if (description == null) description = "";
To generate a service key the description field must be passed and at the very least its value be set to an empty string. It is what it is...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26167919
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -64,6 +66,7 @@
> void restartContainer(@PathParam("id") String id);
>
> @Named("containers:deploy")
> + @Fallback(NullOnContainerResourceUnavailableAnd500.class)
I would remove this fallback, for several reasons: First, we shouldn't return `null` lists; we should fallback to an empty one. And also in a POST or PUT operation, a "unexisting resource" failure is ambiguous. What does a `null`return value mean? Does it mean that the resource you are posting to does not exist? Does it mean that the operation succeeded but returned no result?
The "return null" fallbacks are used in GET operations, to return null when a 404 response is returned. They map a not found error to a null return value, because that makes sense in the scope of a read operation. But when you are creating or modifying a resource, silently returning that is ambiguous.
The fallback should be removed from here, and let the method populate an exception. Then, the error handler should do the same thing the fallback does now, and throw a `ResoueceNotFoundException` or a generic `HttpResonseException` depending on the cause of the failure.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26262980
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -64,6 +66,7 @@
> void restartContainer(@PathParam("id") String id);
>
> @Named("containers:deploy")
> + @Fallback(NullOnContainerResourceUnavailableAnd500.class)
Hmmm does it just return `[ { object } ]`, or does it return something like:
```json
{ "foo": [ { "id": "foo", "bar": "bar" } ] }
```
I mean, the returned list is inside a json object or does it directly return the raw list?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26324884
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -90,7 +94,7 @@ private String parseMessage(HttpResponse response) {
> try {
> return Strings2.toStringAndClose(response.getPayload().openStream());
> } catch (IOException e) {
> - throw Throwables.propagate(e);
> + return Throwables.getStackTraceAsString(e);
I'd revert this change. If you do this, users may get an HttpResponseException that means that the target provider returned an error response but instead of having the real error message you will be silently return a complete different error: the error jclouds found when trying to read the error stream.
Let's fail if you can't read it and propagate that unexpected exception.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26167770
Re: [jclouds-labs] [JCLOUDS-833] ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
@nacx review when you get a chance. Thanks ;)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#issuecomment-78145833
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
> @@ -64,6 +66,7 @@
> void restartContainer(@PathParam("id") String id);
>
> @Named("containers:deploy")
> + @Fallback(NullOnContainerResourceUnavailableAnd500.class)
Exactly as I have it. That is literally the content from the jclouds-wire.log I posted.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26326685
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
@nacx all looks well. I believe all your comments have been addressed and for the better. Specifically the fallbacks should now address the 500's returned both from the ServiceKeysApi and The ContainersApi
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#issuecomment-78271201
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> + List<ServiceKey> possibleServiceKeys = api().listServiceKeys();
> + assertNotNull(possibleServiceKeys, "possibleServiceKeys was not set");
> + assertTrue(possibleServiceKeys.size() > 0, "Expected at least 1 ServiceKey but list was empty");
> + boolean serviceKeyFound = false;
> + for (ServiceKey possibleKey : possibleServiceKeys) {
> + if (possibleKey.key().equals(serviceKey)) {
> + serviceKeyFound = true;
> + }
> + }
> + assertTrue(serviceKeyFound, "Expected but could not find ServiceKey amongst " + possibleServiceKeys.size() + " found");
> + }
> +
> + @Test (expectedExceptions = HttpResponseException.class)
> + public void testRemoveNonExistentServiceKey() throws Exception {
> + DeleteServiceKey deleteServiceKey = DeleteServiceKey.create(UUID.randomUUID().toString().replaceAll("-", ""));
> + api().deleteServiceKey(deleteServiceKey);
Given the exception from the provider can you determine if it is a "Not Found" exception? If you can, I'd create a custom fallback or configure the error handler to propagate a `ResourceNotFoundException` instead of the generic `HttpResponseException`. That's how all other jclouds providers behave.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26168241
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
> @@ -64,6 +66,7 @@
> void restartContainer(@PathParam("id") String id);
>
> @Named("containers:deploy")
> + @Fallback(NullOnContainerResourceUnavailableAnd500.class)
This is a funny one and an open bug for Shipyard. The deployContainer call only deploys a single container but returns a JSON array which is why we return the list here. There will only ever be 1 or 0 elements.
With that said yes an ResourceNotFoundExeption makes more sense.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26269708
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> + MockWebServer server = mockShipyardWebServer();
> + server.enqueue(new MockResponse().setResponseCode(204));
> + ShipyardApi shipyardApi = api(server.getUrl("/"));
> + ServiceKeysApi api = shipyardApi.serviceKeysApi();
> + try {
> + DeleteServiceKey deleteServiceKey = DeleteServiceKey.create("1111222233334444");
> + api.deleteServiceKey(deleteServiceKey);
> + assertSent(server, "DELETE", "/api/servicekeys");
> + } finally {
> + shipyardApi.close();
> + server.shutdown();
> + }
> + }
> +
> + @Test (expectedExceptions = HttpResponseException.class)
> + public void testDeleteNonExistentServiceKey() throws Exception {
Don't add tests for paths that are not explicitly present in the code. If you don't declare a fallback, then remove the test. There's no need to test the jclouds default behavior.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26168317
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
> @@ -64,6 +66,7 @@
> void restartContainer(@PathParam("id") String id);
>
> @Named("containers:deploy")
> + @Fallback(NullOnContainerResourceUnavailableAnd500.class)
[{"id":"8000dcb50ba2da941e49c5aad32d034ce22d2e7b80b675857ce30b38ff1b4149","name":"shipyard-jclouds-container-test","image":{"name":"ubuntu:14.04.1","cpus":1,"memory":512,"environment":{"shipyard-jclouds":"test"},"args":["/bin/bash"],"type":"service","labels":["jclouds-shipard-live-test"],"restart_policy":{},"container_name":"shipyard-jclouds-container-test"},"engine":{"id":"jclouds-shipard-live-test","addr":"http://10.0.0.8:2375","cpus":1,"memory":1024,"labels":["jclouds-shipard-live-test"]}}]
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26295928
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
Squashed and pushed to master and 1.9.x. Thanks @cdancy!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#issuecomment-90268987
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> +@Consumes(MediaType.APPLICATION_JSON)
> +@RequestFilters({ ServiceKeyAuthentication.class })
> +@Path("/api/servicekeys")
> +public interface ServiceKeysApi {
> +
> + @Named("servicekeys:list")
> + @GET
> + List<ServiceKey> listServiceKeys();
> +
> + @Named("servicekeys:create")
> + @POST
> + ServiceKey createServiceKey(@BinderParam(BindToJsonPayload.class) CreateServiceKey createServiceKey);
> +
> + @Named("servicekeys:delete")
> + @DELETE
> + void deleteServiceKey(@BinderParam(BindToJsonPayload.class) DeleteServiceKey deleteServiceKey);
Looking at this with more detail, it makes not sense to have an object just to hold a single value. Change these methods to receive a String, and configure the annotations to automatically populate it to the payload (look at [any Profitbricks API](https://github.com/jclouds/jclouds-labs/blob/master/profitbricks/src/main/java/org/jclouds/profitbricks/features/DataCenterApi.java#L64-L69) for an example) and remove the Create and Delete service key classes.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26168042
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
@nacx I believe all comments have been addressed. You'll notice I used custom binders to address the single field AutoValue classes. I could not find an easy way to inject the json into say @Payload("{ \"key\": "{key}\"}") without the json getting wrongly generated. Other projects solved this same exact problem similarly by creating custom binders even for a single field.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#issuecomment-78186637
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
> @@ -90,7 +94,7 @@ private String parseMessage(HttpResponse response) {
> try {
> return Strings2.toStringAndClose(response.getPayload().openStream());
> } catch (IOException e) {
> - throw Throwables.propagate(e);
> + return Throwables.getStackTraceAsString(e);
Fair enough. I guess my thinking was that the exception turned-into-string would get put inside the exception anyway.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26168037
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
> + List<ServiceKey> possibleServiceKeys = api().listServiceKeys();
> + assertNotNull(possibleServiceKeys, "possibleServiceKeys was not set");
> + assertTrue(possibleServiceKeys.size() > 0, "Expected at least 1 ServiceKey but list was empty");
> + boolean serviceKeyFound = false;
> + for (ServiceKey possibleKey : possibleServiceKeys) {
> + if (possibleKey.key().equals(serviceKey)) {
> + serviceKeyFound = true;
> + }
> + }
> + assertTrue(serviceKeyFound, "Expected but could not find ServiceKey amongst " + possibleServiceKeys.size() + " found");
> + }
> +
> + @Test (expectedExceptions = HttpResponseException.class)
> + public void testRemoveNonExistentServiceKey() throws Exception {
> + DeleteServiceKey deleteServiceKey = DeleteServiceKey.create(UUID.randomUUID().toString().replaceAll("-", ""));
> + api().deleteServiceKey(deleteServiceKey);
Using fallback AbsentOn403Or404Or500 as "delete" call returns 500 if ServiceKey does not exist.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26183537
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
Closed #149.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#event-274613498
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -64,6 +66,7 @@
> void restartContainer(@PathParam("id") String id);
>
> @Named("containers:deploy")
> + @Fallback(NullOnContainerResourceUnavailableAnd500.class)
Can you paste an example of that returned json?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26282031
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
> +@Consumes(MediaType.APPLICATION_JSON)
> +@RequestFilters({ ServiceKeyAuthentication.class })
> +@Path("/api/servicekeys")
> +public interface ServiceKeysApi {
> +
> + @Named("servicekeys:list")
> + @GET
> + List<ServiceKey> listServiceKeys();
> +
> + @Named("servicekeys:create")
> + @POST
> + ServiceKey createServiceKey(@BinderParam(BindToJsonPayload.class) CreateServiceKey createServiceKey);
> +
> + @Named("servicekeys:delete")
> + @DELETE
> + void deleteServiceKey(@BinderParam(BindToJsonPayload.class) DeleteServiceKey deleteServiceKey);
Aaahhh that's much cleaner! Let me jump on that now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26168223
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -70,6 +70,10 @@ public void handleError(HttpCommand command, HttpResponse response) {
> if (command.getCurrentRequest().getEndpoint().getPath().endsWith("/engines")) {
> exception = new HttpResponseException("Connection refused registering docker daemon", command, response);
> }
> + } else if (command.getCurrentRequest().getMethod().equals("DELETE")) {
> + if (command.getCurrentRequest().getEndpoint().getPath().endsWith("/servicekeys")) {
Instead of "polluting" the error handler with custom checks of the invoked request path, it would be better to create a custom [Fallback](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/Fallback.java) and annotate the API methods so they use it.
This way the fallback for each method will be able to properly set the exception message and populate the right one: The error handler will no longer be coupled to the requests and will already receive the proper exception.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26167562
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Ignasi Barrera <no...@github.com>.
Just two comments about the fallbacks. One can be removed, and the remaining one should be changed. Also you should add the corresponding unit tests for the fallback class itself.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#issuecomment-78387893
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
> +@Consumes(MediaType.APPLICATION_JSON)
> +@Produces(MediaType.APPLICATION_JSON)
> +@RequestFilters({ ServiceKeyAuthentication.class })
> +@Path("/api/servicekeys")
> +public interface ServiceKeysApi {
> +
> + @Named("servicekeys:list")
> + @GET
> + List<ServiceKey> listServiceKeys();
> +
> + @Named("servicekeys:create")
> + @POST
> + ServiceKey createServiceKey(@WrapWith("description") String description);
> +
> + @Named("servicekeys:delete")
> + @Fallback(VoidOnServiceKeyNotFoundAnd500.class)
Fair enough. I was just following the API here and since it returns nothing I did as well. Easy enough to implement the boolean value.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149/files#r26269775
Re: [jclouds-labs] [JCLOUDS-833] Shipyard ServiceKeys API (#149)
Posted by Christopher Dancy <no...@github.com>.
As far as adding a new test for the fallback does it make sense to create a separate class or add the test functionality elsewhere? The refactor of the fallbacks is going to leave me with a class 'BooleanOnServiceKeyNotFoundAnd500' which is already going to be exercised as part of ShipyardKeysApiMockTest.testDeleteNonExistentServiceKey. I've got a tester here named 'testDeleteNonExistentServiceKeyWithErroneousBody' which will stress the aforementioned fallback to ensure it throws an exception should all checks not succeed. I guess my ultimate question is: should I put that tester in its own class, say ShipyardFallbacksTest, or append it to ServiceKeysApiMockTest? I can see reasons for both and am not married to either.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/149#issuecomment-78471209