Import support for deployer format

Import support for deployer format Databases can import deployer format dumps with some options around handling items as ghosts or not. https://code.launchpad.net/~bcsaller/juju-gui/import-deployer/+merge/179955 (do not edit description out of merge proposal)

[bcsaller, 2013-08-13 16:27:07]

Please take a look.

[benjamin.saller, 2013-08-13 21:17:38]

I found an issue with the inherits logic, its supposed to do multi, not single inheritence, I'll repush after the update.

[bcsaller, 2013-08-13 23:12:25]

Please take a look.

[matthew.scott, 2013-08-14 17:04:23]

LGTM with comments, most of which are trivial or docs, with the big one being ensuring this works with the charm unification work. Thanks! https://codereview.appspot.com/12866043/diff/5001/app/models/models.js File app/models/models.js (right): https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:516: @param {Database} database to resolve charms/services on. Trivial: @param {Database} db https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:519: @return {Object} A hash with four keys: service (the associated "A two-element array of hashes with four keys, one for each endpoint:...", correct? https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1027: database. This modified the database its called on directly. \ Trivial: modifies, -\ https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1159: return Y.batch.apply(this, charms) Neat! https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1178: if (!source.relations) { return;} Trivial: Previous use was {return;}, use before was { return; }. Might be nice to settle on one. https://codereview.appspot.com/12866043/diff/5001/app/store/charm.js File app/store/charm.js (right): https://codereview.appspot.com/12866043/diff/5001/app/store/charm.js#newcode169 app/store/charm.js:169: promiseCharm: function(charmId, cache) { I haven't looked too closely, but it'd be worth ensuring this mixes gracefully with jcsackett's work. https://codereview.appspot.com/12866043/diff/5001/test/test_model.js File test/test_model.js (right): https://codereview.appspot.com/12866043/diff/5001/test/test_model.js#newcode800 test/test_model.js:800: it('detect service id collisions', function(done) { Trivial: detects

[rharding, 2013-08-14 18:21:07]

