Deploy charms without config options

Deploy charms without config options

QA showed that charms that didn't include options would trigger a number
of minor errors progressing down code paths that expected options. This
adds a test with the puppermaster charm and show successful deployment.

https://code.launchpad.net/~bcsaller/juju-gui/deployer-sans-options/+merge/187122

(do not edit description out of merge proposal)

[bcsaller, 2013-09-24 00:07:00]

Please take a look.

[frankban, 2013-09-24 08:07:37]

LGTM with minor suggestions. Thank you Ben!

https://codereview.appspot.com/13852043/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/13852043/diff/1/app/models/charm.js#newcode78
app/models/charm.js:78: if (!val || Y.Object.keys(val).length === 0) {
It seems we might use Y.Object.size(val) here.

https://codereview.appspot.com/13852043/diff/1/app/models/charm.js#newcode763
app/models/charm.js:763: files = Object.keys(files);
s/Object/Y.Object?

https://codereview.appspot.com/13852043/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (left):

https://codereview.appspot.com/13852043/diff/1/app/views/ghost-inspector.js#o...
app/views/ghost-inspector.js:57: charm.set('options', charm.get('options'));
What? Thanks for removing this :-)

https://codereview.appspot.com/13852043/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/13852043/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:253: // not deployer proper as the code path expected
them
s/deployer/deploy

[jeff.pihach, 2013-09-24 14:53:25]

LGTM with trivial comment

https://codereview.appspot.com/13852043/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/13852043/diff/1/app/models/charm.js#newcode761
app/models/charm.js:761: var files = this.get('files') || [];
Maybe some code comments as to what's happening here and why this formatting is
necessary

[benjamin.saller, 2013-09-24 15:19:50]

Thanks for the reviews

https://codereview.appspot.com/13852043/diff/1/app/models/charm.js
File app/models/charm.js (right):

https://codereview.appspot.com/13852043/diff/1/app/models/charm.js#newcode761
app/models/charm.js:761: var files = this.get('files') || [];
On 2013/09/24 14:53:25, jeff.pihach wrote:
> Maybe some code comments as to what's happening here and why this formatting
is
> necessary

Done.

https://codereview.appspot.com/13852043/diff/1/app/models/charm.js#newcode763
app/models/charm.js:763: files = Object.keys(files);
On 2013/09/24 08:07:37, frankban wrote:
> s/Object/Y.Object?

Here we can use the native Object.keys method. We are still backing away from
YUI methods where they only wrap modern JS primitives.

https://codereview.appspot.com/13852043/diff/1/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (left):

https://codereview.appspot.com/13852043/diff/1/app/views/ghost-inspector.js#o...
app/views/ghost-inspector.js:57: charm.set('options', charm.get('options'));
On 2013/09/24 08:07:37, frankban wrote:
> What? Thanks for removing this :-)

I suspect this was an attempt to trigger a default value being added.

https://codereview.appspot.com/13852043/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/13852043/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:253: // not deployer proper as the code path expected
them
On 2013/09/24 08:07:37, frankban wrote:
> s/deployer/deploy

Ha, I've been typing that too much.

[bcsaller, 2013-09-24 15:26:47]

*** Submitted:

Deploy charms without config options

QA showed that charms that didn't include options would trigger a number
of minor errors progressing down code paths that expected options. This
adds a test with the puppermaster charm and show successful deployment.

R=frankban, jeff.pihach, benjamin.saller
CC=
https://codereview.appspot.com/13852043