You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2014/05/19 18:09:48 UTC
[jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs as
IDs (#63)
Use slugs as IDs and assume a node can have a `null` image if it used an imageId that has been removed.
With this change the objects in the portable model will have the "slug" in the ID field and the real ID in the "providerId" field. Private images, which don't have a slug, will also have the real ID populated in the ID field. This will allow users to use the ComputeService using the slugs, which won't change, to search for images using portable code.
This pull request supercedes https://github.com/jclouds/jclouds-labs/pull/58 to properly use the new slug fields provided by the provider api.
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds-labs slug-ids
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/63
-- Commit Summary --
* JCLOUDS-543/JCLOUDS-572: Use slugs as IDs and assume a node can have a null image if it used an imageId that has been removed
-- File Changes --
M digitalocean/pom.xml (2)
M digitalocean/src/main/java/org/jclouds/digitalocean/compute/extensions/DigitalOceanImageExtension.java (10)
M digitalocean/src/main/java/org/jclouds/digitalocean/compute/functions/DropletToNodeMetadata.java (61)
M digitalocean/src/main/java/org/jclouds/digitalocean/compute/functions/ImageToImage.java (4)
M digitalocean/src/main/java/org/jclouds/digitalocean/compute/functions/SizeToHardware.java (3)
M digitalocean/src/main/java/org/jclouds/digitalocean/compute/strategy/DigitalOceanComputeServiceAdapter.java (6)
M digitalocean/src/main/java/org/jclouds/digitalocean/domain/Size.java (7)
M digitalocean/src/main/java/org/jclouds/digitalocean/features/DropletApi.java (36)
M digitalocean/src/main/java/org/jclouds/digitalocean/features/ImageApi.java (44)
M digitalocean/src/test/java/org/jclouds/digitalocean/compute/functions/DropletToNodeMetadataTest.java (29)
M digitalocean/src/test/java/org/jclouds/digitalocean/compute/functions/ImageToImageTest.java (5)
M digitalocean/src/test/java/org/jclouds/digitalocean/compute/functions/SizeToHardwareTest.java (5)
M digitalocean/src/test/java/org/jclouds/digitalocean/features/DropletApiLiveTest.java (20)
M digitalocean/src/test/java/org/jclouds/digitalocean/features/DropletApiMockTest.java (52)
M digitalocean/src/test/java/org/jclouds/digitalocean/features/ImageApiLiveTest.java (36)
M digitalocean/src/test/java/org/jclouds/digitalocean/features/ImageApiMockTest.java (54)
M digitalocean/src/test/resources/image1.json (2)
M digitalocean/src/test/resources/image2.json (2)
M digitalocean/src/test/resources/images.json (4)
M digitalocean/src/test/resources/sizes.json (8)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/63.patch
https://github.com/jclouds/jclouds-labs/pull/63.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> @@ -113,4 +111,43 @@ public boolean apply(Location location) {
>
> return builder.build();
> }
> +
> + private Optional<? extends Image> findImage(Integer id) {
> + // Try to find the image by ID in the cache. The cache is indexed by slug (for public images) and by id (for
> + // private ones).
> + final String imageId = String.valueOf(id);
> + Optional<? extends Image> image = Optional.fromNullable(images.get().get(imageId));
> + if (!image.isPresent()) {
> + // If it is a public image (indexed by slug) but the "int" form of the id was provided, try to find it in the
> + // whole list of cached images
> + image = tryFind(images.get().values(), new Predicate<Image>() {
> + @Override
> + public boolean apply(Image input) {
> + return input.getProviderId().equals(imageId);
"image provider ID == image ID"? Is that correct?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12805638
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by BuildHive <no...@github.com>.
[jclouds ยป jclouds-labs #1059](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/1059/) 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-labs/pull/63#issuecomment-43523906
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> @@ -116,7 +117,14 @@ public boolean apply(org.jclouds.digitalocean.domain.Image input) {
> @Override
> public boolean deleteImage(String id) {
> try {
> - api.getImageApi().delete(Integer.parseInt(id));
> + // The id of the image can be an id or a slug. Use the corresponding method of the API depending on what is
> + // provided. If it can be parsed as a number, use the method to destroy by ID. Otherwise, destroy by slug.
Is that "algorithm" guaranteed to work? Or are there also numeric slugs?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12805538
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> @@ -169,7 +170,10 @@ public boolean apply(Droplet droplet) {
>
> @Override
> public Image getImage(String id) {
> - return api.getImageApi().get(Integer.parseInt(id));
> + // The id of the image can be an id or a slug. Use the corresponding method of the API depending on what is
> + // provided. If it can be parsed as a number, use the method to get by ID. Otherwise, get by slug.
> + Integer imageId = Ints.tryParse(id);
> + return imageId != null ? api.getImageApi().get(imageId) : api.getImageApi().get(id);
> Did you have something in mind?
Not specifically. We don't normally do "smart" API methods beyond what the underlying API provides, indeed. So let's leave as-is.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r13060567
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Ignasi Barrera <no...@github.com>.
Squashed and pushed to [master](https://git-wip-us.apache.org/repos/asf?p=jclouds-labs.git;a=commit;h=c5afe3cc4af9819f71a12aae74444090955f9d9a).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63#issuecomment-43724313
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -116,7 +117,14 @@ public boolean apply(org.jclouds.digitalocean.domain.Image input) {
> @Override
> public boolean deleteImage(String id) {
> try {
> - api.getImageApi().delete(Integer.parseInt(id));
> + // The id of the image can be an id or a slug. Use the corresponding method of the API depending on what is
> + // provided. If it can be parsed as a number, use the method to destroy by ID. Otherwise, destroy by slug.
I think it will work, as there are not numeric slugs (and I don't expect there to be). This is the entire list of slugs:
```bash
$ curl "https://api.digitalocean.com/v1/images?client_id=<client_id>&api_key=<cpi_key>" | json_pp | grep '"slug"' | cut -d\" -f4 | sort | column
arch-linux-2013-05-x32 debian-6-0-x64 gitlab-ubuntu-13.10 ubuntu-12-10-x32
arch-linux-2013-05-x64 debian-7-0-x32 lamp-ubuntu-12.04 ubuntu-12-10-x64
centos-5-8-x32 debian-7-0-x64 mean-ubuntu-12.04 ubuntu-12-10-x64-desktop
centos-5-8-x64 dokku-ubuntu-14.04 rails-unicorn-nginx-ubuntu-12.10 ubuntu-13-10-x32
centos-6-4-x32 fedora-19-x32 redmine-ubuntu-12.04 ubuntu-13-10-x64
centos-6-4-x64 fedora-19-x64 ubuntu-10-04-x32 ubuntu-14-04-x32
centos-6-5-x32 fedora-20-x32 ubuntu-10-04-x64 ubuntu-14-04-x64
centos-6-5-x64 fedora-20-x64 ubuntu-12-04-x32 wordpress-ubuntu-13.10
debian-6-0-x32 ghost-ubuntu-12.04 ubuntu-12-04-x64 null
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12831366
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> @@ -40,7 +40,9 @@
> @Override
> public org.jclouds.compute.domain.Image apply(final Image input) {
> ImageBuilder builder = new ImageBuilder();
> - builder.ids(String.valueOf(input.getId()));
> + // Private images don't have a slug
> + builder.id(input.getSlug() != null? input.getSlug() : String.valueOf(input.getId()));
[minor] Space before `?`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12805708
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> @@ -71,6 +72,19 @@
> Image get(@PathParam("id") int id);
>
> /**
> + * Gets the details of the given image.
> + *
> + * @param slug The slug of the image to get.
> + * @return The details of the image or <code>null</code> if no image exists with the given slug.
> + */
> + @Named("image:get")
> + @GET
> + @Path("/{slug}")
> + @SelectJson("image")
> + @Fallback(NullOnNotFoundOr404.class)
> + Image get(@PathParam("slug") String slug);
Mark as `@Nullable`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12805867
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> @@ -169,7 +170,10 @@ public boolean apply(Droplet droplet) {
>
> @Override
> public Image getImage(String id) {
> - return api.getImageApi().get(Integer.parseInt(id));
> + // The id of the image can be an id or a slug. Use the corresponding method of the API depending on what is
> + // provided. If it can be parsed as a number, use the method to get by ID. Otherwise, get by slug.
> + Integer imageId = Ints.tryParse(id);
> + return imageId != null ? api.getImageApi().get(imageId) : api.getImageApi().get(id);
This kind of pattern appears at least twice...add as a convenience method to ImageApi?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12805787
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> + @Named("droplet:create")
> + @GET
> + @Path("/new")
> + @SelectJson("droplet")
> + DropletCreation create(@QueryParam("name") String name, @QueryParam("image_slug") String imageSlug,
> + @QueryParam("size_slug") String sizeSlug, @QueryParam("region_slug") String regionSlug);
> +
> + /**
> + * Creates a new droplet.
> + *
> + * @param name The name for the new droplet.
> + * @param imageSlug The slug of the image to use to create the droplet.
> + * @param sizeSlug The slug of the size to use to create the droplet.
> + * @param regionSlug The slug region where the droplet must be created.
> + * @param options Custom options to create the droplet.
> + * @return The created droplet.
Add a `@see` pointing to the other variant of this call?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12805838
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> @@ -116,7 +117,14 @@ public boolean apply(org.jclouds.digitalocean.domain.Image input) {
> @Override
> public boolean deleteImage(String id) {
> try {
> - api.getImageApi().delete(Integer.parseInt(id));
> + // The id of the image can be an id or a slug. Use the corresponding method of the API depending on what is
> + // provided. If it can be parsed as a number, use the method to destroy by ID. Otherwise, destroy by slug.
> + Integer imageId = Ints.tryParse(id);
> + if (imageId != null) {
> + api.getImageApi().delete(imageId);
> + } else {
> + api.getImageApi().delete(id);
Would this be a good place for some debug logging?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12805564
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -169,7 +170,10 @@ public boolean apply(Droplet droplet) {
>
> @Override
> public Image getImage(String id) {
> - return api.getImageApi().get(Integer.parseInt(id));
> + // The id of the image can be an id or a slug. Use the corresponding method of the API depending on what is
> + // provided. If it can be parsed as a number, use the method to get by ID. Otherwise, get by slug.
> + Integer imageId = Ints.tryParse(id);
> + return imageId != null ? api.getImageApi().get(imageId) : api.getImageApi().get(id);
This is also seen int he `ImageExtension.deleteImage` method.
The thing here is that the `ImageApi` is just the API interface with the REST annotations and there is no implementation. It provides two methods, one accepting an integer (the id) and one accepting a string (the slug), and I see no elegant way to provide that convenience method. Did you have something in mind?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12831872
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> @@ -116,7 +117,14 @@ public boolean apply(org.jclouds.digitalocean.domain.Image input) {
> @Override
> public boolean deleteImage(String id) {
> try {
> - api.getImageApi().delete(Integer.parseInt(id));
> + // The id of the image can be an id or a slug. Use the corresponding method of the API depending on what is
> + // provided. If it can be parsed as a number, use the method to destroy by ID. Otherwise, destroy by slug.
> as there are not numeric slugs (and I don't expect there to be)
That's good enough for me, then ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r13060521
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> + *
> + * @author Ignasi Barrera
> + */
> +public class LocationNamingUtils {
> +
> + /**
> + * Extracts the region id for the given location.
> + *
> + * @param location The location to extract the region id from.
> + * @return The id of the region.
> + */
> + public static int extractRegionId(Location location) {
> + checkNotNull(location, "location cannot be null");
> + String regionIdAndName = location.getDescription();
> + int index = regionIdAndName.indexOf('/');
> + checkArgument(index >= 0, "location description should be in the form 'regionId/regionName'");
> Done in 68
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r13318509
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Ignasi Barrera <no...@github.com>.
> + *
> + * @author Ignasi Barrera
> + */
> +public class LocationNamingUtils {
> +
> + /**
> + * Extracts the region id for the given location.
> + *
> + * @param location The location to extract the region id from.
> + * @return The id of the region.
> + */
> + public static int extractRegionId(Location location) {
> + checkNotNull(location, "location cannot be null");
> + String regionIdAndName = location.getDescription();
> + int index = regionIdAndName.indexOf('/');
> + checkArgument(index >= 0, "location description should be in the form 'regionId/regionName'");
Done in https://github.com/jclouds/jclouds-labs/pull/68
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r13223578
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> @@ -71,6 +72,19 @@
> Image get(@PathParam("id") int id);
Mark as `@Nullable`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12805854
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Ignasi Barrera <no...@github.com>.
I've addressed the comments and added a couple improvements:
* Extracted the region id+name encoding logic into a `LocationNamingUtils` class, to avoid exposing it in many places.
* Removed an unnecessary call to the provider in the `ComputeServiceAdapter` to get the details of the location, and used the cached locations instead.
All live tests are passing.
```bash
Tests run: 56, Failures: 0, Errors: 0, Skipped: 0
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 34:48 min (Wall Clock)
[INFO] Finished at: 2014-05-20T12:52:41+01:00
[INFO] Final Memory: 29M/618M
[INFO] ------------------------------------------------------------------------
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63#issuecomment-43611387
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
Some minor comments but overall looks good to me. Do we have any live test results for this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63#issuecomment-43553042
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -113,4 +111,43 @@ public boolean apply(Location location) {
>
> return builder.build();
> }
> +
> + private Optional<? extends Image> findImage(Integer id) {
> + // Try to find the image by ID in the cache. The cache is indexed by slug (for public images) and by id (for
> + // private ones).
> + final String imageId = String.valueOf(id);
> + Optional<? extends Image> image = Optional.fromNullable(images.get().get(imageId));
> + if (!image.isPresent()) {
> + // If it is a public image (indexed by slug) but the "int" form of the id was provided, try to find it in the
> + // whole list of cached images
> + image = tryFind(images.get().values(), new Predicate<Image>() {
> + @Override
> + public boolean apply(Image input) {
> + return input.getProviderId().equals(imageId);
Right.
* jclouds Image ID = DigitalOcean Image slug (or image ID if the image is private or does not have a slug associated).
* jclouds image Provider ID = DigitalOcean Image ID.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12831460
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Ignasi Barrera <no...@github.com>.
Closed #63.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63#event-123239664
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #155](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/155/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63#issuecomment-43525843
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> + *
> + * @author Ignasi Barrera
> + */
> +public class LocationNamingUtils {
> +
> + /**
> + * Extracts the region id for the given location.
> + *
> + * @param location The location to extract the region id from.
> + * @return The id of the region.
> + */
> + public static int extractRegionId(Location location) {
> + checkNotNull(location, "location cannot be null");
> + String regionIdAndName = location.getDescription();
> + int index = regionIdAndName.indexOf('/');
> + checkArgument(index >= 0, "location description should be in the form 'regionId/regionName'");
Here and below: add something like "but was '%s'" to the end of the error message?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r13060572
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Andrew Phillips <no...@github.com>.
> @@ -113,4 +111,43 @@ public boolean apply(Location location) {
>
> return builder.build();
> }
> +
> + private Optional<? extends Image> findImage(Integer id) {
> + // Try to find the image by ID in the cache. The cache is indexed by slug (for public images) and by id (for
> + // private ones).
> + final String imageId = String.valueOf(id);
> + Optional<? extends Image> image = Optional.fromNullable(images.get().get(imageId));
> + if (!image.isPresent()) {
> + // If it is a public image (indexed by slug) but the "int" form of the id was provided, try to find it in the
> + // whole list of cached images
> + image = tryFind(images.get().values(), new Predicate<Image>() {
> + @Override
> + public boolean apply(Image input) {
> + return input.getProviderId().equals(imageId);
OK, thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r13060527
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #156](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/156/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63#issuecomment-43612618
Re: [jclouds-labs] JCLOUDS-543/JCLOUDS-572: Add support to use slugs
as IDs (#63)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -116,7 +117,14 @@ public boolean apply(org.jclouds.digitalocean.domain.Image input) {
> @Override
> public boolean deleteImage(String id) {
> try {
> - api.getImageApi().delete(Integer.parseInt(id));
> + // The id of the image can be an id or a slug. Use the corresponding method of the API depending on what is
> + // provided. If it can be parsed as a number, use the method to destroy by ID. Otherwise, destroy by slug.
> + Integer imageId = Ints.tryParse(id);
> + if (imageId != null) {
> + api.getImageApi().delete(imageId);
> + } else {
> + api.getImageApi().delete(id);
Sure. Will also add some debug logging to the compute functions, when an image is no longer available.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/63/files#r12831391