[
https://issues.apache.org/jira/browse/BROOKLYN-286?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15312075#comment-15312075
]
ASF GitHub Bot commented on BROOKLYN-286:
-----------------------------------------
Github user neykov commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/173#discussion_r65514081
--- Diff:
locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
---
@@ -3308,4 +3314,20 @@ public PersistenceObjectStore
newPersistenceObjectStore(String container) {
return new JcloudsBlobStoreBasedObjectStore(this, container);
}
+ // TODO Duplicate of EntityConfigMap.deepMerge
+ private <T> Maybe<?> deepMerge(Maybe<? extends T> val1, Maybe<?
extends T> val2, ConfigKey<?> keyForLogging) {
+ if (val2.isAbsent() || val2.isNull()) {
+ return val1;
+ } else if (val1.isAbsent()) {
+ return val2;
+ } else if (val1.isNull()) {
+ return val1; // an explicit null means an override; don't merge
+ } else if (val1.get() instanceof Map && val2.get() instanceof Map)
{
+ return
Maybe.of(CollectionMerger.builder().build().merge((Map<?,?>)val1.get(),
(Map<?,?>)val2.get()));
--- End diff --
I'd expect a shallow merge here. That is being able to override the values
of maps in `templateOptions`. But the arguments could go either way. Doesn't
really matter in the and as all maps in `templateOptions` are of simple types.
> merge config keys map values, where appropriate
> -----------------------------------------------
>
> Key: BROOKLYN-286
> URL: https://issues.apache.org/jira/browse/BROOKLYN-286
> Project: Brooklyn
> Issue Type: New Feature
> Affects Versions: 0.9.0
> Reporter: Aled Sage
>
> See the email discussion on [email protected], subject "[PROPOSAL]
> merging config keys", initially kicked off 25/05/2016, 12:12.
> Below is a copy of that initial proposal, which has then been further
> discussed on the mailing list.
> TL;DR: we should merge config when overriding entities/locations, where it's
> obvious that such behaviour is desired. For example, where an entity type
> defines shell.env, then a new entity extending this type should inherit and
> add to those values.
> _*REQUIREMENTS*_
> _*shell.env in entities*_
> When extending an existing entity type in YAML, it is not possible to extend
> the set of environment variables. Instead, if the sub-type declares shell.env
> it will override the inherited values.
> For example, consider the catalog items below:
> # Catalog
> brooklyn.catalog:
> items:
> - id: machine-with-env
> item:
> type:
> org.apache.brooklyn.entity.software.base.VanillaSoftwareProcess
> brooklyn.config:
> shell.env:
> ENV1: myEnv1
> # Blueprint
> location: ...
> services:
> - type: machine-with-env
> brooklyn.config:
> shell.env:
> ENV2: myEnv2
> launch.command: echo "ENV1=$ENV1, ENV2=$ENV2"
> A user might well expect the launch.command to have myEnv1 and myEnv2.
> However, it does not get the ENV1 environment variable. This is a real pain
> when trying to customize stock blueprints.
> We propose that the shell.env map should be *merged*.
> _*provisioning.properties*_
> An entity can be configured with provisioning.properties. These are passed to
> the location when obtaining a new machine. They supplement and override the
> values configured on the location. However, for templateOptions the
> expected/desired behaviour would be to merge the options.
> Consider the blueprint below:_*
> *_
> location:
> minCores: 1
> templateOptions:
> networks: myNetwork
> services:
> - type: org.apache.brooklyn.entity.machine.MachineEntity
> brooklyn.config:
> provisioning.properties:
> minRam: 2G
> templateOptions:
> tags: myTag
> A user might well expect the VM to be created with the given networks and
> tags. However, currently the templateOptions in provisoining.properties will
> override the existing value, rather than being merged with it.
> We propose that the templateOptions map should be *merged*.
> Valentin made a start to fix this in
> https://github.com/apache/brooklyn-server/pull/151.
> _*_*provisioning.properties in sub-entities*_
> *_
> A similar argument holds for when extending an entity-type in YAML.
> If the super-type declares template options, then any additional
> provisioning.properties declared on the entity sub-type should be *merged*
> (including merging the templateOptions map contained within it).
> _*files.preinstall, templates.preinstall, etc*_
> The same applies for the map config for: files.preinstall,
> templates.preinstall, files.install, templates.install, files.runtime and
> templates.runtime.
> We propose that these maps get *merged* with the value defined in the
> super-type.
> _*Overriding default values*_
> For default values in the super-type, we propose that this value *does* get
> overridden, rather than merged.
> For example, in the blueprint below we suggest that the launch-command in the
> sub-type should have ENV2 but not ENV_IN_DEFAULT.
> brooklyn.catalog:
> items:
> - id: machine-with-env
> version: 1.0.0
> item:
> type:
> org.apache.brooklyn.entity.software.base.VanillaSoftwareProcess
> brooklyn.parameters:
> - name: shell.env
> default:
> ENV_IN_DEFAULT: myEnvInDefault
> - id: machine-with-env-2
> version: 1.0.0
> item:
> type: machine-with-env
> brooklyn.config:
> shell.env:
> ENV2: myEnv2
> launch.command: echo "ENV_IN_DEFAULT=$ENV_IN_DEFAULT,
> ENV2=$ENV2"
> (Interestingly, the current behaviour of machine-with-env is that it gets the
> value for ENV_IN_DEFAULT but not for ENV2, so sometime strange is going on
> with re-defining the shell.env config key!)
> _*Extending commands: deferred*_
> Another scenario is where a super-type declares a value for
> `install.command`, and the sub-type wants to augment this by adding
> additional commands. Currently that is not possible. Instead the sub-type
> needs to use pre.install.command and/or post.install.command. But that leads
> to the same problem if a super-type also has a value defined for that key.
> Svet suggested we could perhaps introduce something like $brooklyn:super().
> Unless we can generalise that approach to also solve the merging of
> `shell.env` etc, then I suggest we defer the `install.command` use-case. That
> can be proposed and discussed in a different thread.
> However, if we can solve these problems with clever explicit use of
> $brooklyn:super(), then that could provide an elegant solution to all of
> these problems!
> _*Inheritance from parent entities*_
> Things are made yet more complicated by the fact we inherit config from
> parent entities, in the entity hierarchy.
> We propose that this behaviour is also configurable for the config key, but
> that the defaults stay as they are. The existing logic is applied to find the
> config value that applies to the given entity. That value is then merged with
> its super-type, as appropriate.
> For example, in the blueprint below... machine1 would get ENV1 and ENV2 (i.e.
> the ENV1 definition overrides the ENV_IN_APP definition). However, machine2
> would get ENV1 and ENV_IN_APP (i.e. it inherits ENV_IN_APP from the parent,
> and this is meged with the super-type).
> services:
> - type: org.apache.brooklyn.entity.stock.BasicApplication
> brooklyn.config:
> shell.env:
> ENV_IN_APP: myEnvInApp
> brooklyn.children:
> - type: machine-with-env
> id: machine1
> brooklyn.config:
> shell.env:
> ENV2: myEnv2
> - type: machine-with-env
> id: machine2
> The reasoning behind this is to figure out the inheritance/override rules
> incrementally. We leave the parent-inheritance as-is, and just focus on the
> sub-typing inheritance.
> Note that there is already a ConfigInheritance defined on ConfigKey for
> controlling this kind of inheritance from the parent. The legal values for
> ConfigInheritance are currently just ALWAYS and NONE.
> _*IMPLEMENTATION*_
> Clearly we do not want to implement this piecemeal. We'll add a way to
> declare that a config key should be merged with that value from the
> super-type.
> We'll change the Java ConfigKey code to be:
> public interface ConfigKey {
> /**
> * @since 0.10.0
> */
> @Nullable ConfigInheritance getParentInheritance();
> /**
> * @since 0.10.0
> */
> @Nullable ConfigInheritance getTypeInheritance();
> /**
> * @deprecated since 0.10.0; instead use {@link
> #getParentInheritance()}
> */
> @Nullable ConfigInheritance getInheritance();
> }
> We'll add to ConfigInheritance support for MERGE. We'll change the name
> "ALWAYS" to OVERRIDE (deprecating the old value).
> We'll change EntityConfigMap.getConfig to handle this new merge behaviour.
> And same for locations, policies and enrichers.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
