Display all unit status categories in unit list

Display all unit status categories in unit list

The inspector unit list will now display all unit error types split out each
into their own section. 

It now also has the ability to display all unit pending types but there are
other components which are holding this functionality back. To see it in
action load up the GUI with the 'simPend' flag. It will now show 
'installing', 'dying' as well as 'pending'. This is just for illustrative
purposes.

https://code.launchpad.net/~hatch/juju-gui/split-unit-error/+merge/196405

(do not edit description out of merge proposal)

[jeff.pihach, 2013-11-22 23:06:08]

Please take a look.

[jeff.pihach, 2013-11-22 23:18:57]

Reviewer notes.

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js#newcode1237
app/views/utils.js:1237: utils.simplifyState = function(unit) {
The other code was only there to support pyJuju

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js#newcode1242
app/views/utils.js:1242: return state;
Support any kind of agent state

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js#newcode1276
app/views/utils.js:1276: return 'pending';
Because we now support all agent types this method returns us the 'category
type' of the agent state.

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
File app/views/viewlets/service-overview.js (left):

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:165: pending: ['retry', 'remove'],
juju-core does not support retrying items stuck in pending.

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
File app/views/viewlets/service-overview.js (right):

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:33: return status.category;
Here the status.category is the long error name from agent_state_info
(StatusInfo)

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:36: return status.category + ' units';
anything that isn't an error, running, or landscape is considered 'pending'

ie) 'dying', 'installing' etc

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:127: function sortStatuses(statuses) {
We no longer show 'all' categories because we have no idea what those will be,
so this sorts them by category type.

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:162: function addCharmUpgrade(statuses,
service) {
This is just to break up the huge monolithic function a bit.

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:511: categoryWrapperNodes.order();
D3 orders the DOM elements to match the data set.

https://codereview.appspot.com/31190043/diff/1/lib/views/juju-inspector.less
File lib/views/juju-inspector.less (left):

https://codereview.appspot.com/31190043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:973: &.pending {
We now control these by the category type so no need to hide them with css

https://codereview.appspot.com/31190043/diff/1/lib/views/juju-inspector.less
File lib/views/juju-inspector.less (right):

https://codereview.appspot.com/31190043/diff/1/lib/views/juju-inspector.less#...
lib/views/juju-inspector.less:924: .status-unit-header.landscape,
oops - removed locally

https://codereview.appspot.com/31190043/diff/1/test/test_inspector_overview.js
File test/test_inspector_overview.js (left):

https://codereview.appspot.com/31190043/diff/1/test/test_inspector_overview.j...
test/test_inspector_overview.js:430: units.add({ id: 'mysql/0', agent_state:
'install-error' });
in juju-core it's simply 'error' with agent_state_info explaining why

https://codereview.appspot.com/31190043/diff/1/test/test_inspector_overview.j...
test/test_inspector_overview.js:469: var wrapper4 = unitListWrappers.item(4);
We can no longer hide/show categories to keep them in order because we don't
know what categories will make their way down the pipe so these 'hidden'
categories no longer exist.

https://codereview.appspot.com/31190043/diff/1/test/test_inspector_overview.j...
test/test_inspector_overview.js:860: 'landscape-security-upgrades': {landscape:
true}
no unit with security upgrades? it's not created.

https://codereview.appspot.com/31190043/diff/1/test/test_inspector_overview.js
File test/test_inspector_overview.js (right):

https://codereview.appspot.com/31190043/diff/1/test/test_inspector_overview.j...
test/test_inspector_overview.js:273: agent_state_info: 'hook failed: "install"'
agent_state_info is now required for all errors as it turns into the header
value

https://codereview.appspot.com/31190043/diff/1/test/test_inspector_overview.j...
test/test_inspector_overview.js:615: assert.equal(unitListWrappers.size(), 2);
again, if there is nothing with additional statuses, these elements are not
created.

https://codereview.appspot.com/31190043/diff/1/test/test_utils.js
File test/test_utils.js (left):

https://codereview.appspot.com/31190043/diff/1/test/test_utils.js#oldcode715
test/test_utils.js:715: it('returns "error" if there is a relation error',
function() {
It no longer parses relation errors for errors - juju-core puts the unit
agent_state to 'error' if there is a relation error.

[rharding, 2013-11-25 16:55:32]

Code looks ok with a couple of comments below. Please verify the categoryType
usage and maybe verify there's a test there that should either be failing or
updated? 

Will QA next.

https://codereview.appspot.com/31190043/diff/1/app/store/env/simulator.js
File app/store/env/simulator.js (right):

https://codereview.appspot.com/31190043/diff/1/app/store/env/simulator.js#new...
app/store/env/simulator.js:161: // Because the GUI does not yet support parsing
the 'life'
Should this be an XXX tied to the bug so that we can make sure we go back here
based on the implementation they do in core?

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js#newcode1237
app/views/utils.js:1237: utils.simplifyState = function(unit) {
On 2013/11/22 23:18:58, jeff.pihach wrote:
> The other code was only there to support pyJuju

Are we officially done with pyjuju then since this is not backwards compatible?

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js#newcode1240
app/views/utils.js:1240: if ((/-?error$/).test(state)) { return 'error'; }
if the - id optional why include it at all?

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
File app/views/viewlets/service-overview.js (right):

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:60: 'upgrade-service': function(status) {
upgrade service is not one of the category types allowed now correct?

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:68: var name =
nameMap[status.type][status.categoryType];
 this requires no change to the above nameMap for a service?

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:85: function updateStatusList(unitList) {
<3 meaningful names

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:138: ib = indexMap[b.categoryType];
why does this function need to be redefined every time sortStatuses is called?

[rharding, 2013-11-25 17:12:59]

The UI on the 42machines need to be restarted section moves when you expand it.
The ^ moves to the next line and the icon jumps. 

The same is true for db-relation-changed.

Ok, so this is due to the introduction of the scroll bar, should we take this
space into account as in other parts of the UI?

[rharding, 2013-11-25 17:31:09]

QA ok, LGTM as long as the other notes are addressed.

[gary.poster, 2013-11-25 17:38:02]

Thank you for this work, Jeff.  Cool.

I'm concerned about the direction of simplifyState and friend, as I mention
below.  I'm OK with landing as long as you and I talk through this and you
address my concerns (by convincing me I'm wrong or agreeing on next steps for a
follow-on branch).

I'd also prefer to be more measured about dropping PyJuju support entirely.  I'd
prefer to announce that a particular new release will be the last to support
PyJuju and be able to react to users, than retroactively announce that the last
release was the last to support pyJuju, and we're sorry that this new releases
hoses you.  I'm open to forgiveness/permission arguments or similar here, but
that's how I feel now.

LGTM with those two.

Thank you!

Gary

https://codereview.appspot.com/31190043/diff/1/app/store/env/simulator.js
File app/store/env/simulator.js (right):

https://codereview.appspot.com/31190043/diff/1/app/store/env/simulator.js#new...
app/store/env/simulator.js:160: if (window.flags && window.flags.simPend) {
"simPend"? "simPend"?? "simPend"???

;-)

https://codereview.appspot.com/31190043/diff/1/app/store/env/simulator.js#new...
app/store/env/simulator.js:161: // Because the GUI does not yet support parsing
the 'life'
On 2013/11/25 16:55:32, rharding wrote:
> Should this be an XXX tied to the bug so that we can make sure we go back here
> based on the implementation they do in core?

Well, it's an implementation in the GUI.  IIUC, we'll need to change our
aggregation code.

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js#newcode1237
app/views/utils.js:1237: utils.simplifyState = function(unit) {
On 2013/11/25 16:55:32, rharding wrote:
> On 2013/11/22 23:18:58, jeff.pihach wrote:
> > The other code was only there to support pyJuju
> 
> Are we officially done with pyjuju then since this is not backwards
compatible?

We have permission to do this, but it would be a lot better to *first* announce
that a particular release is the last that will support Python, and *then* make
a release that only supports Go, I think.  Open to counterarguments.

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js#newcode1237
app/views/utils.js:1237: utils.simplifyState = function(unit) {
So IIUC, this is where we want to look at life and have new categories--in other
words, we have the unit here so we can come up with our own artificial state,
yeah?

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
File app/views/viewlets/service-overview.js (right):

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:111: categoryType:
utils.determineCategoryType(category),
I feel like there's some better underlying path to get here.  Specifically, I
feel like the categorization code in the utils should be categorizing with
richer objects, for instance, rather than simply strings.  We can probably do
this in a follow-on branch, but I'm a bit concerned about it so I'd like to talk
through it.

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:138: ib = indexMap[b.categoryType];
On 2013/11/25 16:55:32, rharding wrote:
> why does this function need to be redefined every time sortStatuses is called?


It doesn't, but...this is pretty common JS, yeah?  it's what you do with sort,
forEach, map, reduce, ...tons of stuff.  Isn't this an idiom, and a reasonable
one, for the language?

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:162: function addCharmUpgrade(statuses,
service) {
On 2013/11/22 23:18:58, jeff.pihach wrote:
> This is just to break up the huge monolithic function a bit.

Nice direction.

[jeff.pihach, 2013-11-25 17:57:32]

Thanks for the reviews! - comments below and code changes will be landing.

https://codereview.appspot.com/31190043/diff/1/app/store/env/simulator.js
File app/store/env/simulator.js (right):

https://codereview.appspot.com/31190043/diff/1/app/store/env/simulator.js#new...
app/store/env/simulator.js:160: if (window.flags && window.flags.simPend) {
Well at first I had simulatePending but Rick gets mad when flags are too long
haha!

https://codereview.appspot.com/31190043/diff/1/app/store/env/simulator.js#new...
app/store/env/simulator.js:161: // Because the GUI does not yet support parsing
the 'life'
I could add an XXX but the 'real' functionality is already implemented in core
the GUI just doesn't aggregate the other 'status' properties properly.

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/31190043/diff/1/app/views/utils.js#newcode1240
app/views/utils.js:1240: if ((/-?error$/).test(state)) { return 'error'; }
This allows the various error types returned by PyJuju to still show as 'error'
but the removal of the relationErrors code causes PyJuju backends to report
services with a relation error was 'running'.

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
File app/views/viewlets/service-overview.js (right):

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:60: 'upgrade-service': function(status) {
as per IRC I'll add some comments to clarify how this method works

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:68: var name =
nameMap[status.type][status.categoryType];
Nope, the status.type remains the same so it all works as expected.

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:85: function updateStatusList(unitList) {
heh it was driving me nuts :D

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:111: categoryType:
utils.determineCategoryType(category),
I agree, I have this, and other methods written down to focus on refactoring in
the 'scaling issues' card but this was the 'best' way to handle it without other
refactorings which I felt fell out of scope of this branch

https://codereview.appspot.com/31190043/diff/1/app/views/viewlets/service-ove...
app/views/viewlets/service-overview.js:138: ib = indexMap[b.categoryType];
Yeah this is a pretty common syntax with js. The downside is that the
interpreter has to recompile the function every time it's called so it cannot
optimize it. Moving it out into a static function will probably help with
performance albeit probably a trivial gain.

[jeff.pihach, 2013-11-25 19:04:01]

*** Submitted:

Display all unit status categories in unit list

The inspector unit list will now display all unit error types split out each
into their own section. 

It now also has the ability to display all unit pending types but there are
other components which are holding this functionality back. To see it in
action load up the GUI with the 'simPend' flag. It will now show 
'installing', 'dying' as well as 'pending'. This is just for illustrative
purposes.

R=rharding, gary.poster
CC=
https://codereview.appspot.com/31190043