apiserver: Deployer facade; refactoring

apiserver: Deployer facade; refactoring

This introduces the server-side Deployer API facade.
In addition to being composed by LifeGetter, Remover,
and PasswordChanger, it has WatchUnits method.

Also extracted the common ErrUnauthorized case into
its own common testing package and refactored other
facades to use it. Removed assertError and replaced
it with DeepEquals, because I think it stands out more
and reads better.

https://code.launchpad.net/~dimitern/juju-core/069-apiserver-deployer-initial/+merge/174144

(do not edit description out of merge proposal)

[dimitern, 2013-07-11 09:49:14]

Please take a look.

[mue, 2013-07-11 10:02:55]

LGTM, only some questions.

https://codereview.appspot.com/10949046/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/10949046/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:126: // StringsWatchResults holds the results for
any API call which ends up
Do we need a struct here or wouldn't 

type StringsWatchResults []StringsWatchResult

possible too?

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:17: const shortWait = 50 * time.Millisecond
Is there also a longWait? Or why shortWait?

https://codereview.appspot.com/10949046/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10949046/diff/1/state/state.go#newcode1342
state/state.go:1342: 'm': machineTagPrefix,
I like those prefix definitions. So I think they'll come for service and
environment later?

[jameinel, 2013-07-11 10:26:36]

If I understand the goal of William's getCanChange work, it is because we expect
an agent to maintain its connection to the API for a long time. Longer than the
lifetime of adding or removing a unit to that machine.
Which means when we call something like WatchUnits we need to wait to determine
what units we can see until the point where we ask to watch them. Rather than
doing it on constructing the DeployerAPI facade.

Going further, we'll probably need to even add 1 more test. Which is to start
the WatchUnits call, and while the client is watching those units, add and
remove some units and make sure the client sees them showing up and being
removed.
But that probably needs to be done on the client side.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
File state/apiserver/common/life_test.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:15: apitesting
"launchpad.net/juju-core/state/apiserver/testing"
Given we may be likely to add a launchpad.net/juju-core/state/api/testing module
in the future should this be imported as

apiservertesting ?

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:17: const shortWait = 50 * time.Millisecond
you *could* just pull this from launchpad.net/juju-core/testing/ ShortWait
And that version has doc strings :)

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:37: // Start a watcher on machine's units,
read the initial event and stop it.
Is this really the best way to get a list of a machine's units? (start and stop
a watcher), vs some sort of GetAllUnits call? (which you are actually writing
here, but feels like it should be hanging off of the entity itself or
something.)

I really don't like having a timeout here. Is there any way that we can actually
trigger it?

I sort of would prefer to assume that .Changes() will always have an initial
event and we just block on it.

To be clear, I like reasonable timeouts in test code, because breaking code
causing hanging tests is bad.

In production code timeouts tend to cause failures which is worse than just
hanging for a bit on stuff that shouldn't fail.

If you do feel like we need to have a timeout, I think multiple-seconds of
waiting is more appropriate. As the watcher needs to go and fetch information
out of the database, which might be highly loaded, etc.
Consider the case where you have 2000 machines hammering the database, and a
given machine has 200 units. How long will it take to extract the info from the
DB. I can guarantee the answer isn't 50ms. :)

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:75: return knownUnits.Contains(unitName)
This doesn't appear to be lazy enough, given what William has said about the
auth information for units on a machine.

Specifically, when an agent has connected to the API I think it keeps the
DeployerAPI object static over the lifetime of the connection.

Which means that this pre-canned list of what units are attached to that machine
is also static for the lifetime of the connection.

From what I read from William's statement, we really need to put the
getAllUnits() call inside the func(tag string) bool auth check.


On the other hand, if you really wanted to not have to poll for all units, would
it be possible to seed this set at this time, but leave the watcher running, and
when the associated units changes you can add/remove items from the set.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer_test.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:9: . "launchpad.net/gocheck"
Can we use the 'gc "launchpad.net/gocheck"' version here since these are new
tests?

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:126: {Changes: []string{"mysql/0",
"logging/0"}, StringsWatcherId: "1"},
I'm slightly concerned that the sort ordering of Changes isn't guaranteed. So we
might see 'logging/0' before "mysql/0". Should we be sorting the value of that
field? (Either in the test suite before we compare it, or in the return of the
API itself?

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:142: }
I think we need another test case.

Specifically, after you start the test case (and have s.deployer already
initialized), you should be able to add a unit to the machine, and query its
status, as well as remove a unit from a machine and have its status be
unauthorized.

