Add Go support for removing units from services.

Add Go support for removing units from services.

- This was missed during the transition to the go backend
- Add the Go handler method to removing a unit. 
- Add a test and a note that the equiv. python test is located in another
file.

https://code.launchpad.net/~rharding/juju-gui/resolve-retry/+merge/185784

(do not edit description out of merge proposal)

[rharding, 2013-09-16 11:21:09]

Please take a look.

[gary.poster, 2013-09-16 12:34:23]

Thank you Rick.  I have a few concerns but I suspect you can address them
quickly.

Gary

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

https://codereview.appspot.com/13349047/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:1048: var res =
state.removeUnits(data.Params.UnitNames);
We minimally need to convert "error" into "Error", right?  And I suspect there
is no parallel to "warning"?

In any case, we do need at least some data conversion here, I'm pretty sure.

https://codereview.appspot.com/13349047/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/13349047/diff/1/test/test_env_go.js#newcode1316
test/test_env_go.js:1316: assert.deepEqual(msg.Params.UnitNames, ['mysql/0']);
We should have a test for the error -> Error conversion.  We should have a test
for the "can provide a callback" version of this too: that needs to show that it
converts "Error" to "err" (sadly).

https://codereview.appspot.com/13349047/diff/1/test/test_env_python.js
File test/test_env_python.js (right):

https://codereview.appspot.com/13349047/diff/1/test/test_env_python.js#newcod...
test/test_env_python.js:138: // tested via integration test in
test_sandbox_python
huh.  I wonder about including this as an actual test.  Maybe leave this but
comment out all three lines?

[rharding, 2013-09-16 12:47:42]

Comments/questions below.

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

https://codereview.appspot.com/13349047/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:1048: var res =
state.removeUnits(data.Params.UnitNames);
On 2013/09/16 12:34:23, gary.poster wrote:
> We minimally need to convert "error" into "Error", right?  And I suspect there
> is no parallel to "warning"?
> 
> In any case, we do need at least some data conversion here, I'm pretty sure.

In looking at the code, _basicReceive is doing the error to Error conversion. I
duped this based on the other methods around it.

https://codereview.appspot.com/13349047/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/13349047/diff/1/test/test_env_go.js#newcode1316
test/test_env_go.js:1316: assert.deepEqual(msg.Params.UnitNames, ['mysql/0']);
On 2013/09/16 12:34:23, gary.poster wrote:
> We should have a test for the error -> Error conversion.  We should have a
test
> for the "can provide a callback" version of this too: that needs to show that
it
> converts "Error" to "err" (sadly).

Per the other one, the error -> Error is done in _basicReceive. I can look to
add an error test though to verify it's done. 

For the 'can provide a callback' that's the resolved call and it's doing the
same thing using _basicReceive. In looking at that test, it's dispatching an
Error: response from Go and asserts result.err, is that not doing what you want?
Maybe I'm not following.

https://codereview.appspot.com/13349047/diff/1/test/test_env_python.js
File test/test_env_python.js (right):

https://codereview.appspot.com/13349047/diff/1/test/test_env_python.js#newcod...
test/test_env_python.js:138: // tested via integration test in
test_sandbox_python
On 2013/09/16 12:34:23, gary.poster wrote:
> huh.  I wonder about including this as an actual test.  Maybe leave this but
> comment out all three lines?

Yea, I just happened to be talking to Jeff and mentioned that the Python version
wasn't tested at all either. He pointed me to the tests in the other file and so
I noted them here since I was looking for the tests here. I will comment out so
that it's at least searchable in the file and gives a hint to devs on where to
look.

[gary.poster, 2013-09-16 13:13:38]

quick notes from call.  nothing new here, Rick. :-)

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

https://codereview.appspot.com/13349047/diff/1/app/store/env/sandbox.js#newco...
app/store/env/sandbox.js:1048: var res =
state.removeUnits(data.Params.UnitNames);
On 2013/09/16 12:47:42, rharding wrote:
> On 2013/09/16 12:34:23, gary.poster wrote:
> > We minimally need to convert "error" into "Error", right?  And I suspect
there
> > is no parallel to "warning"?
> > 
> > In any case, we do need at least some data conversion here, I'm pretty sure.
> 
> In looking at the code, _basicReceive is doing the error to Error conversion.
I
> duped this based on the other methods around it. 

Ah, sorry!  Forgot, and didn't look closely enough.  Agreed.

https://codereview.appspot.com/13349047/diff/1/test/test_env_go.js
File test/test_env_go.js (right):

https://codereview.appspot.com/13349047/diff/1/test/test_env_go.js#newcode1316
test/test_env_go.js:1316: assert.deepEqual(msg.Params.UnitNames, ['mysql/0']);
On 2013/09/16 12:47:42, rharding wrote:
> On 2013/09/16 12:34:23, gary.poster wrote:
> > We should have a test for the error -> Error conversion.  We should have a
> test
> > for the "can provide a callback" version of this too: that needs to show
that
> it
> > converts "Error" to "err" (sadly).
> 
> Per the other one, the error -> Error is done in _basicReceive. I can look to
> add an error test though to verify it's done. 
> 
> For the 'can provide a callback' that's the resolved call and it's doing the
> same thing using _basicReceive. In looking at that test, it's dispatching an
> Error: response from Go and asserts result.err, is that not doing what you
want?
> Maybe I'm not following.

As we discussed on the call, this is a valuable, needed test, but of the env,
not the sandbox.  We'll want sandbox tests too.  Feel free to ignore the rest of
the comments, the vast majority of which are confused and wrong. :-)

https://codereview.appspot.com/13349047/diff/1/test/test_env_python.js
File test/test_env_python.js (right):

https://codereview.appspot.com/13349047/diff/1/test/test_env_python.js#newcod...
test/test_env_python.js:138: // tested via integration test in
test_sandbox_python
On 2013/09/16 12:47:42, rharding wrote:
> On 2013/09/16 12:34:23, gary.poster wrote:
> > huh.  I wonder about including this as an actual test.  Maybe leave this but
> > comment out all three lines?
> 
> Yea, I just happened to be talking to Jeff and mentioned that the Python
version
> wasn't tested at all either. He pointed me to the tests in the other file and
so
> I noted them here since I was looking for the tests here. I will comment out
so
> that it's at least searchable in the file and gives a hint to devs on where to
> look.

Yeah OK.  As we discussed, I'd prefer to have unit-y tests here as well in the
future.  Nothing to do with this branch though.

[rharding, 2013-09-16 13:42:04]

Please take a look.

[gary.poster, 2013-09-16 13:45:34]

LGTM, thank you!

[rharding, 2013-09-16 13:50:51]

*** Submitted:

Add Go support for removing units from services.

- This was missed during the transition to the go backend
- Add the Go handler method to removing a unit. 
- Add a test and a note that the equiv. python test is located in another
file.

R=gary.poster
CC=
https://codereview.appspot.com/13349047