[WIP] Subtype setting config val: use as key’s default val

[aledsage]

For review/feedback only - failing unit tests.

Otherwise if another blueprint uses this subtype, it does not see the
config val for that key. Worst case, it says the blueprint is invalid
because the config key has no value.

---

My new unit tests pass, but it breaks existing unit tests (e.g. in ConfigParametersYamlTest.java). The logic is applied to a catalog item and also to a blueprint being deployed. Our assertions in existing tests check that the config keys of the app being deployed have particular default vals, but these are now being overridden by the blueprint.

What do folk think (cc @ahgittin) - should we use this approach, and fix the tests, or do something else?

[ahgittin]

I like this idea but the test failures highlight an edge cases that need some thinking.

BasicConfigKeyOverwriting has different resolution semantics to some of the structured key types (eg SetConfigKey) so by changing the key to have a different default value we inadvertently change how it resolves, leading in some cases to List being returned for a SetConfigKey

This can possibly be resolved by making the key-overwrite method aware of the different ConfigKey subtypes. Will be interesting to see whether that improves the test failure situation.

Re the problem you mention, default values being different now for keys that have a value set in YAML, I don't think that's a problem. It has some notable implications, e.g. if a config value is subsequently cleared the initial YAML-set value will be returned on read, rather than the original key definition default -- but I think this is okay.

[ahgittin]

I've opened aledsage#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. These are fixed as per subsequent notes.

[IGNORE BELOW THIS]

Previously I thought one of the failures was down to how uninherited keys are visible, as follows:

• 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. [UPDATE: This was only because we were deliberately fetching the key from the parent to look up] on the child.

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.)

[END OF OLD THOUGHTS - CONFIRMED THAT (D) DOES NOT HAPPEN UNLESS TEST DOES SOMETHING WEIRD WHICH IT WAS DOING]

(In addition to the failures in ConfigInheritanceYamlTest there is also one in ConfigNestedYamlTest which I'm not sure whether is related or not. UPDATE: confirmed this is fixed by some of the other fixes)

[ahgittin]

The test failures highlight one other problem: if the immediate type and parent type both declare values for a key type which should be merged, in some cases I think where a short v long name is used, it is not correctly merging. [edited, removed a few things that seemed to be a more general problem but I was mislead]

[ahgittin]

Fixed that last problem in my PR, a few StructuredConfigKey methods weren't getting invoked when it was wrapped as a BCKOverwriting. See 731b9ad . The default value doesn't include flag-set values, but we can live with that I think -- broken test added for reminder at 9374ecd.

So I think the only outstanding thing is ensuring non-visible ancestor config keys are not even seen as keys when resolving.

[ahgittin]

I've updated the long note above. The failure I thought was due to non-reinherited keys was actually due to something odd the test was doing. It was purely a case of default-changed-as-expected. However I've expanded that test in my PR to test more things, and everything is acting as it should be, with the exception of set-from-flag values are not taken as default values.

In my view, this is good to merge once my PR is merged -- if tests pass!

[nakomis]

@aledsage Can you take a look at this please to see if it is still relevant, and address the comments above, and also the failing tests if appropriate

Thanks

[nakomis]

retest this please