Github user ahgittin commented on the issue:

    https://github.com/apache/brooklyn-server/pull/281
  
    I've discovered a problem with this /cc @grkvlt @sjcorbett @aledsage 
@neykov.
    
    Some blueprints set some of these keys e.g. `pre.install.command` as 
anonymous config on a non-software-process parent entity (ie where there is no 
corresponding config key) for the express intention that it get picked up by 
multiple different software process descendants.  (One common example is 
machine preparation, e.g. setting up monitoring agents and mounting an extra 
disk.)
    
    But with this change the config is excluded and those commands no longer 
run.
    
    (I also notice that `Entity.getAllConfig()` and the UI config view both 
include ancestor config which is not actually inherited; seems we aren't 
correctly attending to inheritance there.)
    
    A proper way might be (A) to emit a "machine-sshable" sensor as part of 
`SoftwareProcess` and define an `InvokeEffector...` policy at the parent to 
(A.A) invoke an `SshCommandEffector` (with added capability of specifying the 
emitter as the effector target) and then emit another sensor on the emitter 
which unblocks an install latch there), or to (A.B) add and start a child at 
the emitter, and the child runs the ssh commands and emits the sensor.
    
    However it would be very nice to support (B) existing practice eg 
pre-install config being specified in the parent, especially if we can do so in 
a way which addresses the problem this PR fixes where we don't want 
pre-install-command passed from parent software processes to children software 
processes.  As this change is only very recently introduced I think we should 
limit the backwards-incompatibility it introduces before the next release, 
especially to support (B).  (We can do what is necessary for (A.A) and (A.B) 
and suggest people migrate from (B) to (A.B) or (A.A) as best practice but as 
(B) is so easy I don't want to lose it.)
    
    As a concrete way of doing this we could extend how we deal with 
`ConfigInheritance` so that we attend to different behaviours from 
parents/ancestors/bequeathers and children/descendants/inheritors.
    
    Also it's a bit of a smell that we have an `enum` and a `class` that are 
basically the same, and both acting as hints with the logic for dealing with 
inheritance at the callers. Could this be cleaned up, eg with logic in this 
class?  (Especially if we are adding new modes!)
    
    More to follow.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to