Code review comment for lp://staging/~fwereade/juju-core/config-3-state-unit-settings-rename

Reviewers: mp+168577_code.launchpad.net,

Message:
Please take a look.

Description:
state: Unit.ConfigSettings

Better name, better type; suitable changes propagated amongst clients.

https://code.launchpad.net/~fwereade/juju-core/config-3-state-unit-settings-rename/+merge/168577

Requires:
https://code.launchpad.net/~fwereade/juju-core/config-2-trivial-error-message-change/+merge/168576

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/10169044/

Affected files:
   A [revision details]
   M state/unit.go
   M state/unit_test.go
   M worker/uniter/context.go
   M worker/uniter/context_test.go
   M worker/uniter/jujuc/config-get.go
   M worker/uniter/jujuc/context.go
   M worker/uniter/jujuc/util_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: state/unit.go
=== modified file 'state/unit.go'
--- state/unit.go 2013-06-10 20:37:49 +0000
+++ state/unit.go 2013-06-10 21:04:37 +0000
@@ -98,8 +98,11 @@
   return u.st.Service(u.doc.Service)
  }

-// ServiceConfig returns the contents of this unit's service configuration.
-func (u *Unit) ServiceConfig() (map[string]interface{}, error) {
+// ConfigSettings returns the complete set of service charm config settings
+// available to the unit. Unset values will be replaced with the default
+// value for the associated option, and may thus be nil when no default is
+// specified.
+func (u *Unit) ConfigSettings() (charm.Settings, error) {
   if u.doc.CharmURL == nil {
    return nil, fmt.Errorf("unit charm not set")
   }

Index: state/unit_test.go
=== modified file 'state/unit_test.go'
--- state/unit_test.go 2013-06-10 20:37:49 +0000
+++ state/unit_test.go 2013-06-10 21:04:37 +0000
@@ -45,30 +45,30 @@
   c.Assert(svc.Name(), Equals, s.unit.ServiceName())
  }

-func (s *UnitSuite) TestServiceConfigNeedsCharmURLSet(c *C) {
- _, err := s.unit.ServiceConfig()
+func (s *UnitSuite) TestConfigSettingsNeedCharmURLSet(c *C) {
+ _, err := s.unit.ConfigSettings()
   c.Assert(err, ErrorMatches, "unit charm not set")
  }

-func (s *UnitSuite) TestServiceConfigIncludesDefaults(c *C) {
+func (s *UnitSuite) TestConfigSettingsIncludeDefaults(c *C) {
   err := s.unit.SetCharmURL(s.charm.URL())
   c.Assert(err, IsNil)
- settings, err := s.unit.ServiceConfig()
+ settings, err := s.unit.ConfigSettings()
   c.Assert(err, IsNil)
- c.Assert(settings, DeepEquals, map[string]interface{}{"blog-title": "My
Title"})
+ c.Assert(settings, DeepEquals, charm.Settings{"blog-title": "My Title"})
  }

-func (s *UnitSuite) TestServiceConfigReflectsService(c *C) {
+func (s *UnitSuite) TestConfigSettingsReflectService(c *C) {
   err := s.service.SetConfig(map[string]string{"blog-title": "no title"})
   c.Assert(err, IsNil)
   err = s.unit.SetCharmURL(s.charm.URL())
   c.Assert(err, IsNil)
- settings, err := s.unit.ServiceConfig()
+ settings, err := s.unit.ConfigSettings()
   c.Assert(err, IsNil)
- c.Assert(settings, DeepEquals, map[string]interface{}{"blog-title": "no
title"})
+ c.Assert(settings, DeepEquals, charm.Settings{"blog-title": "no title"})
  }

-func (s *UnitSuite) TestServiceConfigReflectsCharm(c *C) {
+func (s *UnitSuite) TestConfigSettingsReflectCharm(c *C) {
   err := s.unit.SetCharmURL(s.charm.URL())
   c.Assert(err, IsNil)
   newCharm := s.AddConfigCharm(c, "wordpress", "options: {}", 123)
@@ -76,16 +76,16 @@
   c.Assert(err, IsNil)

   // Settings still reflect charm set on unit.
- settings, err := s.unit.ServiceConfig()
+ settings, err := s.unit.ConfigSettings()
   c.Assert(err, IsNil)
- c.Assert(settings, DeepEquals, map[string]interface{}{"blog-title": "My
Title"})
+ c.Assert(settings, DeepEquals, charm.Settings{"blog-title": "My Title"})

   // When the unit has the new charm set, it'll see the new config.
   err = s.unit.SetCharmURL(newCharm.URL())
   c.Assert(err, IsNil)
- settings, err = s.unit.ServiceConfig()
+ settings, err = s.unit.ConfigSettings()
   c.Assert(err, IsNil)
- c.Assert(settings, DeepEquals, map[string]interface{}{})
+ c.Assert(settings, DeepEquals, charm.Settings{})
  }

  func (s *UnitSuite) TestGetSetPublicAddress(c *C) {

Index: worker/uniter/context.go
=== modified file 'worker/uniter/context.go'
--- worker/uniter/context.go 2013-05-27 07:53:44 +0000
+++ worker/uniter/context.go 2013-06-10 21:04:37 +0000
@@ -7,6 +7,7 @@
   "bufio"
   "fmt"
   "io"
+ "launchpad.net/juju-core/charm"
   "launchpad.net/juju-core/log"
   "launchpad.net/juju-core/state"
   "launchpad.net/juju-core/worker/uniter/jujuc"
@@ -23,8 +24,8 @@
  type HookContext struct {
   unit *state.Unit

- // config holds the service configuration.
- config map[string]interface{}
+ // configSettings holds the service configuration.
+ configSettings charm.Settings

   // id identifies the context.
   id string
@@ -84,15 +85,19 @@
   return ctx.unit.ClosePort(protocol, port)
  }

-func (ctx *HookContext) Config() (map[string]interface{}, error) {
- if ctx.config == nil {
+func (ctx *HookContext) ConfigSettings() (charm.Settings, error) {
+ if ctx.configSettings == nil {
    var err error
- ctx.config, err = ctx.unit.ServiceConfig()
+ ctx.configSettings, err = ctx.unit.ConfigSettings()
    if err != nil {
     return nil, err
    }
   }
- return ctx.config, nil
+ result := charm.Settings{}
+ for name, value := range ctx.configSettings {
+ result[name] = value
+ }
+ return result, nil
  }

  func (ctx *HookContext) HookRelation() (jujuc.ContextRelation, bool) {

Index: worker/uniter/context_test.go
=== modified file 'worker/uniter/context_test.go'
--- worker/uniter/context_test.go 2013-06-04 21:43:25 +0000
+++ worker/uniter/context_test.go 2013-06-10 21:04:37 +0000
@@ -7,6 +7,7 @@
   "fmt"
   "io/ioutil"
   . "launchpad.net/gocheck"
+ "launchpad.net/juju-core/charm"
   "launchpad.net/juju-core/juju/testing"
   "launchpad.net/juju-core/state"
   "launchpad.net/juju-core/utils"
@@ -571,9 +572,9 @@

  func (s *InterfaceSuite) TestConfigCaching(c *C) {
   ctx := s.GetContext(c, -1, "")
- cfg, err := ctx.Config()
+ settings, err := ctx.ConfigSettings()
   c.Assert(err, IsNil)
- c.Assert(cfg, DeepEquals, map[string]interface{}{"blog-title": "My
Title"})
+ c.Assert(settings, DeepEquals, charm.Settings{"blog-title": "My Title"})

   // Change remote config.
   node, err := s.service.Config()
@@ -583,9 +584,9 @@
   c.Assert(err, IsNil)

   // Local view is not changed.
- cfg, err = ctx.Config()
+ settings, err = ctx.ConfigSettings()
   c.Assert(err, IsNil)
- c.Assert(cfg, DeepEquals, map[string]interface{}{"blog-title": "My
Title"})
+ c.Assert(settings, DeepEquals, charm.Settings{"blog-title": "My Title"})
  }

  type HookContextSuite struct {

Index: worker/uniter/jujuc/config-get.go
=== modified file 'worker/uniter/jujuc/config-get.go'
--- worker/uniter/jujuc/config-get.go 2013-05-02 15:55:42 +0000
+++ worker/uniter/jujuc/config-get.go 2013-06-10 21:04:37 +0000
@@ -42,15 +42,15 @@
  }

  func (c *ConfigGetCommand) Run(ctx *cmd.Context) error {
- cfg, err := c.ctx.Config()
+ settings, err := c.ctx.ConfigSettings()
   if err != nil {
    return err
   }
   var value interface{}
   if c.Key == "" {
- value = cfg
+ value = settings
   } else {
- value, _ = cfg[c.Key]
+ value, _ = settings[c.Key]
   }
   return c.out.Write(ctx, value)
  }

Index: worker/uniter/jujuc/context.go
=== modified file 'worker/uniter/jujuc/context.go'
--- worker/uniter/jujuc/context.go 2013-05-02 15:55:42 +0000
+++ worker/uniter/jujuc/context.go 2013-06-10 21:04:37 +0000
@@ -5,6 +5,7 @@

  import (
   "fmt"
+ "launchpad.net/juju-core/charm"
   "strconv"
   "strings"
  )
@@ -32,7 +33,7 @@
   ClosePort(protocol string, port int) error

   // Config returns the current service configuration of the executing unit.
- Config() (map[string]interface{}, error)
+ ConfigSettings() (charm.Settings, error)

   // HookRelation returns the ContextRelation associated with the executing
   // hook if it was found, and whether it was found.

Index: worker/uniter/jujuc/util_test.go
=== modified file 'worker/uniter/jujuc/util_test.go'
--- worker/uniter/jujuc/util_test.go 2013-05-23 03:31:31 +0000
+++ worker/uniter/jujuc/util_test.go 2013-06-10 21:04:37 +0000
@@ -8,6 +8,7 @@
   "fmt"
   "io"
   . "launchpad.net/gocheck"
+ "launchpad.net/juju-core/charm"
   "launchpad.net/juju-core/state"
   "launchpad.net/juju-core/utils/set"
   "launchpad.net/juju-core/worker/uniter/jujuc"
@@ -96,8 +97,8 @@
   return nil
  }

-func (c *Context) Config() (map[string]interface{}, error) {
- return map[string]interface{}{
+func (c *Context) ConfigSettings() (charm.Settings, error) {
+ return charm.Settings{
    "monsters": false,
    "spline-reticulation": 45.0,
    "title": "My Title",