Code review comment for lp:~bcsaller/pyjuju/subordinate-removes

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

Needs Fixing

There seem to be two separate implementations of unrelated issues here
that would ideally be in separate branches. One for open/close port. And
one for subordinate suicide on removal. I'm focusing this review on
subordinate suicide aspect.
File juju/control/ (right):
juju/control/ has_principal = True
this should default to false, right?
juju/control/ service_pair.insert(0, service)
pls document here why the subordinate is coming first in the ep pair.
its easy to gloss over this is just an error message formatting thing.
File juju/control/tests/ (right):
juju/control/tests/ # resulted in a
This doesn't actually test anything related to the branch afaics. The
actual problem is asynchronously triggered.
File juju/hooks/tests/ (right):
juju/hooks/tests/ """Verify that port hook commands
run and changes are immediate."""
does this change belong in this branch?
File juju/state/tests/ (right):
juju/state/tests/ """Verify that opening/closing
ports triggers the appropriate firewall
does this change belong in this branch?
File juju/unit/ (right):
juju/unit/ def _undeploy(self, service_relation):
method name is a bit ambigious its always a suicide op, ie.
juju/unit/ """Conditionally stop and remove _this_
agents deployment.
s/agents/subordinate agent
juju/unit/ yield unit_deployer.start("subordinate")
are you sure about that? i don't see anything obvious there, if so its a
bug in the deployer.. start would imply deploying again.. which is
File juju/unit/ (right):
juju/unit/ if not stat:
how does this help? what if the node is already gone when the
retry_change fires. also untested.

« Back to merge proposal