Code review comment for lp:~frankban/juju-quickstart/new-bootstrap-strategy

Revision history for this message
Brad Crittenden (bac) wrote :

Code LGTM with minor comments. Will QA now.

https://codereview.appspot.com/172380043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/172380043/diff/1/quickstart/app.py#newcode245
quickstart/app.py:245: # For this reason, a call to status() is usually
required at this point.
This is no different from the calling perspective than the 'not already
bootstrapped' situation, right?

If so, perhaps that should be noted in the docstring for this function.

https://codereview.appspot.com/172380043/diff/1/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/172380043/diff/1/quickstart/manage.py#newcode520
quickstart/manage.py:520: already_bootstrapped = app.bootstrap(
What is the scenario for api_url being None but it is already
bootstrapped? Someone deleted the jenv file out from under juju?

https://codereview.appspot.com/172380043/diff/1/quickstart/netutils.py
File quickstart/netutils.py (right):

https://codereview.appspot.com/172380043/diff/1/quickstart/netutils.py#newcode50
quickstart/netutils.py:50: timeout = 3
Make timeout an arg with default = 3?

https://codereview.appspot.com/172380043/

« Back to merge proposal