brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Richard Downer <rich...@apache.org>
Subject Re: [PROPOSAL] Deprecate @SetFromFlag
Date Wed, 07 Dec 2016 11:32:52 GMT
+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 <sam.corbett@cloudsoftcorp.com>
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, <
>>> 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 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
>>>
>>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message