You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by monitorjbl <no...@github.com> on 2014/12/11 16:05:17 UTC
[jclouds] Fix for JCLOUDS-655 (#626)
First time committing to jclouds, let me know if there are any style guidelines I missed with this change.
You can merge this Pull Request by running:
git pull https://github.com/monitorjbl/jclouds JCLOUDS-655
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/626
-- Commit Summary --
* fix for JCLOUDS-655
-- File Changes --
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/functions/ImageInZoneToImage.java (22)
M apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/domain/Image.java (12)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/compute/functions/ImageInZoneToImageTest.java (42)
A apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/features/ImageApiMockTest.java (52)
M apis/openstack-nova/src/test/java/org/jclouds/openstack/nova/v2_0/parse/ParseImageTest.java (2)
A apis/openstack-nova/src/test/resources/image_list_with_block_device_mapping.json (58)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/626.patch
https://github.com/jclouds/jclouds/pull/626.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
> +package org.jclouds.openstack.nova.v2_0.features;
> +
> +import com.google.common.collect.FluentIterable;
> +import com.squareup.okhttp.mockwebserver.MockResponse;
> +import com.squareup.okhttp.mockwebserver.MockWebServer;
> +import org.jclouds.openstack.nova.v2_0.NovaApi;
> +import org.jclouds.openstack.nova.v2_0.domain.Image;
> +import org.jclouds.openstack.v2_0.internal.BaseOpenStackMockTest;
> +import org.testng.annotations.Test;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +import static org.testng.Assert.assertNull;
> +
> +@Test(groups = "unit")
> +public class ImageApiMockTest extends BaseOpenStackMockTest<NovaApi> {
Will do!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21998702
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> + @Override
> + public Image deserialize(JsonElement jsonElement, Type type, JsonDeserializationContext context)
> + throws JsonParseException {
> + JsonObject json = jsonElement.getAsJsonObject();
> +
> + Map<String, Object> complexMetatdata = null;
> + Map<String, String> metadata = null;
> + JsonElement meta = json.get(METADATA);
> + if (meta != null && meta.isJsonObject()) {
> + metadata = Maps.newTreeMap();
> + complexMetatdata = Maps.newTreeMap();
> + for (Map.Entry<String, JsonElement> e : meta.getAsJsonObject().entrySet()) {
> + Object value;
> + if (e.getValue().isJsonArray()) {
> + value = context.deserialize(e.getValue().getAsJsonArray(), ArrayList.class);
> + } else if (e.getValue().isJsonObject()) {
You'll need to add a test to `ImageApiMockTest` for this condition.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21985936
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> @@ -56,9 +61,10 @@
> @Singleton
> public Map<Type, Object> provideCustomAdapterBindings() {
> return ImmutableMap.<Type, Object>of(
> - HostResourceUsage.class, new HostResourceUsageAdapter(),
> - ServerWithSecurityGroups.class, new ServerWithSecurityGroupsAdapter(),
> - Server.class, new ServerAdapter()
> + HostResourceUsage.class, new HostResourceUsageAdapter(),
> + ServerWithSecurityGroups.class, new ServerWithSecurityGroupsAdapter(),
> + Server.class, new ServerAdapter(),
> + Image.class, new ImageAdapter()
> );
Should use 3 space indents.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21985620
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> @@ -271,6 +282,10 @@ public Resource getServer() {
> return this.metadata;
> }
>
> + public Map<String, Object> getComplexMetadata() {
Both `getComplexMetadata()` and `getMetadata()` will need to be documented very well. Please explain why each one exists and that `metadata` will always be a subset of `complexMetadata`. Explain what kind of `Object`s can be returned in the `Map<String, Object>`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21986299
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> })
> protected ServerInternalWithoutImage(String id, @Nullable String name, java.util.Set<Link> links, @Nullable String uuid, String tenantId,
> - String userId, Date updated, Date created, @Nullable String hostId, @Nullable String accessIPv4,
> - @Nullable String accessIPv6, Server.Status status, Resource flavor, @Nullable String keyName,
> - @Nullable String configDrive, Multimap<String, Address> addresses, Map<String, String> metadata,
> - @Nullable ServerExtendedStatus extendedStatus, @Nullable ServerExtendedAttributes extendedAttributes, @Nullable String diskConfig) {
> + String userId, Date updated, Date created, @Nullable String hostId, @Nullable String accessIPv4,
> + @Nullable String accessIPv6, Server.Status status, Resource flavor, @Nullable String keyName,
> + @Nullable String configDrive, Multimap<String, Address> addresses, Map<String, String> metadata,
> + @Nullable ServerExtendedStatus extendedStatus, @Nullable ServerExtendedAttributes extendedAttributes, @Nullable String diskConfig) {
Please undo all of the unintentional updates to the spacing above.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21985663
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> })
> protected ServerInternalWithoutImage(String id, @Nullable String name, java.util.Set<Link> links, @Nullable String uuid, String tenantId,
> - String userId, Date updated, Date created, @Nullable String hostId, @Nullable String accessIPv4,
> - @Nullable String accessIPv6, Server.Status status, Resource flavor, @Nullable String keyName,
> - @Nullable String configDrive, Multimap<String, Address> addresses, Map<String, String> metadata,
> - @Nullable ServerExtendedStatus extendedStatus, @Nullable ServerExtendedAttributes extendedAttributes, @Nullable String diskConfig) {
> + String userId, Date updated, Date created, @Nullable String hostId, @Nullable String accessIPv4,
> + @Nullable String accessIPv6, Server.Status status, Resource flavor, @Nullable String keyName,
> + @Nullable String configDrive, Multimap<String, Address> addresses, Map<String, String> metadata,
> + @Nullable ServerExtendedStatus extendedStatus, @Nullable ServerExtendedAttributes extendedAttributes, @Nullable String diskConfig) {
I see what you mean. I'm fine with this one the way it is.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r22008280
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Whoops! Fixed it, sorry about that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-71325502
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
I don't like having two accessors for the same information either but we're choosing between the lesser of two evils here. If you only had `complexMetadata`, you'd be forcing users to always write code like
Object value = image.getComplexMetadata().get("key");
if (value instanceof List) {
// do something
}
else if (value instanceof Map) {
// do something
}
else {
// treat it like a String
}
Instead of dealing with the more common case
String value = image.getMetadata().get("key");
It's a testament to how common the `<String, String>` case is that this issue has only come up relatively recent and openstack-nova has been in use for years.
Anything more than one line to deal with metadata is more evil than two accessors for the same info.
P.S. Let's not lose site of the fact that it's the Nova API that's forcing this horribleness on us and forcing us to choose between these two crappy options. I'll be addressing this in OpenStack itself by proposing a guideline to the [API Working Group](https://wiki.openstack.org/wiki/API_Working_Group) that will (one day) stop this kind of thing from happening.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-67413988
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> @@ -85,6 +86,7 @@ public static Status fromValue(String v) {
> protected int minDisk;
> protected int minRam;
> protected Resource server;
> + protected List<BlockDeviceMapping> blockDeviceMapping;
This should be initialized with `ImmutableList.of()` (unless there's a really good reason not to). All of the other Collection types in the domain package are initialized this way. `blockDeviceMapping` should be the same.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r23227753
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> +package org.jclouds.openstack.nova.v2_0.features;
> +
> +import com.google.common.collect.FluentIterable;
> +import com.squareup.okhttp.mockwebserver.MockResponse;
> +import com.squareup.okhttp.mockwebserver.MockWebServer;
> +import org.jclouds.openstack.nova.v2_0.NovaApi;
> +import org.jclouds.openstack.nova.v2_0.domain.Image;
> +import org.jclouds.openstack.v2_0.internal.BaseOpenStackMockTest;
> +import org.testng.annotations.Test;
> +
> +import static org.testng.Assert.assertEquals;
> +import static org.testng.Assert.assertNotNull;
> +import static org.testng.Assert.assertNull;
> +
> +@Test(groups = "unit")
> +public class ImageApiMockTest extends BaseOpenStackMockTest<NovaApi> {
3 space indents. Remove empty lines near end. Add newline at EOF.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21986419
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Switched over to using the formal class instead. I started work on adding a test case for the `ImageAdapter` class, but I'm having a really hard time getting this class to work in isolation. It looks like something, somewhere does some magic to decode this:
```
{
"image": {
"id": "52415800-8b69-11e0-9b19-734f5736d2a2"
}
}
```
as a single `Image` object. I'm not sure where that takes place, and I'd really prefer to not to clutter up your resource directory with a bunch of files just for this test formatted like this:
```
{
"id": "52415800-8b69-11e0-9b19-734f5736d2a2"
}
```
Any suggestions there?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-68227190
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
> @@ -86,6 +86,7 @@ public static Status fromValue(String v) {
> protected int minRam;
> protected Resource server;
> protected Map<String, String> metadata = ImmutableMap.of();
Good question! I don't know if this is common enough in the greater community to warrant that, personally. I'm basing that opinion on the number of Google results I found when I ran into this issue though :) You would have greater exposure to user issues than me.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21995806
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> + server.enqueue(addCommonHeaders(new MockResponse().setBody(stringFromResource("/access.json"))));
> + server.enqueue(addCommonHeaders(new MockResponse().setBody(stringFromResource("/image_list_with_block_device_mapping_object.json"))));
> +
> + try {
> + NovaApi novaApi = api(server.getUrl("/").toString(), "openstack-nova");
> + ImageApi imageApi = novaApi.getImageApiForZone("RegionOne");
> +
> + FluentIterable<? extends Image> images = imageApi.listInDetail().concat();
> +
> + Image img = images.get(0);
> + assertNotNull(img.getMetadata());
> + assertEquals(10, img.getMetadata().size());
> + assertNull(img.getMetadata().get("block_device_mapping"));
> + assertEquals(11, img.getComplexMetadata().size());
> + assertNotNull(img.getComplexMetadata().get("block_device_mapping"));
> + assertTrue(img.getComplexMetadata().get("block_device_mapping") instanceof Map);
Need to `assertEquals` for some of the values _within_ the block device mapping.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r22011507
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
> super(id, name, links, uuid, tenantId, userId, updated, created, hostId, accessIPv4, accessIPv6, status, null, flavor, keyName, configDrive, addresses, metadata, extendedStatus, extendedAttributes, diskConfig);
> }
> }
> }
> +
> + @Singleton
> + public static class ImageAdapter implements JsonDeserializer<Image> {
> + public static final String METADATA = "metadata";
Gonna leave this public so I can unit test the class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r22011722
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Ignasi Barrera <no...@github.com>.
@monitorjbl I've just opened https://github.com/monitorjbl/jclouds/pull/1 to your branch with an approach that works. HTH!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-70241744
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> + @Override
> + public Image deserialize(JsonElement jsonElement, Type type, JsonDeserializationContext context)
> + throws JsonParseException {
> + JsonObject json = jsonElement.getAsJsonObject();
> +
> + Map<String, Object> complexMetatdata = null;
> + Map<String, String> metadata = null;
> + JsonElement meta = json.get(METADATA);
> + if (meta != null && meta.isJsonObject()) {
> + metadata = Maps.newTreeMap();
> + complexMetatdata = Maps.newTreeMap();
> + for (Map.Entry<String, JsonElement> e : meta.getAsJsonObject().entrySet()) {
> + Object value;
> + if (e.getValue().isJsonArray()) {
> + value = context.deserialize(e.getValue().getAsJsonArray(), ArrayList.class);
> + } else if (e.getValue().isJsonObject()) {
Here are some examples.
$ find . -name *ParserModuleTest.java
./jclouds/apis/chef/src/test/java/org/jclouds/chef/config/ChefParserModuleTest.java
./jclouds-chef/target/checkout/core/src/test/java/org/jclouds/chef/config/ChefParserModuleTest.java
./jclouds-labs/docker/src/test/java/org/jclouds/docker/config/DockerParserModuleTest.java
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r22010012
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Ignasi Barrera <no...@github.com>.
> + @Override
> + public Image deserialize(JsonElement jsonElement, Type type, JsonDeserializationContext context)
> + throws JsonParseException {
> + JsonObject json = jsonElement.getAsJsonObject();
> +
> + Map<String, Object> complexMetatdata = null;
> + Map<String, String> metadata = null;
> + JsonElement meta = json.get(METADATA);
> + if (meta != null && meta.isJsonObject()) {
> + metadata = Maps.newTreeMap();
> + complexMetatdata = Maps.newTreeMap();
> + for (Map.Entry<String, JsonElement> e : meta.getAsJsonObject().entrySet()) {
> + Object value;
> + if (e.getValue().isJsonArray()) {
> + value = context.deserialize(e.getValue().getAsJsonArray(), ArrayList.class);
> + } else if (e.getValue().isJsonObject()) {
And also an `ImageAdapterTest`. We need to unit test this class itself.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21987429
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -86,6 +86,7 @@ public static Status fromValue(String v) {
> protected int minRam;
> protected Resource server;
> protected Map<String, String> metadata = ImmutableMap.of();
Should this field be marked `@Deprecated` and properly redirect to the new one int he javadoc?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21987384
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
> @@ -267,7 +267,7 @@ public Resource getServer() {
> return this.server;
> }
>
> - public Map<String, String> getMetadata() {
> + public Map<String, Object> getMetadata() {
Makes sense to me, but I might suggest a different field name for future upgrades (never know when another object will show up in there). Perhaps `complexMetadata`?
Either way, I'll need to dig into the deserialization a bit. Any pointers on where to start looking for examples in the code base?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21717963
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
Merged to [1.8.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/f708d203) and [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/73600c81)
Nice work @monitorjbl! This was a tough one but you stuck with it. Can we give you a shout out on Twitter? If so, what's your Twitter handle?
Thanks for all the help @nacx
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-71532424
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
Can you add that test code as a commit so we can see it? (don't squash it)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-69248945
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Done. It throws an error saying "links" is null, which is to be expected since it's not defined the way that a normal Gson adapter would read the `Image` object from JSON. The trouble I'm having is testing this is an isolated enough way that it's still a unit test of just the adapter; it doesn't appear to work with any sample output from Openstack on its own.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-69264181
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Ignasi Barrera <no...@github.com>.
@monitorjbl @everett-toews I'll have a look at it this week.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-69891714
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
> + @Override
> + public Image deserialize(JsonElement jsonElement, Type type, JsonDeserializationContext context)
> + throws JsonParseException {
> + JsonObject json = jsonElement.getAsJsonObject();
> +
> + Map<String, Object> complexMetatdata = null;
> + Map<String, String> metadata = null;
> + JsonElement meta = json.get(METADATA);
> + if (meta != null && meta.isJsonObject()) {
> + metadata = Maps.newTreeMap();
> + complexMetatdata = Maps.newTreeMap();
> + for (Map.Entry<String, JsonElement> e : meta.getAsJsonObject().entrySet()) {
> + Object value;
> + if (e.getValue().isJsonArray()) {
> + value = context.deserialize(e.getValue().getAsJsonArray(), ArrayList.class);
> + } else if (e.getValue().isJsonObject()) {
D'oh! Good point about the additional unit test for the JsonObject case, I'll get that added ASAP. I'm not sure about testing the class directly though, I'd actually considered doing that originally but the other adapters didn't have any unit tests. I figured that would have just been overkill, but I'm happy to add one if you guys think it's necessary.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21995629
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Ignasi Barrera <no...@github.com>.
Absolutely. It makes more sense to extract that metadata in a specific object, as it shouldn't be an "arbitrary" map of values. @everett-toews are you ok with this approach?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-67786490
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Thanks, and I definitely don't mind a tweet :) My handle is @monitorjbl.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-71562806
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Updated init for blockDeviceMapping.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-71228380
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Ignasi Barrera <no...@github.com>.
It seems that the commit introduced a couple checkstyle violations. Mind fixing them?
https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1462/org.apache.jclouds.api$openstack-nova/violations/
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-66644403
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
I think this is a reasonable way to approach this and not break backwards compatibility or force the user to always deal with `Map<String, Object>` when the more common case is `Map<String, String>`
@nacx @inbarsto Since you were kind enough to reply to the [Dealing with different JSON value types in a Map](http://mail-archives.apache.org/mod_mbox/incubator-jclouds-dev/201409.mbox/%3CA97B84B3-3890-4675-819A-3118249FB43E@rackspace.com%3E), I'd be interested in your opinion on this approach to fix it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-67355954
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
> })
> protected ServerInternalWithoutImage(String id, @Nullable String name, java.util.Set<Link> links, @Nullable String uuid, String tenantId,
> - String userId, Date updated, Date created, @Nullable String hostId, @Nullable String accessIPv4,
> - @Nullable String accessIPv6, Server.Status status, Resource flavor, @Nullable String keyName,
> - @Nullable String configDrive, Multimap<String, Address> addresses, Map<String, String> metadata,
> - @Nullable ServerExtendedStatus extendedStatus, @Nullable ServerExtendedAttributes extendedAttributes, @Nullable String diskConfig) {
> + String userId, Date updated, Date created, @Nullable String hostId, @Nullable String accessIPv4,
> + @Nullable String accessIPv6, Server.Status status, Resource flavor, @Nullable String keyName,
> + @Nullable String configDrive, Multimap<String, Address> addresses, Map<String, String> metadata,
> + @Nullable ServerExtendedStatus extendedStatus, @Nullable ServerExtendedAttributes extendedAttributes, @Nullable String diskConfig) {
I can clean up almost all of this with autoformat, but the `ServerInternalWithoutImage()` layout is apparently special. If you look at the original version, there are two different styles of indentation. [One](https://github.com/jclouds/jclouds/blob/1.8.x/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/config/NovaParserModule.java#L154) is like mine and the [other](https://github.com/jclouds/jclouds/blob/1.8.x/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/config/NovaParserModule.java#L167) isn't. Seeing as autoformat matched one and not the other, I'm inclined to think that one indentation style wasn't intentional.
Do you know which one was intended? Or are you concerned more with isolating changes for this commit?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21994966
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
> @@ -271,6 +282,10 @@ public Resource getServer() {
> return this.metadata;
> }
>
> + public Map<String, Object> getComplexMetadata() {
Where should I put that documentation? I didn't see much javadoc in this class.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21998671
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> @@ -267,7 +267,7 @@ public Resource getServer() {
> return this.server;
> }
>
> - public Map<String, String> getMetadata() {
> + public Map<String, Object> getMetadata() {
So this is the killer part of this problem. This change is backwards incompatible. If someone has
Map<String, String> metadata = image.getMetadata();
in their code (and I know image metadata is used a lot) will be broken by this.
For reference, I brought this up in the thread [Dealing with different JSON value types in a Map](http://www.mail-archive.com/dev%40jclouds.apache.org/msg05255.html) and the suggestions there were to change the method signature as well. That's something I'm not in favour of because of all of the code it would break.
Also, when you do `Map<String, Object>` you force the client to deal with `Object` every single time. Even when `Map<String, String>` is the more common case.
However, the solution space seems to be limited. One alternative is to *add* another field to `Image`
Map<String, Object> metadataWithBlockDeviceInfo;
...
Map<String, Object> getMetadataWithBlockDeviceInfo();
And deserialize to that if the metadata contains `block_device_mapping`. Then one of `metadata` or `metadataWithBlockDeviceInfo` will always be null. The client will have to either know or test to see which one it is.
Thoughts on that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21716908
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
[Checkstyle violation](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/1527/org.apache.jclouds.api$openstack-nova/violations/file/src/test/java/org/jclouds/openstack/nova/v2_0/config/ImageAdapterTest.java/)
Unused import in `ImageAdapterTest.java`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-71259153
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Ignasi Barrera <no...@github.com>.
> +
> + try {
> + NovaApi novaApi = api(server.getUrl("/").toString(), "openstack-nova");
> + ImageApi imageApi = novaApi.getImageApiForZone("RegionOne");
> +
> + FluentIterable<? extends Image> images = imageApi.listInDetail().concat();
> +
> + Image img = images.get(0);
> + assertNotNull(img.getMetadata());
> + assertEquals(11, img.getMetadata().size());
> + assertNotNull(img.getMetadata().get("block_device_mapping"));
> + } finally {
> + server.shutdown();
> + }
> + }
> +}
This "no new line at end of file" is a checkstyle violation that Jenkins is not able to properly report. Just FYI
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21687298
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> @@ -271,6 +282,10 @@ public Resource getServer() {
> return this.metadata;
> }
>
> + public Map<String, Object> getComplexMetadata() {
Put the Javadoc on both `getComplexMetadata()` and `getMetadata()`. Don't worry about the lack of Javadoc elsewhere in this class at the moment. It's these methods that are important to this PR.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r22010130
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> super(id, name, links, uuid, tenantId, userId, updated, created, hostId, accessIPv4, accessIPv6, status, null, flavor, keyName, configDrive, addresses, metadata, extendedStatus, extendedAttributes, diskConfig);
> }
> }
> }
> +
> + @Singleton
> + public static class ImageAdapter implements JsonDeserializer<Image> {
> + public static final String METADATA = "metadata";
This can be private
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21985713
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Ignasi Barrera <no...@github.com>.
Hmmm... In that case, if we are going to *keep* both forever, and now that we have a custom Json adapter for the *entire* image object, wouldn't it be better to remove the *complexMetadata* class, and promote the "block device mapping" metadata as a root level element in the Image class? The custom json adapter can take care of that and we forget about duplicate info and users having to know which accessor to use. We would then have:
```java
Map<String, String> metadata;
Map<String, String> blockDeviceMappingMetadata;
```
Which is also backwards compatible and looks more clean, as we can set proper types for each one?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-67711319
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Thanks for doing that! I thought you wanted me to test literally just that adapter, so I wouldn't have pulled in `NovaParserModule` without your pointer. I'll get a couple more test cases in there now.
How would you like me to handle the commit history? I'd guess you don't want that merge into my fork in there, so is it alright if I squash it down to one?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-70437187
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
:+1:
And note that [BlockDeviceMapping.java](https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/domain/BlockDeviceMapping.java) already exists.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-67863104
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> + }
> + ],
> + "checksum": "32c08d302f9206668030d47789b77858",
> + "min_ram": "1",
> + "disk_format": "qcow2",
> + "image_name": "Ubuntu LTS 14.04",
> + "bdm_v2": "True",
> + "image_id": "cfefefc1-eba2-4b1e-9b07-a8c74a872d65",
> + "root_device_name": "/dev/vda",
> + "container_format": "bare",
> + "min_disk": "8",
> + "size": "254149120"
> + }
> + }
> + ]
> +}
Need newline at EOF.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21986442
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Returned the original field back to `Map<String,String>` and added new field on image called `complexMetadata`. Added new `JsonDeserializer` to make sure that any non-string objects are kept out of `metadata` and `complexMetadata` keeps all values from JSON input.
Let me know if you've got any suggestions!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-66861770
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Looks like Jenkins had a failure of some sort, the build appears to have passed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-66639477
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> @@ -221,6 +229,7 @@ protected Image(String id, @Nullable String name, java.util.Set<Link> links, @Nu
> this.progress = progress;
> this.minDisk = minDisk;
> this.minRam = minRam;
> + this.blockDeviceMapping = blockDeviceMapping;
Init the same as `metadata`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r23227880
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
I think that makes a lot of sense, but a formal definition type might be better if you go that route though. A `BlockDeviceMapping` object would be a lot cleaner than a simple `Map<String, String>`.
Either way, I think I'll wait for a decision from you before I update the PR any further.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-67781978
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Wow, that's a cool Jenkins plugin! I'll get those fixed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-66658827
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Ignasi Barrera <no...@github.com>.
Sure! Forgey about my commit. It is only a proposal :) Feel free to squash and fix it as needed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-70454103
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
Closed #626.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#event-224901404
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Ignasi Barrera <no...@github.com>.
I don't like having two accessors for the same information, but this is going to be temporal. I'm OK with the change as long as we mark the deprecation and properly document it.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-67357190
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
All set!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-66674319
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
I think what @everett-toews was saying though is that `<String,String>` is always going to be more common, so it's not desirable to force casting on every call to `getMetadata()`. It might make more sense to *only* populate `complexMetadata` with non-string data, because you're always guaranteed to get something you need to cast.
What do you think?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-67393229
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> @@ -267,7 +267,7 @@ public Resource getServer() {
> return this.server;
> }
>
> - public Map<String, String> getMetadata() {
> + public Map<String, Object> getMetadata() {
+1 to future proofing the name
Have a look at one of the Adapters in [NovaParserModule.java](https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/config/NovaParserModule.java). You'll want to do something like that for the Image class. Don't forget to add it to the [provideCustomAdapterBindings()](https://github.com/jclouds/jclouds/blob/master/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/config/NovaParserModule.java#L57) method.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r21775522
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Whoops, forgot the newline in `ImageApiMockTest`. I'll wait to see if there are any other violations and fix it shortly.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-66660469
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
@monitorjbl Sorry I haven't been able to have a look at this. I've been fighting the flu.
@nacx Can you help with the unit test of just the adapter? If not, I can look at it later this week or next.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-69770083
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
Haven't heard anything for a while, any updates?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626#issuecomment-69092530
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by monitorjbl <no...@github.com>.
> + server.enqueue(addCommonHeaders(new MockResponse().setBody(stringFromResource("/access.json"))));
> + server.enqueue(addCommonHeaders(new MockResponse().setBody(stringFromResource("/image_list_with_block_device_mapping_object.json"))));
> +
> + try {
> + NovaApi novaApi = api(server.getUrl("/").toString(), "openstack-nova");
> + ImageApi imageApi = novaApi.getImageApiForZone("RegionOne");
> +
> + FluentIterable<? extends Image> images = imageApi.listInDetail().concat();
> +
> + Image img = images.get(0);
> + assertNotNull(img.getMetadata());
> + assertEquals(10, img.getMetadata().size());
> + assertNull(img.getMetadata().get("block_device_mapping"));
> + assertEquals(11, img.getComplexMetadata().size());
> + assertNotNull(img.getComplexMetadata().get("block_device_mapping"));
> + assertTrue(img.getComplexMetadata().get("block_device_mapping") instanceof Map);
Good call, I'll add that.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r22011616
Re: [jclouds] Fix for JCLOUDS-655 (#626)
Posted by Everett Toews <no...@github.com>.
> + server.enqueue(addCommonHeaders(new MockResponse().setBody(stringFromResource("/access.json"))));
> + server.enqueue(addCommonHeaders(new MockResponse().setBody(stringFromResource("/image_list_with_block_device_mapping_array.json"))));
> +
> + try {
> + NovaApi novaApi = api(server.getUrl("/").toString(), "openstack-nova");
> + ImageApi imageApi = novaApi.getImageApiForZone("RegionOne");
> +
> + FluentIterable<? extends Image> images = imageApi.listInDetail().concat();
> +
> + Image img = images.get(0);
> + assertNotNull(img.getMetadata());
> + assertEquals(10, img.getMetadata().size());
> + assertNull(img.getMetadata().get("block_device_mapping"));
> + assertEquals(11, img.getComplexMetadata().size());
> + assertNotNull(img.getComplexMetadata().get("block_device_mapping"));
> + assertTrue(img.getComplexMetadata().get("block_device_mapping") instanceof List);
Need to `assertEquals` for some of the values _within_ the block device mapping.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/626/files#r22011498