Subordinates should not be deployed with unit ct.

Subordinates should not be deployed with unit ct.

Deploying a subordinate with a unit count causes an error in core.  This prevents one from entering a unit count in the charm panel (did not touch ghost inspector, that may also need it) and defaults to 0.  This also affects fakebackend.

https://code.launchpad.net/~makyo/juju-gui/core-sub-num-units/+merge/176583

(do not edit description out of merge proposal)

[matthew.scott, 2013-07-24 03:50:48]

Please take a look.

[matthew.scott, 2013-07-24 03:52:22]

It's way past my bedtime, will gladly rework anything in the morning.

[rharding, 2013-07-24 11:56:51]

LGTM though I think it'd be nice to have new tests to more clearly walk through
that complex if conditional adjusted in fakebackend.

https://codereview.appspot.com/11478046/diff/1/app/templates/charm-pre-config...
File app/templates/charm-pre-configuration.handlebars (right):

https://codereview.appspot.com/11478046/diff/1/app/templates/charm-pre-config...
app/templates/charm-pre-configuration.handlebars:32: <div class="charm-entry">
Indent this please. I missed the if/else my first read through.

https://codereview.appspot.com/11478046/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/11478046/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:171: fakebackend.deploy('cs:wordpress', callback,
{unitCount: 0});
Do we not have tests directly against the conditions in the fakebackend? A
subordinate with > 0 unit count, with undefined, with 0?

Updating this test seems to help it pass without error, but doesn't seem to
directly apply to the changes made and the original issue.

[gary.poster, 2013-07-24 12:23:06]

LGTM with comments.  I'm going to either try to get changes done for this and
landed, or work on the other critical bug you filed last night.  If you are
going to do one of those, please check with me.

Thank you!

Gary

https://codereview.appspot.com/11478046/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (left):

https://codereview.appspot.com/11478046/diff/1/app/store/env/fakebackend.js#o...
app/store/env/fakebackend.js:613: if (service && service.get('is_subordinate'))
{
Ignorable suggestion.

I'd prefer to have better error messages here, and so keep the rough outline of
what we had before.  Therefore, I'd keep the deletions above in lines 603-608,
and have this section (613-615) instead read as follows:

if (service.get('is_subordinate')) {
  if (numUnits !== 0) {
    return {error: serviceName + ' is a subordinate, cannot add unit(s).'};
  }
} else if (numUnits < 0) {
  return {error: 'Invalid number of units.'};
}

https://codereview.appspot.com/11478046/diff/1/app/templates/charm-pre-config...
File app/templates/charm-pre-configuration.handlebars (right):

https://codereview.appspot.com/11478046/diff/1/app/templates/charm-pre-config...
app/templates/charm-pre-configuration.handlebars:32: <div class="charm-entry">
On 2013/07/24 11:56:51, rharding wrote:
> Indent this please. I missed the if/else my first read through.

+1 (he means the else clause body--maybe that's obvious but it took me a second)

https://codereview.appspot.com/11478046/diff/1/test/test_fakebackend.js
File test/test_fakebackend.js (right):

https://codereview.appspot.com/11478046/diff/1/test/test_fakebackend.js#newco...
test/test_fakebackend.js:171: fakebackend.deploy('cs:wordpress', callback,
{unitCount: 0});
On 2013/07/24 11:56:51, rharding wrote:
> Do we not have tests directly against the conditions in the fakebackend? A
> subordinate with > 0 unit count, with undefined, with 0?
> 
> Updating this test seems to help it pass without error, but doesn't seem to
> directly apply to the changes made and the original issue.

+1

[gary.poster, 2013-07-24 12:57:43]

https://codereview.appspot.com/11478046/diff/1/app/store/env/fakebackend.js
File app/store/env/fakebackend.js (left):

https://codereview.appspot.com/11478046/diff/1/app/store/env/fakebackend.js#o...
app/store/env/fakebackend.js:613: if (service && service.get('is_subordinate'))
{
On 2013/07/24 12:23:07, gary.poster wrote:
> Ignorable suggestion.
> 
> I'd prefer to have better error messages here, and so keep the rough outline
of
> what we had before.  Therefore, I'd keep the deletions above in lines 603-608,
> and have this section (613-615) instead read as follows:
> 
> if (service.get('is_subordinate')) {
>   if (numUnits !== 0) {
>     return {error: serviceName + ' is a subordinate, cannot add unit(s).'};
>   }
> } else if (numUnits < 0) {
>   return {error: 'Invalid number of units.'};
> }

nm.  this is buggy.  sticking with yours.