Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 5F40D200BD8 for ; Wed, 7 Dec 2016 12:32:57 +0100 (CET) Received: by cust-asf.ponee.io (Postfix) id 5DDD8160B0A; Wed, 7 Dec 2016 11:32:57 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id 575CE160AFD for ; Wed, 7 Dec 2016 12:32:56 +0100 (CET) Received: (qmail 86260 invoked by uid 500); 7 Dec 2016 11:32:55 -0000 Mailing-List: contact dev-help@brooklyn.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@brooklyn.apache.org Delivered-To: mailing list dev@brooklyn.apache.org Received: (qmail 86249 invoked by uid 99); 7 Dec 2016 11:32:55 -0000 Received: from mail-relay.apache.org (HELO mail-relay.apache.org) (140.211.11.15) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 07 Dec 2016 11:32:55 +0000 Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42]) by mail-relay.apache.org (ASF Mail Server at mail-relay.apache.org) with ESMTPSA id D2FC61A0780 for ; Wed, 7 Dec 2016 11:32:54 +0000 (UTC) Received: by mail-wm0-f42.google.com with SMTP id g23so163328548wme.1 for ; Wed, 07 Dec 2016 03:32:54 -0800 (PST) X-Gm-Message-State: AKaTC00hMStcVSvxuHQxrohXwo3I7g3sZE5QwKF27PDVETDvmP/XNiVmLfRwhXDC9XvasTx5C7kRAPzLjUdJMg== X-Received: by 10.28.150.75 with SMTP id y72mr2298946wmd.47.1481110373422; Wed, 07 Dec 2016 03:32:53 -0800 (PST) MIME-Version: 1.0 Received: by 10.28.132.66 with HTTP; Wed, 7 Dec 2016 03:32:52 -0800 (PST) In-Reply-To: <85c82a7c-da7d-9946-1734-9e092aeafcd7@cloudsoftcorp.com> References: <9cc6e35e-aaa6-10dc-07e5-8e8018eb1282@gmail.com> <5c7ff3b1-5174-bcd1-4af1-15a4f7dafd1c@cloudsoftcorp.com> <85c82a7c-da7d-9946-1734-9e092aeafcd7@cloudsoftcorp.com> From: Richard Downer Date: Wed, 7 Dec 2016 11:32:52 +0000 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PROPOSAL] Deprecate @SetFromFlag To: Brooklyn dev Content-Type: multipart/alternative; boundary=001a114b3506ae608005430fe124 archived-at: Wed, 07 Dec 2016 11:32:57 -0000 --001a114b3506ae608005430fe124 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable +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 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 be= ing >> explicit in the ConfigKey, rather than using the `@SetFromFlag` annotati= on. >> >> --- >> 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 a= re >> 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 toward= s >> 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-generate= d >> docs and so that a more sophisticated YAML composer can get access to th= em. >> >> This proposed change is also a big enabler for the usability improvement= s. >> >> --- >> 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 m= y >>> 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, < >>> alasdair.hodge@cloudsoftcorp.com> >>> 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 int= o >>>>> 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 b= y >>>>> 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) th= en >>>>> 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 do= es >>>>> 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 form= al >>>>>> "config key" definition (i.e. not available via >>>>>> entity.getEntityType()), and is only respected in some situations wh= en >>>>>> 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 i= s >>>>>> (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" a= nd >>>>>> "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 t= o >>>>>> 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 name= s, >>>>>> so need to keep doing that; or because we want to rename a confi= g >>>>>> key, such as correcting its spelling). >>>>>> This use-case could be reworded as the need to support deprecate= d >>>>>> 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 ya= ml >>>>>> blueprints, and also possible to deprecate config key names in YAML >>>>>> brooklyn.parameters. >>>>>> >>>>>> I suggest we support something like this: >>>>>> >>>>>> ConfigKey PRE_INSTALL_COMMAND =3D >>>>>> ConfigKeys.builder(String.class, "my.toy.example") >>>>>> .description("...") >>>>>> .defaultVal("myDefaultVal") >>>>>> .deprecatedAlias("my.toyy.example") // e.g. deprecated to correc= t >>>>>> 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 na= me >>>>>> by itself: >>>>>> - my.toyy.example >>>>>> >>>>>> Here is a real-world example in Java (getting rid of >>>>>> `@SetFromFlag("preInstallCommand")`): >>>>>> >>>>>> ConfigKey PRE_INSTALL_COMMAND =3D >>>>>> 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 real= ly >>>>>> clean up several areas of ugly/fiddly code! >>>>>> >>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>>>> Part 2 >>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>>>> >>>>>> 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 confi= g >>>>>> key aliases" to follow shortly. >>>>>> >>>>>> If desired, we could support `.alias("my.toy.example.otherName")` on >>>>>> the config key. >>>>>> >>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>>>> Part 3 >>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>>>> >>>>>> 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 behave= s >>>>>> 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 suppo= rt >>>>>> reconfigurable config so that can be used instead. >>>>>> >>>>>> >>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>>>> Part 4 >>>>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>>>>> >>>>>> One can use @SetFromFlag on fields of other objects, which allows th= e >>>>>> fields to be set when using the DSL `$brooklyn:object`. >>>>>> >>>>>> I think this will become redundant with Alex's YOML. Let's not discu= ss >>>>>> such usage of @SetFromFlag in this email thread. >>>>>> >>>>>> >>>>>> >>>>> >>>> -- >>>> >>> Thomas Bouron =E2=80=A2 Software Engineer @ Cloudsoft Corporation =E2= =80=A2 >>> http://www.cloudsoftcorp.com/ >>> Github: https://github.com/tbouron >>> Twitter: https://twitter.com/eltibouron >>> >>> >> > --001a114b3506ae608005430fe124--