@SetFromFlag can be deprecated only when: * We have an alternative way for marking (deprecated) aliases as part of the config key * A plan forward what to do with the annotation when used in beans (part 4 from proposal)
It won't be too useful If we deprecate the annotation, but don't let users know how to remove it safely from their code in a way that's compatible with existing blueprints. Suggest leaving it for next release. Svet. > On 7.12.2016 г., at 14:04, Geoff Macartney > <[email protected]> wrote: > > +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 >>>>> >>>>> >>>> >>> >>