Please give this a look over. Some trivial, some questions, and it looks like a missing test. I'm also not a fan of the change of the store.charm response. Let me know if I'm missing the argument for the change. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js File app/models/models.js (right): https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:523: */ * up a space https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:526: function(endpoint) { this should fit on the line before and clean up indentation a little bit. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:549: */ * back a space https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:552: Y.each([0, 1], function(providedIndex) { can this just be [0, 1].forEach? I don't get the Y bit. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:555: provides = Y.merge(providingEndpoint.charm.get('provides') || {}), I don't get this. You're doing a Y.merge, but doing an || {} so there's not two items to merge here? Ok, after looking down at line 601 and trying to figure out the reasoning I see the idea in the YUI docs of doing a shallow copy using merge. Maybe a comment on the *trick* here for that. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:557: requires = Y.merge(requiringEndpoint.charm.get('requires') || {}); same as above https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1049: var rewriteIds = options.rewriteIds || false; why bother with the || false? https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1056: if (!data) {return;} move to the top and avoid the setup as well? https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1061: // Buils out a object will inherited properties. will/with? https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1174: //if (!useGhost) {} so should this if statement be cleared away? https://codereview.appspot.com/12866043/diff/5001/app/store/charm.js File app/store/charm.js (right): https://codereview.appspot.com/12866043/diff/5001/app/store/charm.js#newcode173 app/store/charm.js:173: 'success': function(c) { resolve(c.charm);}, why not keep the api consistent vs changing the rest of the code to be aware of an object with .charm? This also then breaks metadata like related charms because I can never use a promiseCharm to generate a charm with a related charm information. https://codereview.appspot.com/12866043/diff/5001/app/store/env/fakebackend.js File app/store/env/fakebackend.js (right): https://codereview.appspot.com/12866043/diff/5001/app/store/env/fakebackend.j... app/store/env/fakebackend.js:288: var charm = self._getCharmFromData(data.charm || data); not a fan of this especially without a comment as to why there's two possible types of data to get into here. https://codereview.appspot.com/12866043/diff/5001/test/test_model.js File test/test_model.js (right): https://codereview.appspot.com/12866043/diff/5001/test/test_model.js#newcode803 test/test_model.js:803: a: {services: {mysql: { gah, 3 objects opened in one line the burning :) https://codereview.appspot.com/12866043/diff/5001/test/test_model.js#newcode867 test/test_model.js:867: remove one line https://codereview.appspot.com/12866043/diff/5001/test/test_model.js#newcode933 test/test_model.js:933: it('should properly create units when not ghosted'); missing test? https://codereview.appspot.com/12866043/diff/5001/test/utils.js File test/utils.js (right): https://codereview.appspot.com/12866043/diff/5001/test/utils.js#newcode151 test/utils.js:151: callbacks.success(response.charm); again, this is changing the api response of the normal .charm call to be the .charm vs the response itself.

[rharding, 2013-08-14 18:34:01]

LGTM though without the change to charm not returning the response but response.charm.

[benjamin.saller, 2013-08-14 20:38:42]

Thanks for the reviews, fixed the promiseCharm interface issue. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js File app/models/models.js (right): https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:519: @return {Object} A hash with four keys: service (the associated On 2013/08/14 17:04:23, matthew.scott wrote: > "A two-element array of hashes with four keys, one for each endpoint:...", > correct? I tried to improve the text here, this method (and I think two others) were ported from fakebackend so I didn't spend much time on the text. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:526: function(endpoint) { On 2013/08/14 18:21:07, rharding wrote: > this should fit on the line before and clean up indentation a little bit. Done. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:555: provides = Y.merge(providingEndpoint.charm.get('provides') || {}), On 2013/08/14 18:21:07, rharding wrote: > I don't get this. You're doing a Y.merge, but doing an || {} so there's not two > items to merge here? > > Ok, after looking down at line 601 and trying to figure out the reasoning I see > the idea in the YUI docs of doing a shallow copy using merge. Maybe a comment on > the *trick* here for that. We (and I think YUI) use this trick a fair number of places, but I can comment it. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1049: var rewriteIds = options.rewriteIds || false; On 2013/08/14 18:21:07, rharding wrote: > why bother with the || false? Trying to be explicit about the defaults. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1056: if (!data) {return;} On 2013/08/14 18:21:07, rharding wrote: > move to the top and avoid the setup as well? Done. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1061: // Buils out a object will inherited properties. On 2013/08/14 18:21:07, rharding wrote: > will/with? Yes, also got s/Build/ at the front which I think is funnier to miss. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1159: return Y.batch.apply(this, charms) On 2013/08/14 17:04:23, matthew.scott wrote: > Neat! Yeah, I really like the promise style for this type of async operation. The resulting test read well without tons of nesting as well. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1174: //if (!useGhost) {} On 2013/08/14 18:21:07, rharding wrote: > so should this if statement be cleared away? I left it as a placeholder to show intent, but some people are (quite reasonably) bothered more by commented out code. I could go either way with it. https://codereview.appspot.com/12866043/diff/5001/app/models/models.js#newcod... app/models/models.js:1178: if (!source.relations) { return;} On 2013/08/14 17:04:23, matthew.scott wrote: > Trivial: Previous use was {return;}, use before was { return; }. Might be nice > to settle on one. Done. https://codereview.appspot.com/12866043/diff/5001/app/store/charm.js File app/store/charm.js (right): https://codereview.appspot.com/12866043/diff/5001/app/store/charm.js#newcode173 app/store/charm.js:173: 'success': function(c) { resolve(c.charm);}, On 2013/08/14 18:21:07, rharding wrote: > why not keep the api consistent vs changing the rest of the code to be aware of > an object with .charm? This also then breaks metadata like related charms > because I can never use a promiseCharm to generate a charm with a related charm > information. Yeah, I made this method, then merged jc's work and when things broke quickly changed this. You're correct though, I'll return the full data here and deal with it in the caller. https://codereview.appspot.com/12866043/diff/5001/app/store/charm.js#newcode173 app/store/charm.js:173: 'success': function(c) { resolve(c.charm);}, On 2013/08/14 18:21:07, rharding wrote: > why not keep the api consistent vs changing the rest of the code to be aware of > an object with .charm? This also then breaks metadata like related charms > because I can never use a promiseCharm to generate a charm with a related charm > information. Done. https://codereview.appspot.com/12866043/diff/5001/test/test_model.js File test/test_model.js (right): https://codereview.appspot.com/12866043/diff/5001/test/test_model.js#newcode933 test/test_model.js:933: it('should properly create units when not ghosted'); On 2013/08/14 18:21:07, rharding wrote: > missing test? This is related to that commented out line of code we talked about before. If we start creating units then we'll need the test so its left as pending (and shows up that way if you haven't seen that feature of Mocha). https://codereview.appspot.com/12866043/diff/5001/test/utils.js File test/utils.js (right): https://codereview.appspot.com/12866043/diff/5001/test/utils.js#newcode151 test/utils.js:151: callbacks.success(response.charm); On 2013/08/14 18:21:07, rharding wrote: > again, this is changing the api response of the normal .charm call to be the > .charm vs the response itself. Done.

[bcsaller, 2013-08-14 20:46:24]

*** Submitted: Import support for deployer format Databases can import deployer format dumps with some options around handling items as ghosts or not. R=benjamin.saller, matthew.scott, rharding CC= https://codereview.appspot.com/12866043