You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Svet <no...@github.com> on 2015/05/28 21:57:48 UTC
[jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/756
-- Commit Summary --
* Implement setting and retrieving the notes property on Softlayer machines
-- File Changes --
A providers/softlayer/src/main/java/org/jclouds/softlayer/binders/NotesToJson.java (57)
M providers/softlayer/src/main/java/org/jclouds/softlayer/compute/options/SoftLayerTemplateOptions.java (18)
M providers/softlayer/src/main/java/org/jclouds/softlayer/compute/strategy/SoftLayerComputeServiceAdapter.java (8)
M providers/softlayer/src/main/java/org/jclouds/softlayer/features/VirtualGuestApi.java (35)
A providers/softlayer/src/test/java/org/jclouds/softlayer/binders/NotesToJsonTest.java (49)
M providers/softlayer/src/test/java/org/jclouds/softlayer/features/VirtualGuestApiExpectTest.java (32)
M providers/softlayer/src/test/java/org/jclouds/softlayer/features/VirtualGuestApiLiveTest.java (10)
A providers/softlayer/src/test/resources/virtual_guest_set_notes.json (1)
A providers/softlayer/src/test/resources/virtual_guest_set_notes_response.json (1)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/756.patch
https://github.com/jclouds/jclouds/pull/756.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -213,6 +218,11 @@ public SoftLayerComputeServiceAdapter(SoftLayerApi api,
> api.getVirtualGuestApi().setTags(result.getId(), templateOptions.getTags());
> }
>
> + String notes = getNotesFrom(templateOptions);
> + if (!Strings.isNullOrEmpty(notes)) {
> + api.getVirtualGuestApi().setNotes(result.getId(), getNotesFrom(templateOptions));
Use the `notes` variable instead.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583284
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -125,6 +126,15 @@ public void testSetTagsOnVirtualGuest() throws Exception {
> }
>
> @Test(dependsOnMethods = "testSetTagsOnVirtualGuest")
> + public void testSetNotesOnVirtualGuest() throws Exception {
> + // Test with maximum allowed notes length - 1000 characters.
Why the -1000? It would seem better to test the limit values, as those are the ones that may cause trouble.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583778
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
Closed #756.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756#event-328246237
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> + * The string is set into the payload of the HttpRequest
> + *
> + */
> +@Singleton
> +public class NotesToJson implements Binder {
> +
> + private final Json json;
> +
> + @Inject
> + NotesToJson(Json json) {
> + this.json = checkNotNull(json, "json");
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkNotNull(input, "input");
`null` is not an instance of String, so this check is redundant.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r32069074
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
I've just commented on the remaining bits. Also take https://github.com/jclouds/jclouds/pull/763 into account when rebasing, just in case Andrea merges that PR first, as it seems to overlap with some of your changes.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756#issuecomment-109283546
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> + Map<String, String> meta = templateOptions.getUserMetadata();
> + if (meta != null && !meta.isEmpty()) {
> + if (meta.containsKey(USER_META_NOTES)) {
> + return meta.get(USER_META_NOTES);
> + } else {
> + String notes = "User Metadata\n===========\n\n" + Joiner.on("\n").withKeyValueSeparator(": ").join(meta);
> + if (notes.length() > SoftLayerTemplateOptions.NOTES_MAX_LENGTH) {
> + String suffix = "...\n<truncated>";
> + notes = notes.substring(0, SoftLayerTemplateOptions.NOTES_MAX_LENGTH - suffix.length()) + suffix;
> + }
> + return notes;
> + }
> + }
> + }
> + return null;
> + }
Ok. In that case, and given that SoftLayer does not support user metadata I wouldn't have the two approaches of having a dedicated "key" or serialise the entire metadata map. I'd pick just one. We'd document the key to use to set the notes, or we document that we'll assume all user metadata to be serialised as the notes, but not both. Pick the one you prefer :)
Also note that you are not taking not account the notes max length when reading the `USER_META_NOTES` key. Perhaps you also have to truncate it there?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31808918
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> + Map<String, String> meta = templateOptions.getUserMetadata();
> + if (meta != null && !meta.isEmpty()) {
> + if (meta.containsKey(USER_META_NOTES)) {
> + return meta.get(USER_META_NOTES);
> + } else {
> + String notes = "User Metadata\n===========\n\n" + Joiner.on("\n").withKeyValueSeparator(": ").join(meta);
> + if (notes.length() > SoftLayerTemplateOptions.NOTES_MAX_LENGTH) {
> + String suffix = "...\n<truncated>";
> + notes = notes.substring(0, SoftLayerTemplateOptions.NOTES_MAX_LENGTH - suffix.length()) + suffix;
> + }
> + return notes;
> + }
> + }
> + }
> + return null;
> + }
This looks *very* confusing. Are there three different ways to set the notes? There should be only one understandable way to set them.
Also, what is the purpose of serialising the user metadata as notes? I mean, why the user metadata and not some other property?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583472
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> + private final Json json;
> +
> + @Inject
> + public NotesToJson(Json json) {
> + this.json = checkNotNull(json, "json");
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(input instanceof String);
> + String notes = (String)checkNotNull(input, "input");
> + request.setPayload(buildJson(notes));
> + return request;
> + }
> +
> + String buildJson(String notes) {
Why not private?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583660
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Svet <no...@github.com>.
Looking good now :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756#issuecomment-110710696
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
Almost there! :) It looks that there is a [test failing](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/org.apache.jclouds.provider$softlayer/1818/testReport/junit/org.jclouds.softlayer.binders/NotesToJsonTest/testVirtualGuestNullNotes/) now. Mind fixing it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756#issuecomment-110659082
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.inject.Inject;
> +import com.google.inject.Singleton;
> +
> +/**
> + * Converts a String into a json string for changing the notes property of an instance
> + * The string is set into the payload of the HttpRequest
> + *
> + */
> +@Singleton
> +public class NotesToJson implements Binder {
> +
> + private final Json json;
> +
> + @Inject
> + NotesToJson(Json json) {
> + this.json = checkNotNull(json, "json");
Once the constructor is only visible to the injector, the null check is redundant (the injector already checks nulls). Remove it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r32069050
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Svet <no...@github.com>.
Addressed part of the comments, waiting for input on the rest.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756#issuecomment-108385482
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/9d652a9c) and [1.9.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/06f1b132). Thanks @neykov!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756#issuecomment-111041777
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -149,4 +152,34 @@
> @Produces(MediaType.APPLICATION_JSON)
> @Fallback(Fallbacks.FalseOnNotFoundOr404.class)
> boolean setTags(@PathParam("id") long id, @BinderParam(TagToJson.class) Set<String> tags);
> +
> + /**
> + * Set notes (visible in UI)
> + *
> + * @param id
> + * id of the virtual guest
> + */
> + @Named("VirtualGuest:setNotes")
> + @POST
> + @Path("/SoftLayer_Virtual_Guest/{id}/editObject")
> + @Produces(MediaType.APPLICATION_JSON)
> + @Fallback(Fallbacks.FalseOnNotFoundOr404.class)
404 fallbacks should not be used in write operations such as PUT or POST. Remove it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583532
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -213,6 +218,15 @@ public SoftLayerComputeServiceAdapter(SoftLayerApi api,
> api.getVirtualGuestApi().setTags(result.getId(), templateOptions.getTags());
> }
>
> + Map<String, String> meta = templateOptions.getUserMetadata();
> + if (meta != null) {
> + String notes = meta.get(USER_META_NOTES);
> + if (!Strings.isNullOrEmpty(notes)) {
> + checkArgument(notes.length() <= USER_META_NOTES_MAX_LENGTH, "'notes' property in user metadata should be long at most " + USER_META_NOTES_MAX_LENGTH + " characters.");
Failing at this point may leak the virtual guest that is created a few lines before. You'd better validate this at the beginning of the method to fail fast and avoid creating unnecessary resources.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r32069223
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -149,4 +152,34 @@
> @Produces(MediaType.APPLICATION_JSON)
> @Fallback(Fallbacks.FalseOnNotFoundOr404.class)
> boolean setTags(@PathParam("id") long id, @BinderParam(TagToJson.class) Set<String> tags);
> +
> + /**
> + * Set notes (visible in UI)
> + *
> + * @param id
> + * id of the virtual guest
Add also the javadoc for the other parameter.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583568
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Svet <no...@github.com>.
Addressed comments and squashed into a single commit.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756#issuecomment-110616036
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> + * The string is set into the payload of the HttpRequest
> + *
> + */
> +@Singleton
> +public class NotesToJson implements Binder {
> +
> + private final Json json;
> +
> + @Inject
> + public NotesToJson(Json json) {
> + this.json = checkNotNull(json, "json");
> + }
> +
> + @Override
> + public <R extends HttpRequest> R bindToRequest(R request, Object input) {
> + checkArgument(input instanceof String);
Is `null` an instance of a String? Consider doing the null check first.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583651
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -273,4 +273,36 @@ public void testSetTagsOnVirtualGuestWhenResponseIs4xx() {
> VirtualGuest virtualGuest = createVirtualGuest();
> assertFalse(api.getVirtualGuestApi().setTags(virtualGuest.getId(), ImmutableSet.of("test1", "test2", "test3")));
> }
> +
> + public void testSetNotesOnVirtualGuestWhenResponseIs2xx() {
> +
> + HttpRequest setNodesOnVirtualGuest = HttpRequest.builder().method("POST")
> + .endpoint("https://api.softlayer.com/rest/v3/SoftLayer_Virtual_Guest/1301396/editObject")
> + .addHeader("Accept", "application/json")
> + .addHeader("Authorization", "Basic aWRlbnRpdHk6Y3JlZGVudGlhbA==")
> + .payload(payloadFromResourceWithContentType("/virtual_guest_set_notes.json", MediaType.APPLICATION_JSON))
> + .build();
> +
> + HttpResponse setNotesOnVirtualGuestResponse = HttpResponse.builder().statusCode(200)
> + .payload(payloadFromResource("/virtual_guest_set_notes_response.json")).build();
Don't use a file just to store a `"true"`. Directly pass the String here.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583817
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -149,4 +152,34 @@
> @Produces(MediaType.APPLICATION_JSON)
> @Fallback(Fallbacks.FalseOnNotFoundOr404.class)
> boolean setTags(@PathParam("id") long id, @BinderParam(TagToJson.class) Set<String> tags);
> +
> + /**
> + * Set notes (visible in UI)
> + *
> + * @param id
> + * id of the virtual guest
> + */
> + @Named("VirtualGuest:setNotes")
> + @POST
> + @Path("/SoftLayer_Virtual_Guest/{id}/editObject")
> + @Produces(MediaType.APPLICATION_JSON)
> + @Fallback(Fallbacks.FalseOnNotFoundOr404.class)
Not in POST/PUT operations. What the user will perceive is a method returning `null` instead of failing. That seems correct when reading or deleting and the reference resource does not exist, but when writing, we should assume it exists, otherwise, what does a `null` return value mean in that case? The operation failed? You don't know. Did it succeed but there was no response body? You don't know. Fallbacks to null on 404 reponses make sense for read/delete operations, but should be removed from all POST/PUT ones.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31809111
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -125,6 +126,15 @@ public void testSetTagsOnVirtualGuest() throws Exception {
> }
>
> @Test(dependsOnMethods = "testSetTagsOnVirtualGuest")
> + public void testSetNotesOnVirtualGuest() throws Exception {
> + // Test with maximum allowed notes length - 1000 characters.
Oh, yes :) I didn't read the method properly!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r32069290
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + @BeforeClass
> + public void init() {
> + json = new GsonWrapper(new Gson());
> + }
> +
> + @Test
> + public void testVirtualGuestWithOperatingSystem() {
> + HttpRequest request = HttpRequest.builder().method("POST").endpoint("https://api.softlayer.com/rest/v3/SoftLayer_Virtual_Guest").build();
> + NotesToJson binder = new NotesToJson(json);
> + String notes = "some notes";
> +
> + request = binder.bindToRequest(request, notes);
> +
> + assertEquals(request.getPayload().getRawContent(), "{\"parameters\":[{\"notes\":\"some notes\"}]}");
> + }
Add tests for the special cases too: null notes and empty String.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583707
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> + HttpRequest setNodesOnVirtualGuest = HttpRequest.builder().method("POST")
> + .endpoint("https://api.softlayer.com/rest/v3/SoftLayer_Virtual_Guest/1301396/editObject")
> + .addHeader("Accept", "application/json")
> + .addHeader("Authorization", "Basic aWRlbnRpdHk6Y3JlZGVudGlhbA==")
> + .payload(payloadFromResourceWithContentType("/virtual_guest_set_notes.json", MediaType.APPLICATION_JSON))
> + .build();
> +
> + HttpResponse setNotesOnVirtualGuestResponse = HttpResponse.builder().statusCode(200)
> + .payload(payloadFromResource("/virtual_guest_set_notes_response.json")).build();
> +
> + SoftLayerApi api = requestSendsResponse(setNodesOnVirtualGuest, setNotesOnVirtualGuestResponse);
> + VirtualGuest virtualGuest = createVirtualGuest();
> + assertTrue(api.getVirtualGuestApi().setNotes(virtualGuest.getId(), "some notes"));
> + }
> +
> + public void testSetNotesOnVirtualGuestWhenResponseIs4xx() {
Remove this test once the fallback has been removed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583731
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Svet <no...@github.com>.
Addressed comments. To set notes in SL the user should use `notes` property in user metadata.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756#issuecomment-110344711
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Svet <no...@github.com>.
> + Map<String, String> meta = templateOptions.getUserMetadata();
> + if (meta != null && !meta.isEmpty()) {
> + if (meta.containsKey(USER_META_NOTES)) {
> + return meta.get(USER_META_NOTES);
> + } else {
> + String notes = "User Metadata\n===========\n\n" + Joiner.on("\n").withKeyValueSeparator(": ").join(meta);
> + if (notes.length() > SoftLayerTemplateOptions.NOTES_MAX_LENGTH) {
> + String suffix = "...\n<truncated>";
> + notes = notes.substring(0, SoftLayerTemplateOptions.NOTES_MAX_LENGTH - suffix.length()) + suffix;
> + }
> + return notes;
> + }
> + }
> + }
> + return null;
> + }
The canonical way to set the notes is through the property on the `SoftlayerTemplateOptions`. As the user metadata is part of the generic API it might be easier for users to write provider-independent code by setting the notes there, but don't mind removing it if you think it's too much.
Softlayer doesn't support user metadata, so the values there are ignored. The notes looked like a good place to store this information if not overridden by an explicit value. That was my thinking behind the various ways of getting the notes - wdyt?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31609033
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Svet <no...@github.com>.
> @@ -125,6 +126,15 @@ public void testSetTagsOnVirtualGuest() throws Exception {
> }
>
> @Test(dependsOnMethods = "testSetTagsOnVirtualGuest")
> + public void testSetNotesOnVirtualGuest() throws Exception {
> + // Test with maximum allowed notes length - 1000 characters.
Max length for notes is 1000, 1001 would case an error.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31609529
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
Just a couple minors and one comment left. Apart from that lgtm. Thanks @neykov! Mind squashing the commits into a single one when addressing the changes so I can cleanly merge it?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756#issuecomment-110516806
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Svet <no...@github.com>.
> @@ -149,4 +152,34 @@
> @Produces(MediaType.APPLICATION_JSON)
> @Fallback(Fallbacks.FalseOnNotFoundOr404.class)
> boolean setTags(@PathParam("id") long id, @BinderParam(TagToJson.class) Set<String> tags);
> +
> + /**
> + * Set notes (visible in UI)
> + *
> + * @param id
> + * id of the virtual guest
> + */
> + @Named("VirtualGuest:setNotes")
> + @POST
> + @Path("/SoftLayer_Virtual_Guest/{id}/editObject")
> + @Produces(MediaType.APPLICATION_JSON)
> + @Fallback(Fallbacks.FalseOnNotFoundOr404.class)
I've copied it from `setTags`, could make sense if the id is wrong?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31615428
Re: [jclouds] [Softlayer] Implements setting and retrieving the notes
property (#756)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.common.collect.ImmutableMap;
> +import com.google.inject.Inject;
> +import com.google.inject.Singleton;
> +
> +/**
> + * Converts a String into a json string for changing the notes property of an instance
> + * The string is set into the payload of the HttpRequest
> + *
> + */
> +@Singleton
> +public class NotesToJson implements Binder {
> +
> + private final Json json;
> +
> + @Inject
> + public NotesToJson(Json json) {
Remove the modifier to make the constructor package private, and remove the null check.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/756/files#r31583192