Merge into trunk : config-3-state-unit-settings-rename : Code : juju-core

Status:	Merged
Approved by:	William Reade on 2013-06-11
Approved revision:	no longer in the source branch.
Merged at revision:	1272
Proposed branch:	lp://staging/~fwereade/juju-core/config-3-state-unit-settings-rename
Merge into:	lp://staging/~juju/juju-core/trunk
Prerequisite:	lp://staging/~fwereade/juju-core/config-2-trivial-error-message-change
Diff against target:	0 lines
To merge this branch:	bzr merge lp://staging/~fwereade/juju-core/config-3-state-unit-settings-rename
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju Engineering		2013-06-11	Pending
Review via email: mp+168577@code.staging.launchpad.net

Description of the change

state: Unit.ConfigSettings

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

https://codereview.appspot.com/10169044/

Revision history for this message

William Reade (fwereade) wrote on 2013-06-11:

#

Download full text (8.7 KiB)

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, D...

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: fwereade@gmail.com-20130611014406-26bzbnzb26z2cw1l
+New revision: fwereade@gmail.com-20130611014641-i03c8dxuq08l1x9l

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",

Revision history for this message

Tim Penhey (thumper) wrote on 2013-06-11:

#

On 2013/06/11 01:49:24, fwereade wrote:
> Please take a look.

LGTM - no other comments.

https://codereview.appspot.com/10169044/

Revision history for this message

Ian Booth (wallyworld) wrote on 2013-06-11:

#

LGTM

https://codereview.appspot.com/10169044/

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2013-06-11:

#

LGTM

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

https://codereview.appspot.com/10169044/diff/1/worker/uniter/context.go#newcode97
worker/uniter/context.go:97: for name, value := range ctx.configSettings
{
do we really need to make a copy?

https://codereview.appspot.com/10169044/

Revision history for this message

William Reade (fwereade) wrote on 2013-06-12:

#

*** Submitted:

state: Unit.ConfigSettings

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

R=thumper, wallyworld, rog
CC=
https://codereview.appspot.com/10169044

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

https://codereview.appspot.com/10169044/diff/1/worker/uniter/context.go#newcode97
worker/uniter/context.go:97: for name, value := range ctx.configSettings
{
On 2013/06/11 12:10:04, rog wrote:
> do we really need to make a copy?

Better safe than sorry. I'm growing more paranoid in my old age.

https://codereview.appspot.com/10169044/

juju-core

Merge lp://staging/~fwereade/juju-core/config-3-state-unit-settings-rename into lp://staging/~juju/juju-core/trunk

Commit message

Description of the change

Preview Diff

Subscribers