+1 to the proposal, I like the simplification and consistency that it will bring.
Is it worth getting the @Deprecated into 0.10.0? Would we not want to also update the docs at the time to remove recommendations to use @SetFromFlag? It would seem odd to deprecate the annotation in the Java but leave the docs untouched. Geoff On Wed, 7 Dec 2016 at 11:32 Richard Downer <[email protected]> wrote: > +1 > > If there's another 0.10.0 release candidate, do we want to try and get that > "deprecated" tag in to 0.10.0? > > Richard. > > > On 2 December 2016 at 21:16, Sam Corbett <[email protected]> > wrote: > > > I don't see much to disagree with here. The SetFromFlag annotation is an > > abstraction that might have made sense briefly but is less relevant as we > > push pure-YAML blueprints. Deprecating the feature will be a relatively > > simple, unintrusive change that will make the codebase easier to reason > > with. > > > > Sam > > > > > > On 23/11/2016 11:44, Aled Sage wrote: > > > >> Responding to some more of Alex's comments that he made in the renamed > >> email thread "[PROPOSAL] Deprecate config key aliases / SetFromFlag"... > >> > >> --- > >> To be clear, if the conclusion from the "Deprecate config key aliases" > >> discussion is that we support multiple non-deprecated names, then this > >> proposal will support that (see "Part 2" of the original email). > >> > >> I'm assuming we'd *not* want aliases to deliberately have different > >> semantics (e.g. not inherited). But if we did, we can support that by > being > >> explicit in the ConfigKey, rather than using the `@SetFromFlag` > annotation. > >> > >> --- > >> Alex wrote: > >> "the real issue in my view is that we have too many aliases and they > >> are used inconsistently without good docs/help." > >> > >> This proposal gives us a clear way to improve that. The multiple names > >> would all be defined on the ConfigKey, and we'd clearly indicate which > are > >> the deprecated names. > >> > >> Once we have that, one can incrementally clean up ConfigKey names on > >> existing entities, as desired. > >> > >> --- > >> Alex wrote: > >> "a canonical form and interactive help on keys will go a long way > towards > >> solving that" > >> > >> Again, this proposal begins to tackle that. It makes all the name(s) > >> available on the entity's type, so they can be included in > auto-generated > >> docs and so that a more sophisticated YAML composer can get access to > them. > >> > >> This proposed change is also a big enabler for the usability > improvements. > >> > >> --- > >> Alex wrote: > >> "simply deprecating aliases without [a canonical form and interactive > >> help on keys] is just going to be irritating." > >> > >> I disagree. It will make it explicit which name is the recommended one, > >> and which names are deprecated-but-still-supported. > >> > >> --- > >> > "similarly i'd like us to have a better tie in with source control and > >> developer workflow before advocating a big change to blueprints" > >> > >> I don't think that deprecating @SetFromFlag is a big change to > >> blueprints. It is a big clean-up, and makes things clearer. > >> > >> Also, if it allows us to clean up pieces of horrible code that treat > >> `@SetFromFlag` and config key names differently, then it will make > >> subsequent big changes (e.g. to YAML parsing and entity construction) > >> easier and less risky. > >> > >> Aled > >> > >> > >> On 23/11/2016 08:32, Thomas Bouron wrote: > >> > >>> 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 > >>> > >>> > >> > > >
