Hi all. I just pushed a PR on brooklyn-docs[1] that goes toward this proposal by enforcing the use of `brooklyn.config` and real configkey names throughout all yaml examples.
Best. [1] https://github.com/apache/brooklyn-docs/pull/182 On Thu, 8 Dec 2016 at 13:49 Aled Sage <[email protected]> wrote: > Alex, > > I think your "counter proposal" breaks down into two different things. > > _*Complementary Proposal*_ > There is a complementary proposal to get this working with YOML (i.e > with the 12,000 line PR at [1]). > > This leads on to giving better warnings to blueprint users, e.g. that a > config key they are using has been deprecated. This is needed in the > web-console's live YAML editor. It's also needed when adding something > to the catalog, (e.g. warnings come back when you do `br add-catalog > myapp.bom`), when deploying an app instance (e.g. `br deploy > myapp.yaml`), etc. > > I completely agree we want all of those things. > > You also suggest a "convert to canonical form" REST call. I agree that > sounds useful. > > Having a "validate" REST call that gives back warnings would also be > extremely useful, in my opinion (e.g. to validate your blueprints > against a new version of Brooklyn, and/or periodically as part of QA). > > > _*Competing Proposal*_ > You propose that we use the Java annotation `@DeprecatedAlias` on config > keys, rather than having the deprecated aliases as part of the ConfigKey > Java type. > > (but I'm a bit confused if that's really what you're proposing - you > later say "Add `deprecatedNames` to config keys".) > > I strongly favour having it as part of the ConfigKey type. When one gets > hold of a `ConfigKey` object via a Java api (or via the REST api, or in > auto-generated "javadoc style" docs), I want those deprecated aliases to > be part of it. > > I don't think we should support the `@DeprecatedAlias`. I don't see a > compelling use-case for supporting that, if we have a way to define the > deprecated aliases directly on the config key declaration. > > I understand that, longer term, we can use YOML deserialization to > convert the alias automatically. If we did that, the subsequent Java > code (which deals with EntitySpec, etc) would never see that the > deprecated name had been used. However, I think we should still store > the deprecated names on the ConfigKey. > > > _*Accessors of Deprecated Aliases*_ > I agree that if we add `configKey.getDeprecatedAliases()` then we can > support lookup that uses "env" and "shell.env". > > Note this *changes previous semantics of config inhereitance of child > entities*. Previously, if you used the alias (e.g. "env") then the child > entity was passed the value but would never look it up using that > aliased key. Therefore, from a user's perspective the config key was not > passed to the child in the same way as "shell.env". I view this as fixed > a bug - everyone I've spoken to about it (except Alex) has found this > behaviour very surprised and unintuitive. > > We can also support deprecated aliases for effector parameters, etc. > > > _*Priorities*_ > I think a big objection you have to "deprecating @SetFromFlag" is about > the priorities of the work. I believe you think that focusing on YOML, > and the web-console validation/warnings should instead be the priority. > > The work to deprecate `@SetFromFlag` is much smaller/quicker. It also > feels complementary to me. Even if some of that work is wasted when YOML > is merged and widely used in Brooklyn, I think we're talking about a > relatively small amount of work. > > The ability to deprecate (and thus change) config key names is extremely > useful, even if we haven't yet done the piece to report it in the > web-console's YAML editor. We don't need to do the latter before the > former. We just need to be considerate about not deleting the deprecated > aliases until a couple of releases after we support deprecation-warnings > in the UI. > > Aled > > [1] https://github.com/apache/brooklyn-server/pull/363 > > > On 07/12/2016 13:54, Alex Heneveld wrote: > > Hi All- > > > > I'm with Svet: as much as we might like to deprecate things, I think we > > mustn't without a clear path that allows users to move away from anything > > we've deprecated. > > > > In the case of blueprint *authors* (who might have used @SetFromFlag) we > > need to consider blueprint *consumers* who may be using config aliases > > defined by authors. If we deprecate aliases -- either marking > SetFromFlag > > as @Deprecated or introducing a deprecatedAlias method -- we need to > > provide at the same time a mechanism for authors and their blueprint > > consumers to move away from those aliases. This should be something more > > user friendly than comments in the docs. > > > > Until we have that I think it would be poor form to deprecate aliases, as > > we're essentially warning that we might remove something in a subsequent > > version, but the blueprint authors don't have a good way to ensure that > the > > blueprint consumers can migrate to the alias-free world. > > > > I'd make a counter proposal that: > > > > (1) ASAP we document that `@SetFromFlag` and config aliases in general > are > > *discouraged* and we are moving towards a migration strategy for > blueprint > > consumers that will allow them to be deprecated in most or all cases > > > > Re the migration strategy `deprecatedAlias` would work at least to record > > intent but I think YOML will provide a better path especially how we > could > > communicate that intent to blueprint consumers. I'd rather invest energy > > in that. I'd propose we continue as follows: > > > > (2) We work to activate YOML as the preferred mechanism for all > conversion > > of YAML to java objects (specs and catalog format) > > > > (3) We add a @DeprecatedAlias tag to YOML which acts like @Alias but can > > inform users it is deprecated > > > > (4) We adjust the UI to warn on use of deprecated aliases (maybe a new > > warnings view in the first instance, and we warn whenever a deprecated > item > > is encountered -- possibly just intercepting logback statements and > making > > them available) > > > > (5) We deprecate @SetFromFlag pointing at the annotations in 3 (note that > > these are still annotations because any such info is purely for the > > de/serialization from YAML; it is not needed for working with the config > > key once YAML has been parsed because YOML converts it to the official > key > > name, but see 3' below) > > > > I'd also like to: > > > > (6) We provide a REST endpoint that can convert to canonical form (YOML > > supports this out of the box, although not currently with comments), > making > > the canoncial form use the official key name for consistency, and we > expose > > this conversion functionality in the UI > > > > The one thing this doesn't address is changing the runtime name of a key. > > Say I've got a config key whose name is `shell.env` for which `env` is an > > alias, and I want to change it so that `env` is the one true name. The > > process above does not provide a way for *accessors* of the key at > runtime > > to migrate to the new name. Either the key's name is "env" or > "shell.env" > > -- it can't be both -- so all consumption/access would have to be > suddenly > > switched over from `$brooklyn:config("shell.env")` to > > `$brooklyn:config("env")` with no migration period. De/serialization > > aliases don't help us here. For that I'd add: > > > > (3') Add `deprecatedNames` to config keys, and ensure that lookups look > > also for these deprecated *names* (not aliases as aliases have never been > > supported for lookup) > > > > For sensors and effectors this is not needed as there are easy ways to > > propagate a sensor to a deprecated name and write an effector which > > forwards to another. But: > > > > (4') We should also provide a way to deprecate sensors and effectors and > > give warnings when deprecated items are used (and these warnings should > be > > surfaced in the UI) > > > > So in short I agree with the sentiment to move away from aliases but I'm > > opposed to a tactical code-level-only deprecation. A bigger improvement > is > > needed and I really think the idea of a canonical form and YOML is the > way > > to do this. It's not a small effort but I don't think it's as big as > what > > people might fear; YOML gives us a lot of power. It will let us retire > the > > many different piecemeal strategies we have for working with YAML, and > will > > also solve type access issues (who is allowed to access which classes) > and > > informing consumers where things are deprecated ... which is going to be > > hard however we do it. YOML gives us a single way which can be used for > > code completion, documentation, even graphical support for editing. > > > > Best > > Alex > > > > > > > > On 7 December 2016 at 12:31, Svetoslav Neykov < > > [email protected]> wrote: > > > >> @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 <geoff.macartney@ > >> cloudsoftcorp.com> 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 > >>>>>>> > >>>>>>> > >> > > -- Thomas Bouron • Senior Software Engineer @ Cloudsoft Corporation • https://cloudsoft.io/ Github: https://github.com/tbouron Twitter: https://twitter.com/eltibouron
