Bundle promise removal and service updates

Bundle promise removal and service updates

Removed promises (merged from Rick)

Visual updates:
1) summary tab: delete charm stuff
2) change charms tab name to services
3) add service name to top of config info
4) sort services by charm name and then by service name
5) Add this text to top of Services tab: "These are the services that the bundle will deploy. Non-default configuration values are listed."
6) if charm is subordinate, do not show number of units; if charm is not subordinate and does not list number of units, show "1"

https://code.launchpad.net/~huwshimi/juju-gui/bundle-charm-list-updates/+merge/192264

(do not edit description out of merge proposal)

[huwshimi, 2013-10-22 23:12:11]

Please take a look.

[huwshimi, 2013-10-22 23:16:39]

Please take a look.

[gary.poster, 2013-10-23 06:31:10]

I'd like to talk through my comments before giving a blessing, but it looks very
close.  Thank you!

QA good.  I think I want to try and get rid of the Summary tab, but that can be
a separate branch, maybe based on some work Benji and Brad are doing.

https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/...
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/...
app/subapps/browser/views/bundle.js:120: 
_model is a pretty confusing name, IMO.  Why not simply service._charm?  Also,
this is using an underline effectively as a namespace tool, which I don't love.

https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/...
app/subapps/browser/views/bundle.js:138: 
The 'entity' is a bundle here, right?  It is called 'entity' because we are
sharing code with a charm view as well, right, so in other files 'entity' is a
charm?  If so, please add a comment explaining this (at least the fact that
this.get('entity') is a bundle).

https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/...
app/subapps/browser/views/bundle.js:142: 
I'd personally feel that this would be an easier-to-read data structure if you
changed this approach.  For instance, one example would be to build a new
services mapping that was like this: newServicesMappingExample[key] = {charm:
new Y.juju.models.Charm(charm), data: service}.

https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/...
app/subapps/browser/views/bundle.js:194: 
Per Jeff's comments
(http://jujugui.wordpress.com/2013/10/11/javascript-promise-error-handling-tri...),
this should be in a separate .then(null, function(error) {...}) block so that
you can also catch and report errors from the success function.

[rharding, 2013-10-23 13:57:52]

O> 
>
https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/...
> app/subapps/browser/views/bundle.js:120: 
> _model is a pretty confusing name, IMO.  Why not simply service._charm?  Also,
> this is using an underline effectively as a namespace tool, which I don't
love.

We have to be careful not to collide with keys that could occur in the service
block in the deployer file section. Using the _ is a "collision-avoider". What
would you prefer as an alternative?


>
https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/...
> app/subapps/browser/views/bundle.js:138: 
> The 'entity' is a bundle here, right?  It is called 'entity' because we are
> sharing code with a charm view as well, right, so in other files 'entity' is a
> charm?  If so, please add a comment explaining this (at least the fact that
> this.get('entity') is a bundle).

Yep, the details shares a lot of common code in entity-base.js which was
refactored out to share between charm and bundle details views.

[gary.poster, 2013-10-23 15:57:19]

On 2013/10/23 13:57:52, rharding wrote:
> O> 
> >
>
https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/...
> > app/subapps/browser/views/bundle.js:120: 
> > _model is a pretty confusing name, IMO.  Why not simply service._charm? 
Also,
> > this is using an underline effectively as a namespace tool, which I don't
> love.
> 
> We have to be careful not to collide with keys that could occur in the service
> block in the deployer file section. Using the _ is a "collision-avoider".

Right, that's what I meant by namespace

> What
> would you prefer as an alternative?

See my comment starting with "I'd personally feel that this would be an
easier-to-read data structure..."

> 
> 
> >
>
https://codereview.appspot.com/14920057/diff/30001/app/subapps/browser/views/...
> > app/subapps/browser/views/bundle.js:138: 
> > The 'entity' is a bundle here, right?  It is called 'entity' because we are
> > sharing code with a charm view as well, right, so in other files 'entity' is
a
> > charm?  If so, please add a comment explaining this (at least the fact that
> > this.get('entity') is a bundle).
> 
> Yep, the details shares a lot of common code in entity-base.js which was
> refactored out to share between charm and bundle details views.

Cool.

Thank you.

[huwshimi, 2013-10-23 18:48:02]

Please take a look.

[jeff.pihach, 2013-10-23 21:10:08]

LGTM after some small changes below - Thanks for putting in the time on this
one!

https://codereview.appspot.com/14920057/diff/170001/app/subapps/browser/views...
File app/subapps/browser/views/bundle.js (right):

https://codereview.appspot.com/14920057/diff/170001/app/subapps/browser/views...
app/subapps/browser/views/bundle.js:139: var attrs =
this.get('entity').getAttrs();
You have all of this information when you call this function so as a micro
performance improvement you could pass only the required data in so that this is
a utility method instead of just a private method which isn't really a private
method.

https://codereview.appspot.com/14920057/diff/170001/app/subapps/browser/views...
app/subapps/browser/views/bundle.js:165: var templateData = Y.merge(bundleData);
If bundle is a Y.Model then this should be returning a new object so this merge
is not required. Then you can use bundleData below instead of templateData

https://codereview.appspot.com/14920057/diff/170001/app/subapps/browser/views...
app/subapps/browser/views/bundle.js:198: // Fired event to test the topology is
rendered
Fire event to listen to during the tests so that we know when it's rendered

https://codereview.appspot.com/14920057/diff/170001/app/subapps/browser/views...
app/subapps/browser/views/bundle.js:221: this.set('rendered', true);
Now that render is synchronous this is no longer required and the checks can be
removed from the tests.

[huwshimi, 2013-10-24 00:20:03]

Please take a look.

[huwshimi, 2013-10-24 00:27:06]

*** Submitted:

Bundle promise removal and service updates

Removed promises (merged from Rick)

Visual updates:
1) summary tab: delete charm stuff
2) change charms tab name to services
3) add service name to top of config info
4) sort services by charm name and then by service name
5) Add this text to top of Services tab: "These are the services that the bundle
will deploy. Non-default configuration values are listed."
6) if charm is subordinate, do not show number of units; if charm is not
subordinate and does not list number of units, show "1"

R=gary.poster, rharding, jeff.pihach
CC=
https://codereview.appspot.com/14920057