You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2016/04/28 11:51:56 UTC
[GitHub] brooklyn-server pull request: jclouds.ImageChooser configurable
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/128
jclouds.ImageChooser configurable
This makes a custom-supplied `JcloudsLocationConfig.IMAGE_CHOOSER` customizable: if it implements `ConfigAwareChooser` then `cloneFor(ConfigBag)` will be called. This follows the same pattern as `ComputeServiceAwareChooser` (both inside `BrooklynImageChooser`).
This also adds `BrooklynImageChooser.imageChooserFromOrderings(Iterable<? extends Ordering<? super Image>>)`, so that a downstream image chooser can supply multiple orderings (the first taking priority, and subsequent ones to break any ties).
Strictly speaking, this is a break to semantic versioning to put this in the 0.9.x branch. However, the ImageChooser is quite niche. (Frankly, we should have marked `BrooklynImageChooser` as `@Beta` until it had been used in anger in more places.) The change is backwards compatible. It's just that if someone used this in a 0.9.1 and then switched back to 0.9.0 then they `ConfigAwareChooser` wouldn't be there so it wouldn't compile. On balance, I'm fine with us making that sacrifice.
The use case is...
On VMware vCloudDirector there is a customer preference for VM images that are local to the vDC (virtual datacenter) over those in other vDCs and public images. They are using imageNameRegex so jclouds can return us the corresponding image in multiple vDCs and in the public catalog. To choose the local image in a Brooklyn image chooser, we need to know where the VM is going to be provisioned. We therefore need to know the config being supplied to the `jcloudsLocation.obtain()` method *inside* an image chooser.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server fix/ImageChooser-configurable-0.9.x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/128.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #128
----
commit 0d90f5a1f53416be0059429bf673c01756d80b83
Author: Aled Sage <al...@gmail.com>
Date: 2016-04-27T22:38:48Z
Add AbstractJcloudsStubbedLiveTest.getLocationSpec
commit d8d00d88077be5e784a5f2f0f6368d0cc7e4b8ab
Author: Aled Sage <al...@gmail.com>
Date: 2016-04-28T06:41:27Z
jclouds.imageChooser customisable with provisioning config
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: jclouds.ImageChooser configurable
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/brooklyn-server/pull/128#issuecomment-215418455
Replaced by https://github.com/apache/brooklyn-server/pull/129 (which is against master). Closing this, and will cherry-pick to 0.9.x afterwards.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: jclouds.ImageChooser configurable
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage closed the pull request at:
https://github.com/apache/brooklyn-server/pull/128
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: jclouds.ImageChooser configurable
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/brooklyn-server/pull/128#issuecomment-215400629
Makes sense to backport this to `0.9.x`. I don't view that as a violation of semver because for semver `a.b.c` when `a=0` special semantics apply. We've been stricter than semver requires in this case, taking `b` is a major version and `c` a minor version. In this strict case `0.9.1` is not a point release but a minor release, meaning new features can be added so long as it doesn't break backwards compatibility.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: jclouds.ImageChooser configurable
Posted by Graeme-Miller <gi...@git.apache.org>.
Github user Graeme-Miller commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/128#discussion_r61403861
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/BrooklynImageChooser.java ---
@@ -292,27 +325,63 @@ public int compare(Image left, Image right) {
};
}
- public static class ImageChooserFromOrdering implements Function<Iterable<? extends Image>, Image>, ComputeServiceAwareChooser<ImageChooserFromOrdering> {
- final Ordering<Image> ordering;
- public ImageChooserFromOrdering(final Ordering<Image> ordering) { this.ordering = ordering; }
+ public static class ImageChooserFromOrdering implements Function<Iterable<? extends Image>, Image>,
+ ComputeServiceAwareChooser<ImageChooserFromOrdering>, ConfigAwareChooser<ImageChooserFromOrdering> {
+ final List<Ordering<? super Image>> orderings;
+
+ public ImageChooserFromOrdering(final Ordering<Image> ordering) {
+ this(ImmutableList.of(ordering));
+ }
+ public ImageChooserFromOrdering(Iterable<? extends Ordering<? super Image>> orderings) {
+ this.orderings = ImmutableList.copyOf(checkNotNull(orderings, "orderings"));
+ }
@Override
public Image apply(Iterable<? extends Image> input) {
- List<? extends Image> maxImages = multiMax(ordering, input);
+ List<? extends Image> maxImages = multiMax(Ordering.compound(orderings), input);
return maxImages.get(maxImages.size() - 1);
}
@Override
public ImageChooserFromOrdering cloneFor(ComputeService service) {
- if (ordering instanceof ComputeServiceAwareChooser) {
- return new ImageChooserFromOrdering( BrooklynImageChooser.cloneFor(ordering, service) );
+ if (Iterables.tryFind(orderings, Predicates.instanceOf(ComputeServiceAwareChooser.class)).isPresent()) {
+ List<Ordering<? super Image>> clonedOrderings = Lists.newArrayList();
+ for (Ordering<? super Image> ordering : orderings) {
+ if (ordering instanceof ComputeServiceAwareChooser) {
+ clonedOrderings.add(BrooklynImageChooser.cloneFor(ordering, service));
+ } else {
+ clonedOrderings.add(ordering);
+ }
+ }
+ return new ImageChooserFromOrdering(clonedOrderings);
+ } else {
+ return this;
+ }
+ }
+ @Override
+ public ImageChooserFromOrdering cloneFor(ConfigBag config) {
+ if (Iterables.tryFind(orderings, Predicates.instanceOf(ConfigAwareChooser.class)).isPresent()) {
+ List<Ordering<? super Image>> clonedOrderings = Lists.newArrayList();
+ for (Ordering<? super Image> ordering : orderings) {
+ if (ordering instanceof ConfigAwareChooser) {
--- End diff --
Slightly concerned about the use of instanceof here, but I can see we are using the same approach as the ComputeServiceAwareChooser
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: jclouds.ImageChooser configurable
Posted by Graeme-Miller <gi...@git.apache.org>.
Github user Graeme-Miller commented on the pull request:
https://github.com/apache/brooklyn-server/pull/128#issuecomment-215378175
Looks good to me, nothing wrong witht he additions as they are following the pattern already set down by the ComputeServiceImageChooser.
However, I do feel like this class could use a refactor at some point in the future, to split out the classes/interfaces, and possibly to remove the instanceof checks. Don't let that hold up this PR though
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request: jclouds.ImageChooser configurable
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/brooklyn-server/pull/128#issuecomment-215416610
Thanks all - will merge. The PR failed because it can't find a 0.9.1-SNAPSHOT dependency. Not sure what is up with the build (and why it needs something to pre-exist?!).
I'll close-and-reopen another PR against master (like I should have done first), and then cherry-pick it into 0.9.x.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---