Code review comment for lp:~frankban/juju-quickstart/uncommitted-bundles

Revision history for this message
Richard Harding (rharding) wrote :

LGTM, I think most of my questions are answered in some way or another.
I understand that this is really checking is the version of the charm
ready, not the gui, so checking the version.js isn't a perfect match. I
still can't help but feel that the charm revision is tied to the gui
release inside and maybe it's a better overall solution to this.
However, this is a perfectly reasonable way to go about it and should
work out most of the time.

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

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py#newcode659
quickstart/app.py:659: 'deployed Juju GUI charm ({}): requested bundle
deployment '
should we hint at a juju-gui upgrade here?

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py#newcode663
quickstart/app.py:663: print('requesting uncommitted deployment of {}
with the following '
"uncommitted deployment" to "uncomitted loading"? deployment just
strikes me as very...deployed.

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py#newcode679
quickstart/app.py:679: return response['Token']
does this open the gui url as it does when you just run juju-quickstart
without the deployment?

https://codereview.appspot.com/247800043/diff/1/quickstart/jujugui.py
File quickstart/jujugui.py (right):

https://codereview.appspot.com/247800043/diff/1/quickstart/jujugui.py#newcode62
quickstart/jujugui.py:62: def is_promulgated(reference):
I don't get this part. Can this not work if the gui you deployed is a
local deployment of the GUI?

Should this better be a check for a flag/config/call to the GUI source
instead? Can it just check something like a check against version.js in
some way?

https://codereview.appspot.com/247800043/diff/1/quickstart/jujutools.py
File quickstart/jujutools.py (right):

https://codereview.appspot.com/247800043/diff/1/quickstart/jujutools.py#newcode60
quickstart/jujutools.py:60: if not jujugui.is_promulgated(charm_ref):
oic, so we assume. I still wonder about using version.js vs
is_promulgated.

https://codereview.appspot.com/247800043/

« Back to merge proposal