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/09/21 16:59:08 UTC
[GitHub] brooklyn-server pull request #341: BROOKLYN—349: fix DSL resolution in loc...
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/341
BROOKLYN\u2014349: fix DSL resolution in location
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server BROOKLYN-349
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/341.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 #341
----
commit 0c6fcea2a72945b83556170b4879d2b1390ceb3b
Author: Aled Sage <al...@gmail.com>
Date: 2016-09-21T16:58:21Z
BROOKLYN\u2014349: fix DSL resolution in location
----
---
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 #341: BROOKLYN—349: fix DSL resolution in location
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/341
See #279 for an alternative fix (though no test - we could copy the test from here into that PR).
We'll only want one of this PR or #279 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 issue #341: BROOKLYN-349: fix DSL resolution in location
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/341
+1 for merging.
---
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 #341: BROOKLYN-349: fix DSL resolution in location
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/341
Tested by @duncangrant (for his use-case with kubernetes in clocker, which also motivated his https://github.com/apache/brooklyn-server/pull/361). Merging now.
---
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 #341: BROOKLYN-349: fix DSL resolution in locat...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/341
---
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 #341: BROOKLYN-349: fix DSL resolution in location
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/341
@ahgittin I've rebased this against master (now that #340 is merged), and confirmed `JcloudsRebindStubTest` still passes.
Can you review please?
---
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 #341: BROOKLYN-349: fix DSL resolution in location
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/341
@neykov can you review these additional commits please? Do you agree that we should merge this, and not @grkvlt 's #279?
When refactoring the tests to remove duplication, I realised that if one uses:
```
provisioning.properties:
password: $brooklyn:config("password")
```
And then subsequently calls `jcloudsSshMachineLocation.getHostname()` outside of the context of an entity, then it fails to resolve (because it doesn't know which entity to look at the config on).
I discussed the short term fix with @neykov, and applied it here: it avoids propagating the exception if the credentials can't be obtained, and it only look up the credentials if we really need them (e.g. for hostname on AWS, where we ask the VM for it).
---
I also discussed the longer term solution with @neykov. Conclusion is that it's hard. And that we should resolve the DSL config's context at the time the entity is being instantiated (e.g. storing it as a field on the `DslConfigSupplier` or whatever). This would mean we'd always use the entity on which the config was declared, rather than relying on who is looking it up. However, there is (probably?!) an exception to that rule: if you declare config with an `@brooklyn:entitySpec`, such as for a `memberSpec`, then you probably want it to refer to the context of the entity to be created rather than the entity where you are declaring the entitySpec (and therefore if setting the entity context on the DSL object, you'd need to ensure it is a different DSL object instance for each member instantiated using the entitySpec!).
That deserves its own thread on dev@brooklyn.
---
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 #341: BROOKLYN-349: fix DSL resolution in location
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/341
The changes make sense. I think they will work even if the DSL is set on the location object, nothing specific to `provisioning.properties` here. Worth a test case to confirm?
The result still depends on the context where evaluation happens, not where it's set. Or may be not, not sure how #340 interacts with this and locations specifically.
Good to merge.
---
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 #341: BROOKLYN-349: fix DSL resolution in location
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/341
Can we merge #340 and then look at this? It gives clearer methods for dealing with semantics of copying resolved / unresolved semantics, and it moves `Location`'s config to be treated like others. Many of your changes here can still work but some may need changing to fit with how it works.
In particular note that with #340 the `ConfigBag` exposed by `AbstractLocation` is an RO view. (Not sure if that matters.)
---
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.
---