Code review comment for lp:~frankban/juju-quickstart/more-cloud-providers

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

Code looks good, a couple of comments.

Starting QA.

https://codereview.appspot.com/52080044/diff/1/quickstart/__init__.py
File quickstart/__init__.py (right):

https://codereview.appspot.com/52080044/diff/1/quickstart/__init__.py#newcode25
quickstart/__init__.py:25: VERSION = (1, 0, 0)
wahoo!

https://codereview.appspot.com/52080044/diff/1/quickstart/models/envs.py
File quickstart/models/envs.py (right):

https://codereview.appspot.com/52080044/diff/1/quickstart/models/envs.py#newcode529
quickstart/models/envs.py:529: fields.ChoiceField(
should this be a suggestion field to help users move along faster? Just
suggest a sensible default like US East which I think even things like
the AMZ web ui does? It's a suggestion field in the open stack example
above.

https://codereview.appspot.com/52080044/diff/1/quickstart/tests/cli/test_forms.py
File quickstart/tests/cli/test_forms.py (right):

https://codereview.appspot.com/52080044/diff/1/quickstart/tests/cli/test_forms.py#newcode174
quickstart/tests/cli/test_forms.py:174: class
TestCreateStringWidget(ChoicesTestsMixin, unittest.TestCase):
I think that the suggestion field is its own new class and should have
its own tests vs getting shoed into the String widget tests. This seems
like a path down like the one in the charm where the one suite of tests
contained others that weren't directly related.

https://codereview.appspot.com/52080044/

« Back to merge proposal