brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aled Sage <aled.s...@gmail.com>
Subject Re: [PROPOSAL] Deprecate @SetFromFlag
Date Thu, 08 Dec 2016 07:48:24 GMT
I think not in 0.10.0. We only want to deprecate it when we have an 
alternative (preferred) mechanism for people to use. We don't yet 
support declaring deprecated (or aliased?!) names on a config key. I 
therefore suggest we aim for including it in 0.11.0.

Aled


On 07/12/2016 11:32, Richard Downer 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 <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
View raw message