Remove DB.units list

Remove DB.units list This branch is a monster. Apologies in advance. This removes the db.units list and makes the GUI only use service.get('units') for service specific unit lists. Outside of the simulator we only ever look at units in the context of a service and keeping two copies of things was silly. This works better with the databinding code and is a pattern we intend to move forward with (wrt to relations I expect we'll get rid of the global list as well). There was however a fair bit of code that touched units which lead to a number of minor changes. Then I realized I was fixing some code in deprecated views so I went through and did a first pass at removing dead code related to the old views. Also there are changes to the test server to run on a random port which should allow safely running make check/lbox w/o getting the test server from another branch. https://code.launchpad.net/~bcsaller/juju-gui/destroyService-error/+merge/187944 (do not edit description out of merge proposal)

[bcsaller, 2013-09-26 22:24:00]

Please take a look.

[benjamin.saller, 2013-09-26 22:39:46]

Now with review notes https://codereview.appspot.com/13997043/diff/1/app/app.js File app/app.js (left): https://codereview.appspot.com/13997043/diff/1/app/app.js#oldcode550 app/app.js:550: We always had one of these around, but no more. https://codereview.appspot.com/13997043/diff/1/app/app.js#oldcode737 app/app.js:737: Prune unused routes. https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js File app/models/handlers.js (right): https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js#newcode277 app/models/handlers.js:277: resolveModelByName does the right thing, we can't use db['units'] anymore https://codereview.appspot.com/13997043/diff/1/app/models/models.js File app/models/models.js (left): https://codereview.appspot.com/13997043/diff/1/app/models/models.js#oldcode571 app/models/models.js:571: Started off just removing this... and it steamrolled. https://codereview.appspot.com/13997043/diff/1/app/models/models.js#oldcode992 app/models/models.js:992: Our current scaling expectations don't require this comment. https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js File app/store/env/fakebackend.js (right): https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n... app/store/env/fakebackend.js:471: get on modellist returns the array of values for all children by default. https://codereview.appspot.com/13997043/diff/1/app/store/env/simulator.js File app/store/env/simulator.js (right): https://codereview.appspot.com/13997043/diff/1/app/store/env/simulator.js#new... app/store/env/simulator.js:155: Couldn't walk the global units list here so I reordered things a little. https://codereview.appspot.com/13997043/diff/1/test-server.js File test-server.js (right): https://codereview.appspot.com/13997043/diff/1/test-server.js#newcode48 test-server.js:48: Bind on 0 requests an unallocated random port... Oh UNIX :) https://codereview.appspot.com/13997043/diff/1/test/test_model.js File test/test_model.js (right): https://codereview.appspot.com/13997043/diff/1/test/test_model.js#newcode21 test/test_model.js:21: charm.get('revision').should.equal(0); Bad diff, I put the whole thing in a describe so I could run them all at once, but that hides the db.units changes internally, sorry. https://codereview.appspot.com/13997043/diff/1/test/utils.js File test/utils.js (right): https://codereview.appspot.com/13997043/diff/1/test/utils.js#newcode140 test/utils.js:140: This shouldn't have made it in, but is still technically a fix... I guess. https://codereview.appspot.com/13997043/diff/1/undocumented File undocumented (left): https://codereview.appspot.com/13997043/diff/1/undocumented#oldcode1 undocumented:1: from the pruning

[jeff.pihach, 2013-09-27 14:30:59]

LGTM with trivial Thanks for putting in the time to do all of this cleanup! I'll QA shortly and respond. https://codereview.appspot.com/13997043/diff/1/app/app.js File app/app.js (left): https://codereview.appspot.com/13997043/diff/1/app/app.js#oldcode550 app/app.js:550: haha good find https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js File app/models/handlers.js (left): https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js#oldcode124 app/models/handlers.js:124: I didn't see this removed from models.js and I'm pretty sure it's not used anywhere else. https://codereview.appspot.com/13997043/diff/1/app/models/models.js File app/models/models.js (left): https://codereview.appspot.com/13997043/diff/1/app/models/models.js#oldcode571 app/models/models.js:571: lol https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js File app/store/env/fakebackend.js (right): https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n... app/store/env/fakebackend.js:265: Should this var be renamed? https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n... app/store/env/fakebackend.js:471: Interesting, maybe a inline comment to this effect https://codereview.appspot.com/13997043/diff/1/test-server.js File test-server.js (right): https://codereview.appspot.com/13997043/diff/1/test-server.js#newcode48 test-server.js:48: Maybe an inline comment to this effect. https://codereview.appspot.com/13997043/diff/1/test/test_fakebackend.js File test/test_fakebackend.js (right): https://codereview.appspot.com/13997043/diff/1/test/test_fakebackend.js#newco... test/test_fakebackend.js:122: The relations are never verified outside of the deepEquals https://codereview.appspot.com/13997043/diff/1/test/test_model.js File test/test_model.js (right): https://codereview.appspot.com/13997043/diff/1/test/test_model.js#newcode21 test/test_model.js:21: charm.get('revision').should.equal(0); Then I guess I'll just have to take your word on it :)

[gary.poster, 2013-09-27 14:55:53]

LGTM with largely irrelevant comments. This is great, thank you! Exciting to see old code start to be pruned (even though that would have been better as a separate branch or two!). Assuming lint is fixed and qa goes well with Jeff, land it! https://codereview.appspot.com/13997043/diff/1/app/models/models.js File app/models/models.js (left): https://codereview.appspot.com/13997043/diff/1/app/models/models.js#oldcode992 app/models/models.js:992: On 2013/09/26 22:39:47, benjamin.saller wrote: > Our current scaling expectations don't require this comment. +1. https://codereview.appspot.com/13997043/diff/1/app/models/models.js File app/models/models.js (right): https://codereview.appspot.com/13997043/diff/1/app/models/models.js#newcode1028 app/models/models.js:1028: else { return; } or whatever locally correct "missing" behavior is? It probably does this already, but it might be nice to be explicit. https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js File app/store/env/fakebackend.js (right): https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n... app/store/env/fakebackend.js:671: what "above"? Confused. https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n... app/store/env/fakebackend.js:1228: This little six-line snippet is roughly or duped in earlier file, IIRC. Maybe indicates good opportunity to share code, maybe not. Just noting if you hadn't considered. https://codereview.appspot.com/13997043/diff/1/app/views/charm-panel.js File app/views/charm-panel.js (left): https://codereview.appspot.com/13997043/diff/1/app/views/charm-panel.js#oldcode2 app/views/charm-panel.js:2: I love the smell of napalmed code in the morning. https://codereview.appspot.com/13997043/diff/1/test-server.js File test-server.js (right): https://codereview.appspot.com/13997043/diff/1/test-server.js#newcode48 test-server.js:48: On 2013/09/27 14:30:59, jeff.pihach wrote: > Maybe an inline comment to this effect. heh, +1 https://codereview.appspot.com/13997043/diff/1/test-server.js#newcode49 test-server.js:49: cool, thanks for doing this. https://codereview.appspot.com/13997043/diff/1/test/index.html File test/index.html (left): https://codereview.appspot.com/13997043/diff/1/test/index.html#oldcode118 test/index.html:118: I like this fix. ;-) https://codereview.appspot.com/13997043/diff/1/test/test_fakebackend.js File test/test_fakebackend.js (right): https://codereview.appspot.com/13997043/diff/1/test/test_fakebackend.js#newco... test/test_fakebackend.js:122: On 2013/09/27 14:30:59, jeff.pihach wrote: > The relations are never verified outside of the deepEquals Maybe you mean "initialize units and relations," Ben? https://codereview.appspot.com/13997043/diff/1/test/test_model.js File test/test_model.js (right): https://codereview.appspot.com/13997043/diff/1/test/test_model.js#newcode21 test/test_model.js:21: charm.get('revision').should.equal(0); On 2013/09/27 14:30:59, jeff.pihach wrote: > Then I guess I'll just have to take your word on it :) Heh, yeah. :-) https://codereview.appspot.com/13997043/diff/1/test/test_sandbox_go.js File test/test_sandbox_go.js (right): https://codereview.appspot.com/13997043/diff/1/test/test_sandbox_go.js#newcod... test/test_sandbox_go.js:384: huh, that looks like it shouldn't have worked before https://codereview.appspot.com/13997043/diff/1/test/utils.js File test/utils.js (right): https://codereview.appspot.com/13997043/diff/1/test/utils.js#newcode140 test/utils.js:140: On 2013/09/26 22:39:47, benjamin.saller wrote: > This shouldn't have made it in, but is still technically a fix... I guess. heh

[benjamin.saller, 2013-09-27 17:12:42]

Thanks for the reviews, cleaning up a little after the trunk merge, but should land soon. https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js File app/models/handlers.js (left): https://codereview.appspot.com/13997043/diff/1/app/models/handlers.js#oldcode124 app/models/handlers.js:124: On 2013/09/27 14:30:59, jeff.pihach wrote: > I didn't see this removed from models.js and I'm pretty sure it's not used > anywhere else. It is still used here, but with a little refactoring wouldn't be. https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js File app/store/env/fakebackend.js (right): https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n... app/store/env/fakebackend.js:265: On 2013/09/27 14:30:59, jeff.pihach wrote: > Should this var be renamed? unitsInError now https://codereview.appspot.com/13997043/diff/1/app/store/env/fakebackend.js#n... app/store/env/fakebackend.js:471: On 2013/09/27 14:30:59, jeff.pihach wrote: > Interesting, maybe a inline comment to this effect Done.

[bcsaller, 2013-09-27 17:20:53]

*** Submitted: Remove DB.units list This branch is a monster. Apologies in advance. This removes the db.units list and makes the GUI only use service.get('units') for service specific unit lists. Outside of the simulator we only ever look at units in the context of a service and keeping two copies of things was silly. This works better with the databinding code and is a pattern we intend to move forward with (wrt to relations I expect we'll get rid of the global list as well). There was however a fair bit of code that touched units which lead to a number of minor changes. Then I realized I was fixing some code in deprecated views so I went through and did a first pass at removing dead code related to the old views. Also there are changes to the test server to run on a random port which should allow safely running make check/lbox w/o getting the test server from another branch. R=benjamin.saller, jeff.pihach, gary.poster CC= https://codereview.appspot.com/13997043