You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by alasdairhodge <gi...@git.apache.org> on 2015/12/08 11:51:41 UTC
[GitHub] incubator-brooklyn pull request: Fix/deferred location config
GitHub user alasdairhodge opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/1093
Fix/deferred location config
Resolves issues with `$brooklyn:external()` in location config:
* `TypeCoercions` resolves `DeferredSupplier` values
* locations constructed within a new execution context on `LocalLocationManager` so that relevant context objects can be retrieved in a consistent way
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alasdairhodge/incubator-brooklyn fix/deferred-location-config
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/1093.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 #1093
----
commit ba69dba8a3d45a78a64e7367553b1941966bf430
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2015-12-02T18:19:05Z
Resolve DeferredSuppliers when coercing config values.
commit b2302dce07da3461247ac073b1daf9116203f96d
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2015-12-02T18:20:13Z
Instantiate locations in a task context, to permit resolution of certain DeferredSupplier config.
commit 7b7e0e0dddbb1738054b25f99a8f32d8d38a5a06
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2015-12-07T12:46:12Z
Fix JcloudsLocation to use type-coercing get() method.
commit c9c7c6f23fc6eebb5c63ece64fd167b92fdef6f1
Author: Alasdair Hodge <gi...@alasdairhodge.co.uk>
Date: 2015-12-08T10:22:59Z
Fix test assertions in response to task-based location creation.
----
---
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] incubator-brooklyn pull request: Fix/deferred location config
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1093#issuecomment-177053294
@alasdairhodge ping
---
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] incubator-brooklyn pull request: Fix/deferred location config
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1093#discussion_r47370769
--- Diff: core/src/test/java/org/apache/brooklyn/core/location/LocationExtensionsTest.java ---
@@ -164,21 +166,24 @@ public void testAddExtensionThroughLocationSpecIllegally() {
try {
Location loc = createConcrete(MyExtension.class, "not an extension");
fail("loc="+loc);
- } catch (IllegalArgumentException e) {
+ } catch (Exception e) {
+ assertTrue(Exceptions.getFirstInteresting(e) instanceof IllegalArgumentException);
--- End diff --
I prefer that `e` is rethrown if it doesn't match, rather than losing the exception in the error report. Alex (I think it was) added:
Asserts.expectedFailureOfType(e, IllegalArgumentException.class)
But not sure if that deals with looking at the `causedBy` properly.
---
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] incubator-brooklyn pull request: Fix/deferred location config
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1093#issuecomment-163975440
@alasdairhodge Looks good. Only one important comment about reusing the `ManagementContext`s ExecutionManager, rather than creating a new one.
---
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] incubator-brooklyn pull request: Fix/deferred location config
Posted by alasdairhodge <gi...@git.apache.org>.
Github user alasdairhodge commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1093#issuecomment-163601551
Once this is reviewed/merged into `master`, I'll issue the same fix against `0.8.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.
---
[GitHub] incubator-brooklyn pull request: Fix/deferred location config
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1093#issuecomment-163981939
How about a test like this in `ExternalConfigYamlTest`:
@Test
public void testExternalisedLocationConfigAccessedDuringLocationInit() throws Exception {
String yaml = Joiner.on("\n").join(
"services:",
"- type: org.apache.brooklyn.core.test.entity.TestApplication",
"location:",
" aws-ec2:",
" identity: myidentity",
" credential: mycredential",
" region: $brooklyn:external(\"myprovider\", \"mykey\")");
TestApplication app = (TestApplication) createAndStartApplication(new StringReader(yaml));
waitForApplicationTasks(app);
LocationInternal loc = (LocationInternal) Iterables.getOnlyElement( app.getLocations() );
assertEquals(loc.config().getLocalBag().getDescription(), "aws-ec2:myHardcodedVal");
}
Does that now pass for you? It fails for me in master:
org.apache.brooklyn.util.exceptions.CompoundRuntimeException: Unable to instantiate item, 2 errors including: Transformer for brooklyn-camp gave an error creating this plan: Illegal parameter for 'location' (aws-ec2); not resolvable: java.lang.ClassCastException: org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.BrooklynDslCommon$DslExternal cannot be cast to java.lang.String
---
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] incubator-brooklyn pull request: Fix/deferred location config
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1093#issuecomment-165434986
@alasdairhodge any progress on including that test and getting it to pass? Ping me if you want to discuss or pair.
---
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] incubator-brooklyn pull request: Fix/deferred location config
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/1093#discussion_r47369700
--- Diff: core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalLocationManager.java ---
@@ -78,9 +85,13 @@
public LocalLocationManager(LocalManagementContext managementContext) {
this.managementContext = checkNotNull(managementContext, "managementContext");
this.locationFactory = new InternalLocationFactory(managementContext);
-
+
this.storage = managementContext.getStorage();
- locationTypes = storage.getMap("locations");
+ this.locationTypes = storage.getMap("locations");
+
+ BasicExecutionManager executionManager = new BasicExecutionManager(managementContext.getManagementNodeId());
--- End diff --
Do we really need to create our own `BasicExecutionManager`? Can we not reuse `managementContext.getExecutionManager()`?
---
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] incubator-brooklyn pull request: Fix/deferred location config
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1093#issuecomment-172890614
@alasdairhodge ping
---
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] incubator-brooklyn pull request: Fix/deferred location config
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/1093#issuecomment-163977089
Actually, can we have a test that externalised configuration now works in locations? As I recall, it was only in one situation that it failed - when the config was accessed during the `init` of the location object? For example in `JcloudsLocation.setCreationString()`, which is called by `JcloudsLocation.configure()`?
---
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.
---