[GitHub] brooklyn-server issue #341: BROOKLYN-349: fix DSL resolution in location
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #341: BROOKLYN-349: fix DSL resolution in location
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #341: BROOKLYN-349: fix DSL resolution in location
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #341: BROOKLYN-349: fix DSL resolution in location
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #341: BROOKLYN-349: fix DSL resolution in location
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #341: BROOKLYN-349: fix DSL resolution in location
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] brooklyn-server issue #341: BROOKLYN—349: fix DSL resolution in location
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
