[GitHub] brooklyn-server issue #612: BROOKLYN-460: Brooklyn Camp syntax for adding ta...
Github user geomacy commented on the issue: https://github.com/apache/brooklyn-server/pull/612 +1 LGTM; have run some tests successfully, and have checked back through all of @aledsage's and @neykov's comments, which are all addressed. Will 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 #612: BROOKLYN-460: Brooklyn Camp syntax for adding ta...
Github user neykov commented on the issue: https://github.com/apache/brooklyn-server/pull/612 LGTM. Haven't checked if all of @aledsage's comments are addressed - there's at least one re `getTagsEventually` so can you double check that. A couple of minor comments from me as well. --- 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 #612: BROOKLYN-460: Brooklyn Camp syntax for adding ta...
Github user neykov commented on the issue: https://github.com/apache/brooklyn-server/pull/612 @aledsage @bostko I think we should insert the object as is, without wrapping it in `CampBrooklynTag`. The wrapper doesn't provide any useful functionality and in fact could lead to unexpected behaviour - the user is no longer able to control the type of the tag which defeats the purpose. --- 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 #612: BROOKLYN-460: Brooklyn Camp syntax for adding ta...
Github user geomacy commented on the issue: https://github.com/apache/brooklyn-server/pull/612 Given that there's lots of discussion above, can I +1 the suggestion by @m4rkmckenna to take this to the mailing list as a proposal and invite discussion there? --- 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 #612: BROOKLYN-460: Brooklyn Camp syntax for adding ta...
Github user neykov commented on the issue: https://github.com/apache/brooklyn-server/pull/612 +1 for supporting arbitrary objects, but making strings very easy to add. What about DSL in tags? This is a prerequisite for supporting objects, but on the other hand tags need to be evaluated at the time the entity is constructed which restricts DSL quite a bit. Need to be careful not to set the DSL object itself as a tag. --- 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 #612: BROOKLYN-460: Brooklyn Camp syntax for adding ta...
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/612
@bostko it's worth adding this test (which passes), for use of a DSL
expression in the tags:
```
@Test
public void testTagWithDslValue() throws Exception {
Entity app = createAndStartApplication(
"services:",
"- type: " + BasicApplication.class.getName(),
" brooklyn.tags:",
" - $brooklyn:literal(\"myval\")");
assertTrue(getTagsEventually(app).contains(BrooklynTags.newNotesTag("myval")));
}
```
---
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 #612: BROOKLYN-460: Brooklyn Camp syntax for adding ta...
Github user aledsage commented on the issue: https://github.com/apache/brooklyn-server/pull/612 @bostko (cc @tbouron) Thinking about this more, I'm not sure what types we should support for tags. Maybe accepting any object is a bad idea, as it might make subsequent features harder to support (e.g. support for looking up / processing tags). On the other hand, locking down the types to be just strings without a good reason seems too extreme. Looking at the REST api (`org.apache.brooklyn.rest.resources.EntityResource.listTags()`), it turns the tags into json via the standard mechanism. It can therefore handle non-string types. if you use something that does not have a good json representation, then it becomes a lot less useful. But arguably that's the user's problem (presumably they had a reason for putting the object in the tags in the first place). --- 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. ---
