Thanks Alex.
For aliases for the _type_, agreed. We'll need to be careful how we
choose/register those aliases. That feels like a different discussion
from this one, which focuses on configKey names though.
---
For "replace the ad hoc code for different types e.g. EntitySpec,
PolicySpec, ..."
My understanding was that #363 would radically change our YAML parsing
(and later persistence). However, we would still generate the Java
objects like `EntitySpec` and still use the Entity Factory for
constructing them. I assumed we would still (at the java backend level)
have the ConfigKey class.
I agree we can make other big improvements for getting rid of
differences between EntitySpec, PolicySpec etc, and differences between
how Entity and Policy handle config/attributes etc. Again, that feels
like a different topic.
---
You said "for readability i'd say that a comfortable alias is nicer to
work with and should be the canonical form, as opposed to a long and
ugly identifier".
I question whether we need the long and ugly identifier at all (except
as a deprecated name for backwards compatibility).
Perhaps we also have a difference of opinion of how the alias should
behave. I think that, if it exists, it should be a genuine alias that
can be used anywhere that the other name(s) can be used. Do you think we
should have different rules for where the alias can be used and how it
behaves (e.g. never inherited in the runtime management hierarchy,
and/or never inherited by sub-types)?
---
I'll respond on the separate email thread ("Deprecate @SetFromFlag")
about how best to move towards those usability improvements.
(So this thread is just about whether we should have aliases - they are
separate questions).
Aled
On 23/11/2016 10:27, Alex Heneveld wrote:
concrete example where aliases are essential in my view: when
entering the _type_. how many of us switch to an IDE or grep to find
the package.Class name then paste it in. having `server` as an alias
for the type makes it much easier to write yaml.
my canonical form vision is that in the ui we can highlight wherever a
non-canonical form is being used and give the option to switch. for
readability i'd say that a comfortable alias is nicer to work with and
should be the canonical form, as opposed to a long and ugly
identifier. so re your obj #1 we want some aliases: it's not a
dead-end code path. discarding them altogether is going to cause
other issues. (that's not to say we can't clean up their usage, which
of course we can.)
btw #363 allows design time yaml to be converted to canonical form not
just persisted state of deployed models (and yes, i think it should
replace the ad hoc code for different types eg EntitySpec, PolicySpec,
....).
the real issue in my view is that we have too many aliases and they
are used inconsistently without good docs/help. a canonical form and
interactive help on keys will go a long way towards solving that.
simply deprecating aliases without that is just going to be
irritating. 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 think this will help with #2; ie have in-product
warnings on deprecated usage and paths to upgrade, before we
deprecate/drop aliases ... this is an issue either with your proposal
or mine).
so my strong preference is to focus on those usability items first,
and *then* look at eliminating some aliases. any other path is going
to be even more disruptive for users!
--a
On 23/11/2016 10:09, Aled Sage wrote:
// The @SetFromFlag is an implementation detail - this proposal is
just to discuss whether we should have aliases.
As Alex says, the main problem being solved is that having multiple
names makes things confusing (even if we were to fix other
inconsistencies so that those names could be used in the same way
anywhere).
---
Alex suggests having a "clear preferred way -- a canonical form if
you like -- for any blueprint, and the ability to output things in
that format."
Two things scare me about that:
1. It suggests that we go to the effort of supporting an alternative
name, but make sure we never use that alternate name in any of our
examples and thus try to make sure users don't use it. If so, why is
it there? Will it just lead to confusion when someone comes across a
blueprint that uses the short-form name, which is thus different
from all "official" examples?
2. For "output things in that format", their YAML blueprint is likely
in version control (or blog, or whatever). We are thus not changing
their blueprint. If they use a short-form name, then that will
continue to be in version control. If the blueprint is added to an
online catalog, it will continue to use that short-form name
(because we'd show in the catalog the exact blueprint from their git
repo).
---
Alex says "It also gives us an easy way to update a blueprint where
things are deprecated/changed."
I don't follow - are we talking about solving different problems, or
is your vision of PR #363 that we eventually replace the EntitySpec
and ConfigKey classes, and the `brooklyn.parameters` as well?
Let's take a concrete use-cases. (let's not argue/discuss the
specific names, and instead focus on the use-cases):
Someone writes a blueprint with `enricher.sourceSensor: cpuUsage`. We
deprecate "enricher.sourceSensor", preferring the name "sourceSensor".
The desired behaviour (IMO) is that:
1. We continue to support both names for X releases/months.
2. The blueprint author is warned about use of the deprecated name
(next time they validate, or next time they deploy).
3. The entity's type show the new name and deprecated name(s). This is
also included in auto-generated docs (similar to auto-generated
javadoc), and is available for Brooklyn's YAML composer to give
warnings (either while editing, or when the blueprint is submitted).
I'm guessing what you mean by "easy way to update a blueprint" is for
the persisted state: to switch the name that is written to the
persisted state, so that it uses the new name. That is probably good,
but we should think carefully about implications for rolling back to
older Brooklyn versions.
---
For comparing "long-name syntax" versus short "flag name" with CLIs...
CLIs usually follow a very specific convention, e.g. `diff -w` and
`diff --ignore-all-space`; except for java which uses single "-" for
both short and long form - a very bad thing in my opinion :-)
Some CLIs (like `br`) accept long and short forms (e.g. `br
application` or `br app`). This is ok because the context is never
ambiguous - you never pass "application" or "app" to a different
command, expecting different behaviour / ambiguity for it then
invoking `br`.
---
If we conclude that aliases are sometimes a good idea, we should
agree when and how they should be used (e.g. is it primarily for
short-form; is it for multiple sensible names; is it for supporting
camel-case versus dot or underscore forms; etc).
Unfortunately our aliases are massively over used in Brooklyn (in my
opinion), in an ad hoc manner. Most (if not all) should be deprecated.
Aled
On 22/11/2016 22:31, Alex Heneveld wrote:
Hi Aled -
There are a few more things that I think need to be considered
here. Also, combining your proposals.
Firstly -- throughout Brooklyn we use the long-name syntax as an
internal / formal name of a key to prevent ambiguity, and a short
"flag name" to make it easy for a user to write. This is sometimes
useful where you set a config key at the root, using the formal
name, so that it is inherited at a specific descendant. If we don't
need to rely on inheritance this argument goes away somewhat but I
don't think we're there yet.
Secondly -- what problem are we trying to solve? I think we should
be dismissive of proposals that don't solve a problem. Yours does
but it doesn't say what that problem is. I think the problem is
that having multiple ways to do the same thing can be confusing,
especially if docs and examples are inconsistent.
I think a better solution that that problem is a clear *preferred*
way -- a canonical form if you like -- for any blueprint, and the
ability to output things in that format.
This means users looking at our docs and examples -- or anything
that uses canonical forms -- will see a consistent style. It
eliminates some, if not all, of the confusion which is the problem.
It also gives us an easy way to update a blueprint where things are
deprecated/changed.
I'd much prefer going that route than the proposal you suggest, and
then deciding after that whether deprecating all aliases is the
right thing. (Given the short/long distinction I'd prefer the idea
that "all-but-one" alias might be deprecated in most cases. But I'm
also unsure that with a canonical form and good tooling, aliases
might actually be a good thing. They are commonplace in more text
interactive scripting -- which this approaches, as opposed to the
method names in Aled's proposal. Think of CLI arguments and of
course the text adventure games of our youth...
(As you know this is largely implemented in #363, at which point
deprecated @SetFromFlag and moving to preferred aliases becomes
simple, and optionally saying that all or any non-preferred alias is
deprecated.)
--A
[1] https://github.com/apache/brooklyn-server/pull/363
On 22/11/2016 21:48, Aled Sage wrote:
Hi all,
TL;DR: aliases for config keys should be deprecated. Each config
key should have only one proper name, with other names deprecated.
We should change this *after* releasing 0.10.0, to decrease risk.
(This was mentioned in the email thread "[PROPOSAL] Deprecate
@SetFromFlag").
_*Current Situation*_
When defining a config keys in Java, one can add an annotation like:
@SetFromFlag("version")
ConfigKey<String> SUGGESTED_VERSION =
newStringConfigKey("install.version", "Suggested version");
This alternative name specified in @SetFromFlag is respected in
some situations, but not others.
_*Requirements*_
The desire to support multiple names can be split into three
different use-cases:
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.
It is covered in the email thread "[PROPOSAL] Deprecate
@SetFromFlag".
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: Don't Use Aliases For Config Keys*_
I propose that we deprecate support for use-case (2) above: use of
aliases.
The use of aliases leads to confusion about what the different
names mean. When someone is looking at examples, it's unclear
whether they mean the same thing, or if one is valid but the other
is not. There is a scary amount of folk lore about config key names
already!
Example blueprints have a tendency to proliferate: a blueprint
written within a company adopting Brooklyn is often used as the
basis for other blueprints. If we support an alias without a very
obvious deprecation warning in the YAML composer, then use of that
alias will spread.
---
Note that this is a separate discussion from whether our existing
names are right! There are probably a lot of names we should
deprecate and improve.
_*Proposal: Guidelines for "deprecated"*_
For use-case (1) above, i.e. deprecated names, we should treat that
in a similar way to Java deprecated methods.
We should *not* add a deprecated name just because we think it's a
nice alternative name. We should only add deprecated names when it
is an undesirable name that we need to support for backwards
compatibility.
For example, if someone submitted a pull request with three methods
that all did the same thing, then I'd reject that PR - e.g.
sort(collection), arrange(collection) and order(collection).
_*Proposal: Hints for Names*_
There is a compelling argument for providing hints for incorrect
names, particularly when using an online YAML composer or when
validating a YAML blueprint.
For example, if someone uses "environment.variables" but the real
name is "env", then a validation warning can be shown with an error
message proposing the correct name.
This could be achieved by providing "close names". If the name
matches another config key, then that would be used. Otherwise, if
the name matches a "close name" of a config key, then it would show
the validation warning. Note that it is a warning rather than an
error because of the rules for config inheritance: it could be that
the config key will be inherited by children that will understand
the given name.
We could have a "strict" mode that treated such warnings as errors
(sounds like a topic for a different email thread!).
We could do some similar automatic checks for close matches, e.g.
to warn if "installCommand" is used instead of "install.command".
To me, it feels like "hints" is stage two - i.e. lower priority
than agreeing each config key should have a single definitive name,
and deprecating the other names.
