Hi all. +1 as well from me. As a user, it is indeed very confusing to have different names for the name config across our documentation.
I also went down the inheritance hell because sometimes it didn't work for the very reasons you gave above Aled. As a result, I now always write my config keys within a `brooklyn.config` block, using the real config key name because I am sure how it behaves at runtime. On Wed, 23 Nov 2016, 05:16 Alasdair Hodge, <[email protected]> wrote: > Excellent write-up, Aled. > > I wholly support this, as someone who fastidiously avoids the > 'setFromFlag' names having been tripped up too many times by the > inconsistent inheritance and other "surprises" you mention. > > A. > -- > Alasdair Hodge > Principal Engineer, > Cloudsoft Corporation > > > On 23/11/2016 01:30, Aled Sage wrote: > > Alex, > > > > Sticking with the two proposals being discussed separately... > > > > The problems that deprecating @SetFromFlag (and moving the naming into > > config key) is solving are: > > > > 1. The behaviour is currently inconsistent and thus confusing - e.g. > > the name from @SetFromFlag will not be inherited, but the config key > > name will be (so "env" is not inherited but "shell.env" will be). > > People can't easily tell how things will behave when writing > > blueprints based on existing examples. > > 2. The name in @SetFromFlag is not part of the entity definition - it > > cannot be discovered on the entity type, so could never be used by a > > YAML composer. Moving aliases (deprecated or otherwise) into the > > config key definition makes it a proper part of the type. It can > > thus be included in docs as well. > > 3. We have no way to rename a YAML config key, defined in a > > brooklyn.parameters section, deprecating the old name. > > If a better name is agreed upon (e.g. correcting spelling etc) then > > one either has to break backwards compatibility or jump through > > hoops in bash/freemarker to respect both names. > > 4. Our code for handling @SetFromFlag is pretty horrible - mostly > > because it is set via a different code path from config keys, > > leading again to inconsistencies and difficult-to-maintain code. > > > > This proposal is complementary/separate from > > https://github.com/apache/brooklyn-server/pull/363 - I don't think that > > PR (or continuation of that work) would solve these problems. YOML does > > not change the nature of config keys, or EntitySpec's flag/config. > > > > Aled > > > > > > On 22/11/2016 21:44, Aled Sage wrote: > >> Hi all, > >> > >> TL;DR: deprecate `@SetFromFlag`; instead explicitly set > >> deprecatedAlias (and/or alias?) on the ConfigKey. > >> > >> We should change this *after* releasing 0.10.0, to decrease risk. > >> > >> _*Current Situation*_ > >> When writing a Java entity, one can declare a config key type as a > >> static field. One can also use the annotation `@SetFromFlag` on this. > >> The annotation allows one to include an alternative name for the key. > >> > >> This alternative name is often confusing. It is not part of the formal > >> "config key" definition (i.e. not available via > >> entity.getEntityType()), and is only respected in some situations when > >> supplying config values (e.g. when supplying config directly to an > >> entity, but not when using config inheritance). > >> > >> The `@SetFromFlag` annotation also supports "defaultVal", but this is > >> (hopefully!) never used. There is a better way to specify a default > >> value, which is to pass it into the Config Key's definition. > >> > >> The other options on the `@SetFromFlag` annotation are "immutable" and > >> "nullable". These are ignored in some/many situations, so are (I > >> believe) a bad idea to use (certainly bad to rely on). It's better to > >> use `.constraint(Predicates.notNull())` on the config key, rather than > >> "nullable". > >> > >> The reason for supporting multiple names can be split into (at least?) > >> three different use-cases (we'll discuss the second and third in a > >> separate email thread): > >> > >> 1. Backwards compatibility (e.g. because we already support two names, > >> so need to keep doing that; or because we want to rename a config > >> key, such as correcting its spelling). > >> This use-case could be reworded as the need to support deprecated > >> names. > >> 2. Aliases (i.e. a deliberate desire to support different names, > >> because those different names are seen as good). > >> 3. Hints on blueprint validation in a composer (e.g. > >> "environment.variables not valid; did you mean env?") > >> > >> _*Proposal*_ > >> We should stop using `@SetFromFlag` on config keys entirely - > >> deprecating its use, and warning anywhere it's used. > >> > >> Instead, we should add support for multiple names on the ConfigKey > >> definition itself (i.e. to the ConfigKey interface). This will make it > >> part of the entity's type. This will make it easier to support in yaml > >> blueprints, and also possible to deprecate config key names in YAML > >> brooklyn.parameters. > >> > >> I suggest we support something like this: > >> > >> ConfigKey<String> PRE_INSTALL_COMMAND = > >> ConfigKeys.builder(String.class, "my.toy.example") > >> .description("...") > >> .defaultVal("myDefaultVal") > >> .deprecatedAlias("my.toyy.example") // e.g. deprecated to correct > >> the misspelling > >> .deprecatedAliasSince("my.old.example", "0.10.0") // second arg is > >> version when it was deprecated > >> .build(); > >> > >> Or in YAML: > >> > >> brooklyn.parameters: > >> - name: my.toy.example > >> type: String > >> description: ... > >> default: myDefaultVal > >> deprecatedAliases: > >> - name: my.old.example > >> deprecatedSince: 0.10.0 > >> # The version above is optional; it will also accept the name > >> by itself: > >> - my.toyy.example > >> > >> Here is a real-world example in Java (getting rid of > >> `@SetFromFlag("preInstallCommand")`): > >> > >> ConfigKey<String> PRE_INSTALL_COMMAND = > >> ConfigKeys.builder(String.class, "pre.install.command") > >> .description("Command to be run prior to the install method > >> being called on the driver") > >> .runtimeInheritance(BasicConfigInheritance.NOT_REINHERITED) > >> .deprecatedName("preInstallCommand") > >> .build(); > >> > >> When parsing the entity's Java config keys, we should be able to > >> translate the `@SetFromFlag` into an equivalent "deprecated aliases" > >> on the Config Key object (while preserving backwards compatibility). > >> When we can delete the `@SetFromFlag` support entirely, it will really > >> clean up several areas of ugly/fiddly code! > >> > >> ========== > >> Part 2 > >> ========== > >> > >> We should discuss *in a different thread* whether we want to support > >> multiple aliases that are deliberately not deprecated (e.g. > >> "shell.env" and "env"). See email thread "[PROPOSAL] Deprecate config > >> key aliases" to follow shortly. > >> > >> If desired, we could support `.alias("my.toy.example.otherName")` on > >> the config key. > >> > >> ========== > >> Part 3 > >> ========== > >> > >> We currently also support the `@SetFromFlag` annotation on fields > >> (let's focus here on entity, location, enricher and policy). > >> > >> The value of the field is persisted, and is set on rebind. It behaves > >> like an attribute that can be set via a config key. Its use has been > >> discouraged (on entities, at least) for quite some time, but not > >> formally deprecated. > >> > >> _Proposal_ > >> For entities, we should formally deprecated this - warning in any > >> entity that uses it - and delete support for it in a couple of > >> releases time. > >> > >> For locations/policies/enrichers, I think we should also deprecated > >> it. Unfortunately they don't support "attributes", but they do support > >> reconfigurable config so that can be used instead. > >> > >> > >> ========== > >> Part 4 > >> ========== > >> > >> One can use @SetFromFlag on fields of other objects, which allows the > >> fields to be set when using the DSL `$brooklyn:object`. > >> > >> I think this will become redundant with Alex's YOML. Let's not discuss > >> such usage of @SetFromFlag in this email thread. > >> > >> > > > > > > > -- Thomas Bouron • Software Engineer @ Cloudsoft Corporation • http://www.cloudsoftcorp.com/ Github: https://github.com/tbouron Twitter: https://twitter.com/eltibouron