So something like:
TestWatchUnitsChanging(c *gc.C) {
  // Remove the logging/0 unit from this machine
  // And add a new unit to the machine
  service2 := s.State.AddService("wordpress", AddTestingCharm(...))
  principal2 := service2.AddUnit()
  principal2.AssignToMachine(s.machine1)
  s.subordinate0.Stop()/// Not sure what the syntax is here

  args := [Same as WatchUnits]
  c.Assert(result, ...)

}

[jameinel, 2013-07-11 10:27:27]

I just realized that the permission checks apply to Remove and those apis *not*
to WatchUnits.

However, we still need a test about what happens when you try to Remove a unit
that you used to be able to see, but has already been removed, and that you
*can* remove a unit that has been added to your list of things.

[jameinel, 2013-07-11 10:31:44]

Some updates about where I was misunderstanding the code.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:75: return knownUnits.Contains(unitName)
Dimiter corrected me. getAuthFunc is called to refresh this set every time
Remove (etc) is called.

So that part is fine. Ignore this comment.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer_test.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:142: }
this doesn't actually apply to WatchUnits. It *does* apply that we should
probably check we have getAuthFunc properly integrated with Remove style
functions.

Then I would have seen that my original assumption was incorrect. :)

[fwereade, 2013-07-11 13:44:59]

Looking great, only one substantial issue (let's block remove of Alive units)
and one that's a matter of judgment (I think that machine.AssignedUnitNames()
nicely echoes unit.AssignedMachineId() and that it would be worth adding to
state, especially since the watcher technique is a little bit opaque at first
glance). YMMV, though, I'll leave it to your judgment. It's not worth a lot of
our time now...

https://codereview.appspot.com/10949046/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/10949046/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:126: // StringsWatchResults holds the results for
any API call which ends up
On 2013/07/11 10:02:55, mue wrote:
> Do we need a struct here or wouldn't 
> 
> type StringsWatchResults []StringsWatchResult
> 
> possible too?

We need structs for all parameter and return types.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
File state/apiserver/common/life_test.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:15: apitesting
"launchpad.net/juju-core/state/apiserver/testing"
On 2013/07/11 10:26:36, jameinel wrote:
> Given we may be likely to add a launchpad.net/juju-core/state/api/testing
module
> in the future should this be imported as
> 
> apiservertesting ?

+1

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:49: {Error: apitesting.UnauthorizedError},
Nice. ErrUnauthorized? YMMV.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/passwor...
File state/apiserver/common/password_test.go (left):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/passwor...
state/apiserver/common/password_test.go:61: Message: "x2 error",
maybe `{Message: "x2 error"},` on one line instead?

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:17: const shortWait = 50 * time.Millisecond
On 2013/07/11 10:26:36, jameinel wrote:
> you *could* just pull this from launchpad.net/juju-core/testing/ ShortWait
> And that version has doc strings :)

+1

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:37: // Start a watcher on machine's units,
read the initial event and stop it.
On 2013/07/11 10:26:36, jameinel wrote:
> Is this really the best way to get a list of a machine's units? (start and
stop
> a watcher), vs some sort of GetAllUnits call? (which you are actually writing
> here, but feels like it should be hanging off of the entity itself or
> something.)
> 
> I really don't like having a timeout here. Is there any way that we can
actually
> trigger it?
> 
> I sort of would prefer to assume that .Changes() will always have an initial
> event and we just block on it.
> 
> To be clear, I like reasonable timeouts in test code, because breaking code
> causing hanging tests is bad.
> 
> In production code timeouts tend to cause failures which is worse than just
> hanging for a bit on stuff that shouldn't fail.
> 
> If you do feel like we need to have a timeout, I think multiple-seconds of
> waiting is more appropriate. As the watcher needs to go and fetch information
> out of the database, which might be highly loaded, etc.
> Consider the case where you have 2000 machines hammering the database, and a
> given machine has 200 units. How long will it take to extract the info from
the
> DB. I can guarantee the answer isn't 50ms. :)
> 

Hmm, this is trickier than I thought. I agree that a timeout here is probably a
bad idea, and should be long if we decide we have to have one; and what I *want*
to do is trust the watcher implementations, which should guarantee a first event
or an error (and hence close of he changes chan).

I think I'm comfortable assuming that one of the two will happen "soon enough";
and that if that mechanism fails enough other stuff will be broken anyway that I
don't care about leaving a few API methods hanging.

