You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by grkvlt <gi...@git.apache.org> on 2016/07/29 07:05:26 UTC
[GitHub] brooklyn-server pull request #279: Allow provisioning.properties to referenc...
GitHub user grkvlt opened a pull request:
https://github.com/apache/brooklyn-server/pull/279
Allow provisioning.properties to reference resolvable config
Enables constructions like the following:
```YAML
- type: vanilla-software-process
brooklyn.config:
provisioning.properties:
minRam: $brooklyn:scopeRoot().config("entity.min.ram")
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/grkvlt/brooklyn-server resolve-jclouds-location-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/279.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 #279
----
commit 3497e9a4c529871e6546c0b33027055d9ef70919
Author: Andrew Donald Kennedy <an...@cloudsoftcorp.com>
Date: 2016-07-29T05:02:21Z
Allow provisioning.properties to reference resolvable 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 #279: Allow provisioning.properties to referenc...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/279#discussion_r79716634
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -49,6 +50,68 @@
import javax.annotation.Nullable;
import javax.xml.ws.WebServiceException;
+import org.apache.commons.lang3.ArrayUtils;
--- End diff --
BTW, my ordering was to keep the `org.apache.brooklyn` and `io.brooklyn` and `brooklyn` imports at the bottom, with more generic imports above. But, whatever, am happy to use the standard, have changed my IDE 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 #279: Allow provisioning.properties to referenc...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt closed the pull request at:
https://github.com/apache/brooklyn-server/pull/279
---
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 #279: Allow provisioning.properties to referenc...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/279#discussion_r78161634
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -49,6 +50,68 @@
import javax.annotation.Nullable;
import javax.xml.ws.WebServiceException;
+import org.apache.commons.lang3.ArrayUtils;
--- End diff --
@grkvlt just came across another import rearrange you did in 33b0b1c879a9ed07bcdc3138893cd8017e211cfa
can you configure your import order as per
http://brooklyn.apache.org/v/latest/dev/env/ide/index.html#intellij-idea ?
---
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 #279: Allow provisioning.properties to referenc...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/279#discussion_r74495798
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -49,6 +50,68 @@
import javax.annotation.Nullable;
import javax.xml.ws.WebServiceException;
+import org.apache.commons.lang3.ArrayUtils;
--- End diff --
Can we avoid re-ordering all the imports?
---
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 issue #279: Allow provisioning.properties to reference resol...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/279
@grkvlt you said that you'd "add some more [tests] to illustrate the use of resolvable entity config in a jclouds location context". Is that something you plan to do for this PR? Do you know when you'll have time to do that?
---
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 issue #279: Allow provisioning.properties to reference resol...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/279
I think this resolves and stores config too early. We need to ensure the raw unresolved values are given to the `JcloudsSshMachineLocation` (and a few other places). See https://github.com/apache/brooklyn-server/pull/194.
Can you check whether `ExternalConfigYamlTest` passes? They are integration tests (probably shouldn't be!) but I think would illustrate why we can't just resolve everything.
The use case you introduce @grkvlt is useful. I think we should handle this either by ensuring we resolve each access eg in `buildTemplate` or by maintaining two maps in the code path, `newItemConfigResolved` and `newItemConfigRaw` (instead of `setup`).
Also agree we want a test for this and please undo the huge mad import order change (under what ordering is the new one more sensible??).
Note that the semantics flip once a location (or any adjunct or entity) is managed, `BrooklynObject.config().get()` returns resolved values. A more elegant but much bigger fix might be to create the `JcloudsSshMachineLocation` early, pass it unresolved config, and then for it to call its own config at which point resolving on access would be a sensible default. This makes it more like entities (which tend to create the thing they represent, as opposed to the parent creating the thing first which is what `JcloudsLocation` is doing).
---
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 #279: Allow provisioning.properties to referenc...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/279#discussion_r72820800
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -527,7 +531,7 @@ public ComputeService getComputeService(Map<?,?> flags) {
public ComputeService getComputeService(ConfigBag config) {
ComputeServiceRegistry registry = getConfig(COMPUTE_SERVICE_REGISTRY);
- return registry.findComputeService(ResolvingConfigBag.newInstanceExtending(getManagementContext(), config), true);
--- End diff --
Why are you removing the `ResolvingConfigBag`?
---
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 #279: Allow provisioning.properties to referenc...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/279#discussion_r72888151
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -527,7 +531,7 @@ public ComputeService getComputeService(Map<?,?> flags) {
public ComputeService getComputeService(ConfigBag config) {
ComputeServiceRegistry registry = getConfig(COMPUTE_SERVICE_REGISTRY);
- return registry.findComputeService(ResolvingConfigBag.newInstanceExtending(getManagementContext(), config), true);
--- End diff --
By the time `getComputeService()` is called, I will already have set up the `ConfigBag` to be a resolving config, see `private ConfigBag getConfigBag(Map<?, ?> flags)` method below. The addition of a new `ResolvingConfigBag` was redundant. In essence I'm solving the same problem as **BROOKLYN-249** and also adding the ability to resolve config through the calling entity context, if it exists.
---
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 #279: Allow provisioning.properties to referenc...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/279#discussion_r74532002
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -49,6 +50,68 @@
import javax.annotation.Nullable;
import javax.xml.ws.WebServiceException;
+import org.apache.commons.lang3.ArrayUtils;
--- End diff --
But this way is more logically defensible!
---
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 issue #279: Allow provisioning.properties to reference resol...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/279
@grkvlt test failures look plausibly related: failures in `JcloudsLocationTest`.
---
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 issue #279: Allow provisioning.properties to reference resol...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the issue:
https://github.com/apache/brooklyn-server/pull/279
@aledsage I've fixed the failing tests (there may not be a caller context, so do not rely on its existence) and will add some more to illustrate the use of resolvable entity config in a jclouds location context.
---
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 issue #279: Allow provisioning.properties to reference resol...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/279
The jenkins test failures look plausibly related: `JcloudsLocationTest` tests are failing.
@grkvlt please add test(s) for this. See the existing tests like those in `JcloudsLocationExternalConfigYamlTest`. Can you also confirm that those live tests work (they don't create any VMs, but they go through talking to the cloud for the list of images etc to create the `Template`).
---
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 #279: Allow provisioning.properties to referenc...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/279#discussion_r72821487
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -527,7 +531,7 @@ public ComputeService getComputeService(Map<?,?> flags) {
public ComputeService getComputeService(ConfigBag config) {
ComputeServiceRegistry registry = getConfig(COMPUTE_SERVICE_REGISTRY);
- return registry.findComputeService(ResolvingConfigBag.newInstanceExtending(getManagementContext(), config), true);
--- End diff --
This was added to fix "BROOKLYN-249: JcloudsLocation resolves external config" (see commit 99dd3ba4fc507988bdecd4e57c2b36cbb465b2a5)
---
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 issue #279: Allow provisioning.properties to reference resol...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/279
@grkvlt https://github.com/apache/brooklyn-server/pull/341 is now merged, so this PR is no longer needed. Can you close it please (assuming you agree)?
---
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 #279: Allow provisioning.properties to referenc...
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/279#discussion_r79714457
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -49,6 +50,68 @@
import javax.annotation.Nullable;
import javax.xml.ws.WebServiceException;
+import org.apache.commons.lang3.ArrayUtils;
--- End diff --
I have done this for all my Brooklyn projects now, thanks for the pointer, @ahgittin
---
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 issue #279: Allow provisioning.properties to reference resol...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/279
See https://issues.apache.org/jira/browse/BROOKLYN-349 for a description of the underlying problem.
Also see https://github.com/apache/brooklyn-server/pull/341 for an alternative fix and a test for that fix. In PR #341 the resolution is deferred until the value is actually requested. This was a bit more fiddly to implement, but more because it highlighted a problem where the `DslComponent`'s task lost the context of the entity that called it.
We'll only want one of this PR or #341 to be merged, I think. @grkvlt @ahgittin @neykov I'm keen to hear your thoughts on it.
---
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 #279: Allow provisioning.properties to referenc...
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/279#discussion_r78140278
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java ---
@@ -603,21 +607,37 @@ public MachineLocation obtain(Map<?,?> flags, TemplateBuilder tb) throws NoMachi
return obtain(MutableMap.builder().putAll(flags).put(TEMPLATE_BUILDER, tb).build());
}
- /** core method for obtaining a VM using jclouds;
+ private ConfigBag getConfigBag(Map<?, ?> flags) {
--- End diff --
call this `getConfigBagExtendedWithFlagsResolved` ?
---
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.
---