state/apiserver: Last batch of Uniter API calls

state/apiserver: Last batch of Uniter API calls

This implements the last (hopefully) needed calls
in the uniter server-side API facade:
* ProviderType (added a TODO in uniter/modes to
refactor the code and use that instead of EnvironConfig,
just to get the provider type);
* EnterScope;
* LeaveScope;
* ReadSettings (working on both the local and remote unit
settings, depending on args);
* WriteSettings (needed to implement the client-side
Settings.Write() method).

Added some suggested changes from the previous CL: slight
reformatting of common_test.go to save some space; renamed
params.Relations to params.RelationUnits; Changed AuthEiter
to take GetAuthFunc arguments and return one.

In the course of writing tests for EnterScope and LeaveScope
I run into some issues with how to reliably verify the unit
has entered or left the scope. I tried using the same way as
in relationunit_test.go - RelationScopeWatcher, but it didn't
work. So I decided to add a small function to state.RelationUnit,
called InScope() bool, and updated the state tests to verify it
works. I use InScope() in the uniter_test.go

https://code.launchpad.net/~dimitern/juju-core/106-apiserver-uniter-relationunit-ops/+merge/180597

(do not edit description out of merge proposal)

[dimitern, 2013-08-16 15:34:02]

Please take a look.

[natefinch, 2013-08-16 16:48:12]

LGTM modulo a couple really minor changes

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

https://codereview.appspot.com/13055043/diff/1/state/relationunit.go#newcode318
state/relationunit.go:318: return count != 0
Can you make this count > 0 just to soothe my paranoid brain?

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

https://codereview.appspot.com/13055043/diff/1/state/relationunit_test.go#new...
state/relationunit_test.go:59: c.Assert(pr.ru0.InScope(), Equals, false)
there's a IsFalse check in juju-core/testing/checkers that produces better
output

https://codereview.appspot.com/13055043/diff/1/state/relationunit_test.go#new...
state/relationunit_test.go:81: c.Assert(pr.ru0.InScope(), Equals, true)
as above, but IsTrue, obviously ... same below here

[fwereade, 2013-08-19 09:41:50]

comments mostly as discussed online

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

https://codereview.appspot.com/13055043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:114: Settings Settings
I think this should be a map[string]string (we can use "" to signify deletes,
it's how it works already, will save us a bit of code in uniter and jujuc
eventually, I think).

https://codereview.appspot.com/13055043/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/13055043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:631: // EnvironConfig() just to get the
provider type.
Maybe add a note that it might be completely unnecessary once we have machine
addresses properly available.

https://codereview.appspot.com/13055043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:644: func (u *UniterAPI) EnterScope(args
params.RelationUnitsSettings) (params.ErrorResults, error) {
I don't think the settings need to come up here -- just grab PrivateAddress from
the unit and poke them in directly inside this method, and call it with
RelationUnits alone.

https://codereview.appspot.com/13055043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:685: func (u *UniterAPI) ReadSettings(args
params.RelationUnitPairs) (params.SettingsResults, error) {
As discussed, let's break this into (approximately) ReadSettings(rel, unit) and
ReadRemoteSettings(rel, unit, remote)

https://codereview.appspot.com/13055043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:713: func (u *UniterAPI) WriteSettings(args
params.RelationUnitsSettings) (params.ErrorResults, error) {
UpdateSettings would be nicer.

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

https://codereview.appspot.com/13055043/diff/1/state/relationunit.go#newcode318
state/relationunit.go:318: return count != 0
On 2013/08/16 16:48:12, nate.finch wrote:
> Can you make this count > 0 just to soothe my paranoid brain?

+1

https://codereview.appspot.com/13055043/diff/1/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/13055043/diff/1/worker/uniter/modes.go#newcode31
worker/uniter/modes.go:31: // block to use st.ProviderType() instead of calling
st.EnvironConfig.
more like "TODO we might be able to drop all this address stuff entirely" :)

[dimitern, 2013-08-19 10:46:59]

Please take a look.

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

https://codereview.appspot.com/13055043/diff/1/state/api/params/internal.go#n...
state/api/params/internal.go:114: Settings Settings
On 2013/08/19 09:41:50, fwereade wrote:
> I think this should be a map[string]string (we can use "" to signify deletes,
> it's how it works already, will save us a bit of code in uniter and jujuc
> eventually, I think).

Done.

https://codereview.appspot.com/13055043/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (right):

https://codereview.appspot.com/13055043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:631: // EnvironConfig() just to get the
provider type.
On 2013/08/19 09:41:50, fwereade wrote:
> Maybe add a note that it might be completely unnecessary once we have machine
> addresses properly available.

Done.

https://codereview.appspot.com/13055043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:644: func (u *UniterAPI) EnterScope(args
params.RelationUnitsSettings) (params.ErrorResults, error) {
On 2013/08/19 09:41:50, fwereade wrote:
> I don't think the settings need to come up here -- just grab PrivateAddress
from
> the unit and poke them in directly inside this method, and call it with
> RelationUnits alone.

Done.

https://codereview.appspot.com/13055043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:685: func (u *UniterAPI) ReadSettings(args
params.RelationUnitPairs) (params.SettingsResults, error) {
On 2013/08/19 09:41:50, fwereade wrote:
> As discussed, let's break this into (approximately) ReadSettings(rel, unit)
and
> ReadRemoteSettings(rel, unit, remote)

Done.

https://codereview.appspot.com/13055043/diff/1/state/apiserver/uniter/uniter....
state/apiserver/uniter/uniter.go:713: func (u *UniterAPI) WriteSettings(args
params.RelationUnitsSettings) (params.ErrorResults, error) {
On 2013/08/19 09:41:50, fwereade wrote:
> UpdateSettings would be nicer.

Done.

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

https://codereview.appspot.com/13055043/diff/1/state/relationunit.go#newcode318
state/relationunit.go:318: return count != 0
On 2013/08/16 16:48:12, nate.finch wrote:
> Can you make this count > 0 just to soothe my paranoid brain?

Deal :)

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

https://codereview.appspot.com/13055043/diff/1/state/relationunit_test.go#new...
state/relationunit_test.go:59: c.Assert(pr.ru0.InScope(), Equals, false)
On 2013/08/16 16:48:12, nate.finch wrote:
> there's a IsFalse check in juju-core/testing/checkers that produces better
> output

Yes, but this is an old test module, which I didn't want to change altogether. I
can do it in a follow-up, if you insist.

https://codereview.appspot.com/13055043/diff/1/state/relationunit_test.go#new...
state/relationunit_test.go:81: c.Assert(pr.ru0.InScope(), Equals, true)
On 2013/08/16 16:48:12, nate.finch wrote:
> as above, but IsTrue, obviously ... same below here

See above.

https://codereview.appspot.com/13055043/diff/1/worker/uniter/modes.go
File worker/uniter/modes.go (right):

https://codereview.appspot.com/13055043/diff/1/worker/uniter/modes.go#newcode31
worker/uniter/modes.go:31: // block to use st.ProviderType() instead of calling
st.EnvironConfig.
On 2013/08/19 09:41:50, fwereade wrote:
> more like "TODO we might be able to drop all this address stuff entirely" :)

Done.