However... I'm *also* not sure that this technique really wins us all that much,
in light of the not-unreasonable controversy here.
`state.Machine.AssignedUnitNames() ([]string, error)` seems like it might be
easier and simpler.

But I'd still accept this, so long as we dropped the timeout, because, hey, it's
prewritten functionality that does what we want, just not necessarily quite as
efficiently as one might hope.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer_test.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:9: . "launchpad.net/gocheck"
On 2013/07/11 10:26:36, jameinel wrote:
> Can we use the 'gc "launchpad.net/gocheck"' version here since these are new
> tests?

+1. Everybody remind me in my own CLs too please :).

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:81: 
I'm finding the service0 and principal0 bits a little hard to follow. A short
comment might help:

// The two known machines now contain the following units:
// machine 0 (not authorized): mysql/1
// machine 1 (authorized): mysql/0, logging/0

?

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:126: {Changes: []string{"mysql/0",
"logging/0"}, StringsWatcherId: "1"},
On 2013/07/11 10:26:36, jameinel wrote:
> I'm slightly concerned that the sort ordering of Changes isn't guaranteed. So
we
> might see 'logging/0' before "mysql/0". Should we be sorting the value of that
> field? (Either in the test suite before we compare it, or in the return of the
> API itself?

I don't want to guarantee sorted results over the API. Sort in the test please.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:202: c.Assert(s.subordinate0.Life(),
Equals, state.Alive)
I think it is currently correct, and may well always be correct, to require that
entities Remove()d already be Dying or Dead. This doesn't involve changing
getAuthFunc, it's just a check on the (authorized) unit once we've managed to
get a *state.Unit.

Sorry I didn't think of this before, but I think we should add and test for this
behaviour in a spirit of eager pedantry; it's easier to loosen restrictions
later than to tighten them.

[dimitern, 2013-07-11 15:14:16]

Please take a look.

https://codereview.appspot.com/10949046/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/10949046/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:126: // StringsWatchResults holds the results for
any API call which ends up
On 2013/07/11 10:02:55, mue wrote:
> Do we need a struct here or wouldn't 
> 
> type StringsWatchResults []StringsWatchResult
> 
> possible too?

It has to be a struct, so it can be properly marshaled and returned by the API
server's RPC layer.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
File state/apiserver/common/life_test.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:15: apitesting
"launchpad.net/juju-core/state/apiserver/testing"
On 2013/07/11 10:26:36, jameinel wrote:
> Given we may be likely to add a launchpad.net/juju-core/state/api/testing
module
> in the future should this be imported as
> 
> apiservertesting ?

Even if we do, it won't be here, but in the client tests, so I think we're safe.
I'd prefer to use shorter aliases when possible, which still make sense.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:15: apitesting
"launchpad.net/juju-core/state/apiserver/testing"
On 2013/07/11 13:44:59, fwereade wrote:
> On 2013/07/11 10:26:36, jameinel wrote:
> > Given we may be likely to add a launchpad.net/juju-core/state/api/testing
> module
> > in the future should this be imported as
> > 
> > apiservertesting ?
> 
> +1

But since both of you want this, ok I'll change it.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:49: {Error: apitesting.UnauthorizedError},
On 2013/07/11 13:44:59, fwereade wrote:
> Nice. ErrUnauthorized? YMMV.

Done.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/passwor...
File state/apiserver/common/password_test.go (left):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/passwor...
state/apiserver/common/password_test.go:61: Message: "x2 error",
On 2013/07/11 13:44:59, fwereade wrote:
> maybe `{Message: "x2 error"},` on one line instead?

Done.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:17: const shortWait = 50 * time.Millisecond
On 2013/07/11 10:26:36, jameinel wrote:
> you *could* just pull this from launchpad.net/juju-core/testing/ ShortWait
> And that version has doc strings :)

Ah, good idea, thanks, but see below (will remove this).

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:17: const shortWait = 50 * time.Millisecond
On 2013/07/11 10:02:55, mue wrote:
> Is there also a longWait? Or why shortWait?

