[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[GitHub] brooklyn-server issue #1014: [WIP] Subtype setting config val: use as key’...

Github user ahgittin commented on the issue:

    I've opened https://github.com/aledsage/brooklyn-server/pull/4 against _this_ PR to address many of the failing tests and improve logic as noted in previous comment ("making the key-overwrite method aware of the different ConfigKey subtypes").  It also adds a test for a corner case that was previously overlooked that was exposed by this, and changes serialization slightly (you can revert that commit if you don't like it).
    With these changes, it is now failing in the `camp` project on some of the `ConfigInheritanceYamlTest` cases, where:
    * type `entity-with-keys` defines `map.type-never` as "never inherit" with default key/value referring to `myDefault`
    * parent instance of type `entity-with-keys` sets `map.type-never` to `mykey=myval`
    * attempts to get anonymous key `map.type-never` on child instance which is _not_ of type `entity-with-keys` 
    The code (A) looks in the parent to find the key, (B) finds it, then (C) resolves it, correctly detecting that it is "never reinherited" but with these changes (D) it now sees `mykey=myval` as the default value so that is what is returned, which I think is definitely wrong.
    To preserve original semantics here we need to stop (D), i.e. the default values at runtime shouldl _not_ be changed, which is one option, ie only change the default value on catalog reads (which will invalidate a few of the previous test changes but that's okay and minor).
    Alternatively, and better in my view, would be to change semantics so that if a key is never inherited or not reinherited, _the attempt to find it (B)_ is unable to do so.  This feels better because the key should not even be visible to the child entity.  Thus it will not return either the original default value or the set value; it won't even know the type; and (C) will return `null` when looked up on the child entity.  This way we get away with overriding the default value.  (And, obviously if the key were defined on the child entity, we would get its originally declared default value.)
    (In addition to the failures in `ConfigInheritanceYamlTest` there is also one in `ConfigNestedYamlTest` which I'm not sure whether is related or not.)