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

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.

https://codereview.appspot.com/6256054/diff/1/juju/control/remove_relation.py
File juju/control/remove_relation.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/control/remove_relation.py#newcode72
juju/control/remove_relation.py:72: has_principal = True
this should default to false, right?

https://codereview.appspot.com/6256054/diff/1/juju/control/remove_relation.py#newcode79
juju/control/remove_relation.py:79: 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.

https://codereview.appspot.com/6256054/diff/1/juju/control/tests/test_destroy_service.py
File juju/control/tests/test_destroy_service.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/control/tests/test_destroy_service.py#newcode138
juju/control/tests/test_destroy_service.py:138: # resulted in a
traceback
This doesn't actually test anything related to the branch afaics. The
actual problem is asynchronously triggered.

https://codereview.appspot.com/6256054/diff/1/juju/hooks/tests/test_invoker.py
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/hooks/tests/test_invoker.py#newcode1480
juju/hooks/tests/test_invoker.py:1480: """Verify that port hook commands
run and changes are immediate."""
does this change belong in this branch?

https://codereview.appspot.com/6256054/diff/1/juju/state/tests/test_firewall.py
File juju/state/tests/test_firewall.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/state/tests/test_firewall.py#newcode189
juju/state/tests/test_firewall.py:189: """Verify that opening/closing
ports triggers the appropriate firewall
does this change belong in this branch?

https://codereview.appspot.com/6256054/diff/1/juju/unit/lifecycle.py
File juju/unit/lifecycle.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/unit/lifecycle.py#newcode458
juju/unit/lifecycle.py:458: def _undeploy(self, service_relation):
method name is a bit ambigious its always a suicide op, ie.
_undeploy_self..

https://codereview.appspot.com/6256054/diff/1/juju/unit/lifecycle.py#newcode459
juju/unit/lifecycle.py:459: """Conditionally stop and remove _this_
agents deployment.
s/agents/subordinate agent

https://codereview.appspot.com/6256054/diff/1/juju/unit/lifecycle.py#newcode500
juju/unit/lifecycle.py:500: 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
bogus.

https://codereview.appspot.com/6256054/diff/1/juju/unit/workflow.py
File juju/unit/workflow.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/unit/workflow.py#newcode229
juju/unit/workflow.py:229: if not stat:
how does this help? what if the node is already gone when the
retry_change fires. also untested.

https://codereview.appspot.com/6256054/