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
