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/03 05:50:30 UTC
[GitHub] brooklyn-server pull request #229: Merge brooklyn.parameters in YAML files
GitHub user grkvlt opened a pull request:
https://github.com/apache/brooklyn-server/pull/229
Merge brooklyn.parameters in YAML files
Implements behaviour required to merge `brooklyn.parameters` sections of specifications in YAML files, allowing addition of new parameters to entities without having to specify the complete list again.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/grkvlt/brooklyn-server merge-brooklyn-parameters
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/229.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 #229
----
commit 96f4cb6ca7f45b619eeb3fe70c1d8457b755296e
Author: Andrew Donald Kennedy <an...@cloudsoftcorp.com>
Date: 2016-07-03T00:24:38Z
Merge brooklyn.parameters in YAML files
----
---
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 #229: Merge brooklyn.parameters in YAML files
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/229#discussion_r69448156
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,73 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Returns true if the {@link SpecParameter parameter name} is the same as
+ * that on the specified paramater.
+ */
+ protected static Predicate<SpecParameter<?>> sameName(final SpecParameter<?> param) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getConfigKey().getName().equals(param.getConfigKey().getName());
+ }
+ };
+ }
+
+ /**
+ * Returns true if the {@link SpecParameter#getLabel() label} is the same as
+ * the specified string.
+ */
+ public static Predicate<SpecParameter<?>> labelEqualTo(final String label) {
+ return new Predicate<SpecParameter<?>>() {
--- End diff --
Don't use anonymous inner classes, in case they get persisted.
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/229
+1 for the change. I've personally felt the pain of not being able to merge parameters.
I think that we need to discuss with a broader audience how the merge happens - suggest mailing to the dev list.
What I'm concerned about is mostly how the parameters look in the UI when merged so the input of blueprint authors will be very valuable. Should override parameters be appended or prepended? Should they be merged with those coming from the class?
---
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 #229: Merge brooklyn.parameters in YAML files
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/229#discussion_r69386810
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java ---
@@ -173,17 +173,128 @@ public void testDepentantCatalogsOverrideParameters(Class<? extends BrooklynObje
" - simple",
" - id: " + SYMBOLIC_NAME,
" item:",
- // Don't set explicit version, not supported by locations
" type: paramItem",
" brooklyn.parameters:",
" - override");
CatalogItem<?, ?> item = catalog.getCatalogItem(SYMBOLIC_NAME, TEST_VERSION);
AbstractBrooklynObjectSpec<?,?> spec = createSpec(item);
- List<SpecParameter<?>> inputs = spec.getParameters();
- assertEquals(inputs.size(), 1);
- SpecParameter<?> firstInput = inputs.get(0);
- assertEquals(firstInput.getLabel(), "override");
+ List<SpecParameter<?>> params = spec.getParameters();
+ assertEquals(params.size(), 2);
+ assertTrue(Iterables.tryFind(params, nameEqualTo("simple")).isPresent());
+ assertTrue(Iterables.tryFind(params, nameEqualTo("override")).isPresent());
+ }
+
+ @Test(dataProvider="brooklynTypes")
+ public void testDependantCatalogMergesParameters(Class<? extends BrooklynObject> type) {
+ addCatalogItems(
+ "brooklyn.catalog:",
+ " version: " + TEST_VERSION,
+ " items:",
+ " - id: paramItem",
+ " item:",
+ " type: " + type.getName(),
+ " brooklyn.parameters:",
+ " - name: simple",
+ " label: simple",
+ " - id: " + SYMBOLIC_NAME,
+ " item:",
+ " type: paramItem",
+ " brooklyn.parameters:",
+ " - name: simple",
+ " label: override");
+
+ CatalogItem<?, ?> item = catalog.getCatalogItem(SYMBOLIC_NAME, TEST_VERSION);
+ AbstractBrooklynObjectSpec<?,?> spec = createSpec(item);
+ List<SpecParameter<?>> params = spec.getParameters();
+ assertEquals(params.size(), 1);
+ assertTrue(Iterables.tryFind(params, nameEqualTo("simple")).isPresent());
+ assertTrue(Iterables.tryFind(params, labelEqualTo("override")).isPresent());
+ }
+
+ @Test
+ public void testDependantCatalogConfigOverridesParameters() {
--- End diff --
Note only for entities, as `brooklyn.config` is split into flags and config for policies and locations, so the parameter logic does not allow detection of existing config here.
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/229
---
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 #229: Merge brooklyn.parameters in YAML files
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/229#discussion_r69448257
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,73 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Returns true if the {@link SpecParameter parameter name} is the same as
+ * that on the specified paramater.
+ */
+ protected static Predicate<SpecParameter<?>> sameName(final SpecParameter<?> param) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getConfigKey().getName().equals(param.getConfigKey().getName());
+ }
+ };
+ }
+
+ /**
+ * Returns true if the {@link SpecParameter#getLabel() label} is the same as
+ * the specified string.
+ */
+ public static Predicate<SpecParameter<?>> labelEqualTo(final String label) {
+ return new Predicate<SpecParameter<?>>() {
--- End diff --
I'd be tempted to move the public methods to a separate `SpecParameterPredicates` class.
---
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 #229: Merge brooklyn.parameters in YAML files
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/229#discussion_r70851216
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynEntityDecorationResolver.java ---
@@ -181,19 +181,17 @@ protected void addDecorationFromJsonMap(Map<?, ?> decorationJson, List<EntityIni
public static class SpecParameterResolver extends BrooklynEntityDecorationResolver<SpecParameter<?>> {
protected SpecParameterResolver(BrooklynYamlTypeInstantiator.Factory instantiator) { super(instantiator); }
- @Override protected String getDecorationKind() { return "Spec Parameter initializer"; }
+
+ @Override
+ protected String getDecorationKind() { return "Spec Parameter initializer"; }
@Override
public void decorate(EntitySpec<?> entitySpec, ConfigBag attrs) {
List<? extends SpecParameter<?>> explicitParams = buildListOfTheseDecorationsFromEntityAttributes(attrs);
- // TODO see discussion at EntitySpec.parameters;
- // maybe we should instead inherit always, or
- // inherit except where it is set as config and then add the new explicit ones
- if (!explicitParams.isEmpty()) {
- entitySpec.parameters(explicitParams);
- }
- if (entitySpec.getParameters().isEmpty()) {
- entitySpec.parameters(BasicSpecParameter.fromSpec(instantiator.loader.getManagementContext(), entitySpec));
+ if (explicitParams.size() > 0) {
+ BasicSpecParameter.addParameters(entitySpec, explicitParams, instantiator.loader);
+ } else {
+ BasicSpecParameter.addParameters(entitySpec, ImmutableList.<SpecParameter<?>>of(), instantiator.loader);
--- End diff --
True, not sure what I was thinking 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #229: Merge brooklyn.parameters in YAML files
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/229#discussion_r70794277
--- Diff: camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynEntityDecorationResolver.java ---
@@ -181,19 +181,17 @@ protected void addDecorationFromJsonMap(Map<?, ?> decorationJson, List<EntityIni
public static class SpecParameterResolver extends BrooklynEntityDecorationResolver<SpecParameter<?>> {
protected SpecParameterResolver(BrooklynYamlTypeInstantiator.Factory instantiator) { super(instantiator); }
- @Override protected String getDecorationKind() { return "Spec Parameter initializer"; }
+
+ @Override
+ protected String getDecorationKind() { return "Spec Parameter initializer"; }
@Override
public void decorate(EntitySpec<?> entitySpec, ConfigBag attrs) {
List<? extends SpecParameter<?>> explicitParams = buildListOfTheseDecorationsFromEntityAttributes(attrs);
- // TODO see discussion at EntitySpec.parameters;
- // maybe we should instead inherit always, or
- // inherit except where it is set as config and then add the new explicit ones
- if (!explicitParams.isEmpty()) {
- entitySpec.parameters(explicitParams);
- }
- if (entitySpec.getParameters().isEmpty()) {
- entitySpec.parameters(BasicSpecParameter.fromSpec(instantiator.loader.getManagementContext(), entitySpec));
+ if (explicitParams.size() > 0) {
+ BasicSpecParameter.addParameters(entitySpec, explicitParams, instantiator.loader);
+ } else {
+ BasicSpecParameter.addParameters(entitySpec, ImmutableList.<SpecParameter<?>>of(), instantiator.loader);
--- End diff --
No need for an `if-else` here. Above case handles all code paths.
---
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 #229: Merge brooklyn.parameters in YAML files
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/229#discussion_r70851697
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,34 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Adds the given list of {@link SpecParameter parameters} to the provided
+ * {@link AbstractBrooklynObjectSpec spec}. Inherits parent parameters except
+ * where there is an existing {@link ConfigKey config key}.
+ * <p>
+ * Implemented by explicitly replacing the existing parameters with an
+ * updated list.
+ *
+ * @see EntitySpec#parameters(List)
+ */
+ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? extends SpecParameter<?>> explicitParams, BrooklynClassLoadingContext loader) {
+ if (explicitParams.size() > 0) {
+ List<SpecParameter<?>> currentParams = MutableList.copyOf(spec.getParameters());
+ Map<ConfigKey<?>, ?> currentConfig = spec.getConfig();
+ for (final SpecParameter<?> param : explicitParams) {
+ Optional<ConfigKey<?>> configExists = Iterables.tryFind(currentConfig.keySet(), ConfigPredicates.nameEqualTo(param.getConfigKey().getName()));
--- End diff --
I was trying to deal with the situation where a config key exists on the class, and a value is set in the YAML for it, and did not want the parameter to override that value. I now think that this won't be a problem - the default value will change (perhaps) but the actual value set will be the value from the YAML brooklyn.config setting. I'll write some more tests for this.
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/229#discussion_r70796539
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,34 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Adds the given list of {@link SpecParameter parameters} to the provided
+ * {@link AbstractBrooklynObjectSpec spec}. Inherits parent parameters except
+ * where there is an existing {@link ConfigKey config key}.
+ * <p>
+ * Implemented by explicitly replacing the existing parameters with an
+ * updated list.
+ *
+ * @see EntitySpec#parameters(List)
+ */
+ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? extends SpecParameter<?>> explicitParams, BrooklynClassLoadingContext loader) {
+ if (explicitParams.size() > 0) {
+ List<SpecParameter<?>> currentParams = MutableList.copyOf(spec.getParameters());
+ Map<ConfigKey<?>, ?> currentConfig = spec.getConfig();
+ for (final SpecParameter<?> param : explicitParams) {
+ Optional<ConfigKey<?>> configExists = Iterables.tryFind(currentConfig.keySet(), ConfigPredicates.nameEqualTo(param.getConfigKey().getName()));
+ if (!configExists.isPresent()) {
+ Optional<SpecParameter<?>> paramExists = Iterables.tryFind(currentParams, SpecParameterPredicates.sameName(param));
+ if (paramExists.isPresent()) {
+ currentParams.remove(paramExists.get());
+ }
+ currentParams.add(param);
+ }
--- End diff --
Could use a `LinkedHashSet` to simplify the code.
---
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 #229: Merge brooklyn.parameters in YAML files
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/229#discussion_r69508940
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,73 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Returns true if the {@link SpecParameter parameter name} is the same as
+ * that on the specified paramater.
+ */
+ protected static Predicate<SpecParameter<?>> sameName(final SpecParameter<?> param) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getConfigKey().getName().equals(param.getConfigKey().getName());
+ }
+ };
+ }
+
+ /**
+ * Returns true if the {@link SpecParameter#getLabel() label} is the same as
+ * the specified string.
+ */
+ public static Predicate<SpecParameter<?>> labelEqualTo(final String label) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getLabel().equals(label);
+ }
+ };
+ }
+
+ /**
+ * Returns true if the {@link ConfigKey#getName() config key name} is the same
+ * as the specified string.
+ */
+ public static Predicate<SpecParameter<?>> nameEqualTo(final String name) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getConfigKey().getName().equals(name);
+ }
+ };
+ }
+
+ /**
+ * Adds the given list of {@link SpecParameter parameters} to the provided
+ * {@link AbstractBrooklynObjectSpec spec}. Inherits parent parameters except
+ * where there is an existing {@link ConfigKey config key}.
+ * <p>
+ * Implemented by explicitly replacing the existing parameters with an
+ * updated list.
+ *
+ * @see EntitySpec#parameters(List)
+ */
+ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? extends SpecParameter<?>> explicitParams, BrooklynClassLoadingContext loader) {
+ if (explicitParams.size() > 0) {
+ List<SpecParameter<?>> currentParams = MutableList.copyOf(spec.getParameters());
+ Map<ConfigKey<?>, ?> currentConfig = spec.getConfig();
+ for (final SpecParameter<?> param : explicitParams) {
+ Optional<ConfigKey<?>> configExists = Iterables.tryFind(currentConfig.keySet(), ConfigPredicates.nameEqualTo(param.getConfigKey().getName()));
+ if (!configExists.isPresent()) {
+ Optional<SpecParameter<?>> paramExists = Iterables.tryFind(currentParams, sameName(param));
+ if (paramExists.isPresent()) {
+ currentParams.remove(paramExists.get());
--- End diff --
@aledsage not sure, can you suggest anything?
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/229
Thanks @grkvlt. 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 pull request #229: Merge brooklyn.parameters in YAML files
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/229#discussion_r70148657
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,73 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Returns true if the {@link SpecParameter parameter name} is the same as
+ * that on the specified paramater.
+ */
+ protected static Predicate<SpecParameter<?>> sameName(final SpecParameter<?> param) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getConfigKey().getName().equals(param.getConfigKey().getName());
+ }
+ };
+ }
+
+ /**
+ * Returns true if the {@link SpecParameter#getLabel() label} is the same as
+ * the specified string.
+ */
+ public static Predicate<SpecParameter<?>> labelEqualTo(final String label) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getLabel().equals(label);
+ }
+ };
+ }
+
+ /**
+ * Returns true if the {@link ConfigKey#getName() config key name} is the same
+ * as the specified string.
+ */
+ public static Predicate<SpecParameter<?>> nameEqualTo(final String name) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getConfigKey().getName().equals(name);
+ }
+ };
+ }
+
+ /**
+ * Adds the given list of {@link SpecParameter parameters} to the provided
+ * {@link AbstractBrooklynObjectSpec spec}. Inherits parent parameters except
+ * where there is an existing {@link ConfigKey config key}.
+ * <p>
+ * Implemented by explicitly replacing the existing parameters with an
+ * updated list.
+ *
+ * @see EntitySpec#parameters(List)
+ */
+ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? extends SpecParameter<?>> explicitParams, BrooklynClassLoadingContext loader) {
+ if (explicitParams.size() > 0) {
+ List<SpecParameter<?>> currentParams = MutableList.copyOf(spec.getParameters());
+ Map<ConfigKey<?>, ?> currentConfig = spec.getConfig();
+ for (final SpecParameter<?> param : explicitParams) {
+ Optional<ConfigKey<?>> configExists = Iterables.tryFind(currentConfig.keySet(), ConfigPredicates.nameEqualTo(param.getConfigKey().getName()));
+ if (!configExists.isPresent()) {
+ Optional<SpecParameter<?>> paramExists = Iterables.tryFind(currentParams, sameName(param));
+ if (paramExists.isPresent()) {
+ currentParams.remove(paramExists.get());
--- End diff --
OK, I think that might work, if I remove the code that deals with `ConfigKey` then things will be OK, because the actual config will still override a parameter with a default, no matter where in the hierarchy it is set. I'll add a test case for the inherited config case. I assume you mean something like this:
```YAML
- type: entity
id: parent
brooklyn.config:
key: value
brooklyn.children:
- type: entity
id: child
brooklyn.parameters:
- name: key
default: other
```
So, the value of `$brooklyn:entity("child").config("key")` will be `value` because it inherits from the parent entity, yes?
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the issue:
https://github.com/apache/brooklyn-server/pull/229
@neykov I have updated this with the suggested changes
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the issue:
https://github.com/apache/brooklyn-server/pull/229
See [message](http://mail-archives.apache.org/mod_mbox/brooklyn-dev/201607.mbox/%3CCAEK0Ph2S4WJ7pV6XNr21wJGPfikX5tQYgm3at1dNDoBwoacy6Q%40mail.gmail.com%3E) on <ma...@brooklyn.apache.org> for discussion.
---
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 #229: Merge brooklyn.parameters in YAML files
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/229#discussion_r70851874
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java ---
@@ -158,18 +157,18 @@ public void testDepentantCatalogsInheritParameters(Class<? extends BrooklynObjec
" - simple",
" - id: " + SYMBOLIC_NAME,
" item:",
- " type: paramItem:" + TEST_VERSION);
+ " type: paramItem");
--- End diff --
Don't remember; I think it just didn't work properly with it set, and other tests didn't have 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 #229: Merge brooklyn.parameters in YAML files
Posted by grkvlt <gi...@git.apache.org>.
Github user grkvlt commented on the issue:
https://github.com/apache/brooklyn-server/pull/229
@neykov I implemented your suggestion (first add from spec if no spec parameters then merge with explicit parameters) and it also now places the current entities params at the start of the list. If there is no objection from the mailing list, we should be able to merge soon.
---
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 #229: Merge brooklyn.parameters in YAML files
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/229#discussion_r69450574
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,73 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Returns true if the {@link SpecParameter parameter name} is the same as
+ * that on the specified paramater.
+ */
+ protected static Predicate<SpecParameter<?>> sameName(final SpecParameter<?> param) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getConfigKey().getName().equals(param.getConfigKey().getName());
+ }
+ };
+ }
+
+ /**
+ * Returns true if the {@link SpecParameter#getLabel() label} is the same as
+ * the specified string.
+ */
+ public static Predicate<SpecParameter<?>> labelEqualTo(final String label) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getLabel().equals(label);
+ }
+ };
+ }
+
+ /**
+ * Returns true if the {@link ConfigKey#getName() config key name} is the same
+ * as the specified string.
+ */
+ public static Predicate<SpecParameter<?>> nameEqualTo(final String name) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getConfigKey().getName().equals(name);
+ }
+ };
+ }
+
+ /**
+ * Adds the given list of {@link SpecParameter parameters} to the provided
+ * {@link AbstractBrooklynObjectSpec spec}. Inherits parent parameters except
+ * where there is an existing {@link ConfigKey config key}.
+ * <p>
+ * Implemented by explicitly replacing the existing parameters with an
+ * updated list.
+ *
+ * @see EntitySpec#parameters(List)
+ */
+ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? extends SpecParameter<?>> explicitParams, BrooklynClassLoadingContext loader) {
+ if (explicitParams.size() > 0) {
+ List<SpecParameter<?>> currentParams = MutableList.copyOf(spec.getParameters());
+ Map<ConfigKey<?>, ?> currentConfig = spec.getConfig();
+ for (final SpecParameter<?> param : explicitParams) {
+ Optional<ConfigKey<?>> configExists = Iterables.tryFind(currentConfig.keySet(), ConfigPredicates.nameEqualTo(param.getConfigKey().getName()));
+ if (!configExists.isPresent()) {
+ Optional<SpecParameter<?>> paramExists = Iterables.tryFind(currentParams, sameName(param));
+ if (paramExists.isPresent()) {
+ currentParams.remove(paramExists.get());
--- End diff --
Not sure I follow. I'd have though we still want to have the parameter (e.g. so we get the metadata such as its description). I can see how removing it would mean that it would pick up the config value rather than the param's default value, but is there not a better way to do this?
---
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 #229: Merge brooklyn.parameters in YAML files
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/229#discussion_r69521020
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,73 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Returns true if the {@link SpecParameter parameter name} is the same as
+ * that on the specified paramater.
+ */
+ protected static Predicate<SpecParameter<?>> sameName(final SpecParameter<?> param) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getConfigKey().getName().equals(param.getConfigKey().getName());
+ }
+ };
+ }
+
+ /**
+ * Returns true if the {@link SpecParameter#getLabel() label} is the same as
+ * the specified string.
+ */
+ public static Predicate<SpecParameter<?>> labelEqualTo(final String label) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getLabel().equals(label);
+ }
+ };
+ }
+
+ /**
+ * Returns true if the {@link ConfigKey#getName() config key name} is the same
+ * as the specified string.
+ */
+ public static Predicate<SpecParameter<?>> nameEqualTo(final String name) {
+ return new Predicate<SpecParameter<?>>() {
+ @Override
+ public boolean apply(SpecParameter<?> input) {
+ return input.getConfigKey().getName().equals(name);
+ }
+ };
+ }
+
+ /**
+ * Adds the given list of {@link SpecParameter parameters} to the provided
+ * {@link AbstractBrooklynObjectSpec spec}. Inherits parent parameters except
+ * where there is an existing {@link ConfigKey config key}.
+ * <p>
+ * Implemented by explicitly replacing the existing parameters with an
+ * updated list.
+ *
+ * @see EntitySpec#parameters(List)
+ */
+ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? extends SpecParameter<?>> explicitParams, BrooklynClassLoadingContext loader) {
+ if (explicitParams.size() > 0) {
+ List<SpecParameter<?>> currentParams = MutableList.copyOf(spec.getParameters());
+ Map<ConfigKey<?>, ?> currentConfig = spec.getConfig();
+ for (final SpecParameter<?> param : explicitParams) {
+ Optional<ConfigKey<?>> configExists = Iterables.tryFind(currentConfig.keySet(), ConfigPredicates.nameEqualTo(param.getConfigKey().getName()));
+ if (!configExists.isPresent()) {
+ Optional<SpecParameter<?>> paramExists = Iterables.tryFind(currentParams, sameName(param));
+ if (paramExists.isPresent()) {
+ currentParams.remove(paramExists.get());
--- End diff --
@grkvlt I wondered about the same pattern as is used for config (see `BrooklynComponentTemplateResolver.decorateSpec`). Not sure if the `configureEntityConfig` does anything significantly different from what you have though, just in a different place.
My gut high-level feel was that you'd always overwrite the inherited SpecParameter if there was one with the same name (irrespective of whether an additional config value is defined). Then when we look up the config value, we'd figure out the right ConfigKey (see `EntityConfigMap.getConfig`, and how it looks up "ownKey" - that should return the config key produced from our SpecParameter). Then we will use that as the default.
The area that this PR does not seem to address (in the test cases at least, as far as I can see) is the second kind of inheritance: an entity inheriting config from its parent. What should this do? Should we find the `SpecParameter` of our parent entity, and use that default? Or are you leaving that for a separate PR?
But I really don't like the code in there (or our camp parsing in general - hard to know exactly what to do to improve it though).
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/229#discussion_r70800494
--- Diff: camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/SpecParameterUnwrappingTest.java ---
@@ -158,18 +157,18 @@ public void testDepentantCatalogsInheritParameters(Class<? extends BrooklynObjec
" - simple",
" - id: " + SYMBOLIC_NAME,
" item:",
- " type: paramItem:" + TEST_VERSION);
+ " type: paramItem");
--- End diff --
Why remove the version?
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/229
@grkvlt Changes look good and can be merged as is. Still think it's a good idea to gather some input on the mailing list, or at least increase exposure of the upcoming change before merging. For example - are we prepending or appending parameters (matters for UI).
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/229#discussion_r70798866
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,34 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Adds the given list of {@link SpecParameter parameters} to the provided
+ * {@link AbstractBrooklynObjectSpec spec}. Inherits parent parameters except
+ * where there is an existing {@link ConfigKey config key}.
+ * <p>
+ * Implemented by explicitly replacing the existing parameters with an
+ * updated list.
+ *
+ * @see EntitySpec#parameters(List)
+ */
+ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? extends SpecParameter<?>> explicitParams, BrooklynClassLoadingContext loader) {
--- End diff --
Isn't this helper better moved in the `parametersAdd` method of `AbstractBrooklynobjectSpec`?
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/229#discussion_r70798405
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,34 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Adds the given list of {@link SpecParameter parameters} to the provided
+ * {@link AbstractBrooklynObjectSpec spec}. Inherits parent parameters except
+ * where there is an existing {@link ConfigKey config key}.
+ * <p>
+ * Implemented by explicitly replacing the existing parameters with an
+ * updated list.
+ *
+ * @see EntitySpec#parameters(List)
+ */
+ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? extends SpecParameter<?>> explicitParams, BrooklynClassLoadingContext loader) {
+ if (explicitParams.size() > 0) {
--- End diff --
Shouldn't be one or the other, but both instead. First add from spec (if no spec parameters), then merge with explicit parameters.
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/229#discussion_r72664225
--- Diff: api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java ---
@@ -153,17 +155,20 @@ public SpecT parameters(List<? extends SpecParameter<?>> parameters) {
@Beta
public SpecT parametersAdd(List<? extends SpecParameter<?>> parameters) {
// parameters follows immutable pattern, unlike the other fields
- Builder<SpecParameter<?>> result = ImmutableList.<SpecParameter<?>>builder();
- if (this.parameters!=null)
- result.addAll(this.parameters);
- result.addAll( checkNotNull(parameters, "parameters") );
- this.parameters = result.build();
+ Set<SpecParameter<?>> params = MutableSet.<SpecParameter<?>>copyOf(parameters);
+ Set<SpecParameter<?>> current = MutableSet.<SpecParameter<?>>copyOf(this.parameters);
+ current.removeAll(params);
+
+ this.parameters = ImmutableList.<SpecParameter<?>>builder()
+ .addAll(params)
+ .addAll(current)
--- End diff --
I really like how this turned out. We can use it to reorder/pull up parameters we are interested in.
---
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 #229: Merge brooklyn.parameters in YAML files
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/229#discussion_r70795863
--- Diff: core/src/main/java/org/apache/brooklyn/core/objs/BasicSpecParameter.java ---
@@ -384,5 +387,34 @@ public static WeightedParameter getFieldConfig(ConfigKey<?> config, Field config
}
}
+ /**
+ * Adds the given list of {@link SpecParameter parameters} to the provided
+ * {@link AbstractBrooklynObjectSpec spec}. Inherits parent parameters except
+ * where there is an existing {@link ConfigKey config key}.
+ * <p>
+ * Implemented by explicitly replacing the existing parameters with an
+ * updated list.
+ *
+ * @see EntitySpec#parameters(List)
+ */
+ public static void addParameters(AbstractBrooklynObjectSpec<?, ?> spec, List<? extends SpecParameter<?>> explicitParams, BrooklynClassLoadingContext loader) {
+ if (explicitParams.size() > 0) {
+ List<SpecParameter<?>> currentParams = MutableList.copyOf(spec.getParameters());
+ Map<ConfigKey<?>, ?> currentConfig = spec.getConfig();
+ for (final SpecParameter<?> param : explicitParams) {
+ Optional<ConfigKey<?>> configExists = Iterables.tryFind(currentConfig.keySet(), ConfigPredicates.nameEqualTo(param.getConfigKey().getName()));
--- End diff --
Should add parameters even if a config key with the same name exists. Config keys coming from yaml are too generic if the config key doesn't exist on the class. An explicit parameter with type information is always preferred. And even if a config key with the same name exists on the class, a parameter from yaml should override 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.
---