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