I guess a better name is in order. I can change it. No longWait - this is just
the timeout used in getAllUnits to wait on the UnitsWatcher.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:37: // Start a watcher on machine's units,
read the initial event and stop it.
On 2013/07/11 10:26:36, jameinel wrote:
> Is this really the best way to get a list of a machine's units? (start and
stop
> a watcher), vs some sort of GetAllUnits call? (which you are actually writing
> here, but feels like it should be hanging off of the entity itself or
> something.)
> 
> I really don't like having a timeout here. Is there any way that we can
actually
> trigger it?
> 
> I sort of would prefer to assume that .Changes() will always have an initial
> event and we just block on it.
> 
> To be clear, I like reasonable timeouts in test code, because breaking code
> causing hanging tests is bad.
> 
> In production code timeouts tend to cause failures which is worse than just
> hanging for a bit on stuff that shouldn't fail.
> 
> If you do feel like we need to have a timeout, I think multiple-seconds of
> waiting is more appropriate. As the watcher needs to go and fetch information
> out of the database, which might be highly loaded, etc.
> Consider the case where you have 2000 machines hammering the database, and a
> given machine has 200 units. How long will it take to extract the info from
the
> DB. I can guarantee the answer isn't 50ms. :)
> 

As agreed online, will do a blocking read, trusting the watcher to deliver the
initial even in reasonable time.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer.go:37: // Start a watcher on machine's units,
read the initial event and stop it.
On 2013/07/11 13:44:59, fwereade wrote:
> On 2013/07/11 10:26:36, jameinel wrote:
> > Is this really the best way to get a list of a machine's units? (start and
> stop
> > a watcher), vs some sort of GetAllUnits call? (which you are actually
writing
> > here, but feels like it should be hanging off of the entity itself or
> > something.)
> > 
> > I really don't like having a timeout here. Is there any way that we can
> actually
> > trigger it?
> > 
> > I sort of would prefer to assume that .Changes() will always have an initial
> > event and we just block on it.
> > 
> > To be clear, I like reasonable timeouts in test code, because breaking code
> > causing hanging tests is bad.
> > 
> > In production code timeouts tend to cause failures which is worse than just
> > hanging for a bit on stuff that shouldn't fail.
> > 
> > If you do feel like we need to have a timeout, I think multiple-seconds of
> > waiting is more appropriate. As the watcher needs to go and fetch
information
> > out of the database, which might be highly loaded, etc.
> > Consider the case where you have 2000 machines hammering the database, and a
> > given machine has 200 units. How long will it take to extract the info from
> the
> > DB. I can guarantee the answer isn't 50ms. :)
> > 
> 
> Hmm, this is trickier than I thought. I agree that a timeout here is probably
a
> bad idea, and should be long if we decide we have to have one; and what I
*want*
> to do is trust the watcher implementations, which should guarantee a first
event
> or an error (and hence close of he changes chan).
> 
> I think I'm comfortable assuming that one of the two will happen "soon
enough";
> and that if that mechanism fails enough other stuff will be broken anyway that
I
> don't care about leaving a few API methods hanging.
> 
> However... I'm *also* not sure that this technique really wins us all that
much,
> in light of the not-unreasonable controversy here.
> `state.Machine.AssignedUnitNames() ([]string, error)` seems like it might be
> easier and simpler.
> 
> But I'd still accept this, so long as we dropped the timeout, because, hey,
it's
> prewritten functionality that does what we want, just not necessarily quite as
> efficiently as one might hope.

Dropping the timeout and will add AssignedUnitNames in state as a follow-up.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
File state/apiserver/deployer/deployer_test.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:9: . "launchpad.net/gocheck"
On 2013/07/11 10:26:36, jameinel wrote:
> Can we use the 'gc "launchpad.net/gocheck"' version here since these are new
> tests?

Done.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:81: 
On 2013/07/11 13:44:59, fwereade wrote:
> I'm finding the service0 and principal0 bits a little hard to follow. A short
> comment might help:
> 
> // The two known machines now contain the following units:
> // machine 0 (not authorized): mysql/1
> // machine 1 (authorized): mysql/0, logging/0
> 
> ?

Done.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:126: {Changes: []string{"mysql/0",
"logging/0"}, StringsWatcherId: "1"},
On 2013/07/11 10:26:36, jameinel wrote:
> I'm slightly concerned that the sort ordering of Changes isn't guaranteed. So
we
> might see 'logging/0' before "mysql/0". Should we be sorting the value of that
> field? (Either in the test suite before we compare it, or in the return of the
> API itself?

Good point, will sort them in the test.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:142: }
On 2013/07/11 10:31:44, jameinel wrote:
> this doesn't actually apply to WatchUnits. It *does* apply that we should
> probably check we have getAuthFunc properly integrated with Remove style
> functions.
> 
> Then I would have seen that my original assumption was incorrect. :)

Will add tests for Remove, Life and SetPasswords with changing units.

https://codereview.appspot.com/10949046/diff/1/state/apiserver/deployer/deplo...
state/apiserver/deployer/deployer_test.go:202: c.Assert(s.subordinate0.Life(),
Equals, state.Alive)
On 2013/07/11 13:44:59, fwereade wrote:
> I think it is currently correct, and may well always be correct, to require
that
> entities Remove()d already be Dying or Dead. This doesn't involve changing
> getAuthFunc, it's just a check on the (authorized) unit once we've managed to
> get a *state.Unit.
> 
> Sorry I didn't think of this before, but I think we should add and test for
this
> behaviour in a spirit of eager pedantry; it's easier to loosen restrictions
> later than to tighten them.

I'll add a separate check for Alive and a test.

https://codereview.appspot.com/10949046/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10949046/diff/1/state/state.go#newcode1342
state/state.go:1342: 'm': machineTagPrefix,
On 2013/07/11 10:02:55, mue wrote:
> I like those prefix definitions. So I think they'll come for service and
> environment later?

I could've added them now, but I prefer to add them as we need them, along with
ServiceNameFromTag and EnvironmentNameFromTag (if we need both).

[jameinel, 2013-07-11 15:51:12]

LGTM

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
File state/apiserver/common/life_test.go (right):

https://codereview.appspot.com/10949046/diff/1/state/apiserver/common/life_te...
state/apiserver/common/life_test.go:15: apitesting
"launchpad.net/juju-core/state/apiserver/testing"
On 2013/07/11 15:14:17, dimitern wrote:
> On 2013/07/11 10:26:36, jameinel wrote:
> > Given we may be likely to add a launchpad.net/juju-core/state/api/testing
> module
> > in the future should this be imported as
> > 
> > apiservertesting ?
> 
> Even if we do, it won't be here, but in the client tests, so I think we're
safe.
> I'd prefer to use shorter aliases when possible, which still make sense.

Elsewhere we've taken the approach that each short name is actually unique
across the codebase (statetesting, coretesting, stdtesting, etc)

I'd like to keep that as much as possible. So that when you see footesting you
know what it is immediately.

[fwereade, 2013-07-12 14:05:56]

LGTM, just trivials.

https://codereview.appspot.com/10949046/diff/9001/state/apiserver/deployer/de...
File state/apiserver/deployer/deployer_test.go (right):

https://codereview.appspot.com/10949046/diff/9001/state/apiserver/deployer/de...
state/apiserver/deployer/deployer_test.go:276: // Not make the subordinate dead
and try again.
s/Not/Now/

https://codereview.appspot.com/10949046/diff/9001/state/apiserver/testing/err...
File state/apiserver/testing/errors.go (right):

https://codereview.appspot.com/10949046/diff/9001/state/apiserver/testing/err...
state/apiserver/testing/errors.go:10: var ErrUnauthorized *params.Error =
&params.Error{
s/*params.Error //

https://codereview.appspot.com/10949046/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10949046/diff/9001/state/state.go#newcode603
state/state.go:603: return nil, fmt.Errorf("entity %q does not support
removing", tag)
s/removing/removal/

[dimitern, 2013-07-12 14:21:06]

Please take a look.

https://codereview.appspot.com/10949046/diff/9001/state/apiserver/deployer/de...
File state/apiserver/deployer/deployer_test.go (right):

https://codereview.appspot.com/10949046/diff/9001/state/apiserver/deployer/de...
state/apiserver/deployer/deployer_test.go:276: // Not make the subordinate dead
and try again.
On 2013/07/12 14:05:56, fwereade wrote:
> s/Not/Now/

Done.

https://codereview.appspot.com/10949046/diff/9001/state/apiserver/testing/err...
File state/apiserver/testing/errors.go (right):

https://codereview.appspot.com/10949046/diff/9001/state/apiserver/testing/err...
state/apiserver/testing/errors.go:10: var ErrUnauthorized *params.Error =
&params.Error{
On 2013/07/12 14:05:56, fwereade wrote:
> s/*params.Error //

Done.

https://codereview.appspot.com/10949046/diff/9001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/10949046/diff/9001/state/state.go#newcode603
state/state.go:603: return nil, fmt.Errorf("entity %q does not support
removing", tag)
On 2013/07/12 14:05:56, fwereade wrote:
> s/removing/removal/

Done.