state: factor out getConfigString and setConfigString

state: factor out getConfigString and setConfigString

This code was repeated in quite a few places with minor (and
IMHO mostly unjustified) variations. We're about to add more
calls (version and proposed-version), which hopefully should
justify this even more.

https://code.launchpad.net/~rogpeppe/juju-core/state-getconfigstring/+merge/116871

(do not edit description out of merge proposal)

[rog, 2012-07-26 14:05:14]

Please take a look.

[niemeyer, 2012-07-26 16:27:21]

That's a huge improvement, thanks.

A few details:

https://codereview.appspot.com/6442051/diff/2001/state/confignode.go
File state/confignode.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/confignode.go#newcode120
state/confignode.go:120: func getConfigString(zk *zookeeper.Conn, path, what
string, attr string) (string, error) {
s/String/Attr/, I suppose?

Also, the code is concatenating stuff and using Sprintf all the time. I suggest
this instead:

func getConfigString(zk *zookeeper.Conn, path, attr string, whatFormat string,
whatArgs ...interface{}) (string, error)

This has the added benefit that we will only really call Sprintf very
infrequently.

https://codereview.appspot.com/6442051/diff/2001/state/confignode.go#newcode136
state/confignode.go:136: func setConfigString(zk *zookeeper.Conn, path, what,
attr, val string) error {
Same.

https://codereview.appspot.com/6442051/diff/2001/state/confignode.go#newcode144
state/confignode.go:144: return fmt.Errorf("cannot set %s to %q: %v", what, err,
val)
s/err, val/val, err/ (it would be a very funny error message :-)

https://codereview.appspot.com/6442051/diff/2001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/machine.go#newcode56
state/machine.go:56: return setConfigString(m.st.zk, m.zkPath(), "instance id of
machine "+m.String(), providerMachineId, id)
E.g.:

return setConfigString(m.st.zk, m.zkPath(), providerMachineId,
        "instance id of machine %s", m)

I suggest conventionally putting the what message in the following line as
above.

https://codereview.appspot.com/6442051/diff/2001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/relation.go#newcode185
state/relation.go:185: return nil, err
This should either be reverted or all the others should be changed accordingly.

https://codereview.appspot.com/6442051/diff/2001/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/relation_test.go#newco...
state/relation_test.go:195: c.Assert(err, ErrorMatches, `cannot join unit
"peer/0" to relation "peer:baz": private address of unit "peer/0" not found`)
Are tests passing? This seems to disagree with the implementation.

https://codereview.appspot.com/6442051/diff/2001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/service.go#newcode29
state/service.go:29: return nil, errors.New("service has no charm URL")
This has lost its context information.

https://codereview.appspot.com/6442051/diff/2001/state/service.go#newcode31
state/service.go:31: return charm.ParseURL(surl)
This has lost its context information.

https://codereview.appspot.com/6442051/diff/2001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/unit.go#newcode150
state/unit.go:150: return nil, errors.New("unit has no charm URL")
This has lost its context information.

https://codereview.appspot.com/6442051/diff/2001/state/unit.go#newcode152
state/unit.go:152: return charm.ParseURL(surl)
This has lost its context information.

[rog, 2012-07-26 17:17:28]

Please take a look.

https://codereview.appspot.com/6442051/diff/2001/state/confignode.go
File state/confignode.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/confignode.go#newcode120
state/confignode.go:120: func getConfigString(zk *zookeeper.Conn, path, what
string, attr string) (string, error) {
On 2012/07/26 16:27:21, niemeyer wrote:
> s/String/Attr/, I suppose?
> 
> Also, the code is concatenating stuff and using Sprintf all the time. I
suggest
> this instead:
> 
> func getConfigString(zk *zookeeper.Conn, path, attr string, whatFormat string,
> whatArgs ...interface{}) (string, error)
> 
> This has the added benefit that we will only really call Sprintf very
> infrequently.

Done, except for the first item, as discussed.

https://codereview.appspot.com/6442051/diff/2001/state/confignode.go#newcode136
state/confignode.go:136: func setConfigString(zk *zookeeper.Conn, path, what,
attr, val string) error {
On 2012/07/26 16:27:21, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/6442051/diff/2001/state/confignode.go#newcode144
state/confignode.go:144: return fmt.Errorf("cannot set %s to %q: %v", what, err,
val)
On 2012/07/26 16:27:21, niemeyer wrote:
> s/err, val/val, err/ (it would be a very funny error message :-) 

Done.

https://codereview.appspot.com/6442051/diff/2001/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/machine.go#newcode56
state/machine.go:56: return setConfigString(m.st.zk, m.zkPath(), "instance id of
machine "+m.String(), providerMachineId, id)
On 2012/07/26 16:27:21, niemeyer wrote:
> E.g.:
> 
> return setConfigString(m.st.zk, m.zkPath(), providerMachineId,
>         "instance id of machine %s", m)
> 
> I suggest conventionally putting the what message in the following line as
> above.

Done.

https://codereview.appspot.com/6442051/diff/2001/state/relation.go
File state/relation.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/relation.go#newcode185
state/relation.go:185: return nil, err
On 2012/07/26 16:27:21, niemeyer wrote:
> This should either be reverted or all the others should be changed
accordingly.

Done.

https://codereview.appspot.com/6442051/diff/2001/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/relation_test.go#newco...
state/relation_test.go:195: c.Assert(err, ErrorMatches, `cannot join unit
"peer/0" to relation "peer:baz": private address of unit "peer/0" not found`)
On 2012/07/26 16:27:21, niemeyer wrote:
> Are tests passing? This seems to disagree with the implementation.

tests pass, yes, and this one is definitely being run.

https://codereview.appspot.com/6442051/diff/2001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/service.go#newcode29
state/service.go:29: return nil, errors.New("service has no charm URL")
On 2012/07/26 16:27:21, niemeyer wrote:
> This has lost its context information.

Done.

https://codereview.appspot.com/6442051/diff/2001/state/service.go#newcode31
state/service.go:31: return charm.ParseURL(surl)
On 2012/07/26 16:27:21, niemeyer wrote:
> This has lost its context information.

Done.

https://codereview.appspot.com/6442051/diff/2001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6442051/diff/2001/state/unit.go#newcode150
state/unit.go:150: return nil, errors.New("unit has no charm URL")
On 2012/07/26 16:27:21, niemeyer wrote:
> This has lost its context information.

Done.

https://codereview.appspot.com/6442051/diff/2001/state/unit.go#newcode152
state/unit.go:152: return charm.ParseURL(surl)
On 2012/07/26 16:27:21, niemeyer wrote:
> This has lost its context information.

Done.

[niemeyer, 2012-07-26 18:21:25]

LGTM with the following fixed:

https://codereview.appspot.com/6442051/diff/7002/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6442051/diff/7002/state/machine.go#newcode57
state/machine.go:57: "instance id of machine %v", m.String())
s/m.String()/m/

https://codereview.appspot.com/6442051/diff/7002/state/machine.go#newcode65
state/machine.go:65: "instance id of machine %v", m.String())
s/m.String()/m/

https://codereview.appspot.com/6442051/diff/7002/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6442051/diff/7002/state/service.go#newcode34
state/service.go:34: return nil, fmt.Errorf("failed to parse charm URL of
service %q: %v", err)
Wrong number of arguments.

https://codereview.appspot.com/6442051/diff/7002/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6442051/diff/7002/state/unit.go#newcode159
state/unit.go:159: return nil, fmt.Errorf("failed to parse charm URL of unit %q:
%v", err)
Wrong number of arguments.

https://codereview.appspot.com/6442051/diff/7002/state/unit.go#newcode167
state/unit.go:167: "charm URL of unit %v", u)
s/%v/%q/

[rog, 2012-07-26 18:31:00]

*** Submitted:

state: factor out getConfigString and setConfigString

This code was repeated in quite a few places with minor (and
IMHO mostly unjustified) variations. We're about to add more
calls (version and proposed-version), which hopefully should
justify this even more.

R=niemeyer
CC=
https://codereview.appspot.com/6442051

https://codereview.appspot.com/6442051/diff/7002/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/6442051/diff/7002/state/machine.go#newcode57
state/machine.go:57: "instance id of machine %v", m.String())
On 2012/07/26 18:21:25, niemeyer wrote:
> s/m.String()/m/

Done.

https://codereview.appspot.com/6442051/diff/7002/state/machine.go#newcode65
state/machine.go:65: "instance id of machine %v", m.String())
On 2012/07/26 18:21:25, niemeyer wrote:
> s/m.String()/m/

Done.

https://codereview.appspot.com/6442051/diff/7002/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6442051/diff/7002/state/service.go#newcode34
state/service.go:34: return nil, fmt.Errorf("failed to parse charm URL of
service %q: %v", err)
On 2012/07/26 18:21:25, niemeyer wrote:
> Wrong number of arguments.

Done.

https://codereview.appspot.com/6442051/diff/7002/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6442051/diff/7002/state/unit.go#newcode159
state/unit.go:159: return nil, fmt.Errorf("failed to parse charm URL of unit %q:
%v", err)
On 2012/07/26 18:21:25, niemeyer wrote:
> Wrong number of arguments.

Done.

https://codereview.appspot.com/6442051/diff/7002/state/unit.go#newcode167
state/unit.go:167: "charm URL of unit %v", u)
On 2012/07/26 18:21:25, niemeyer wrote:
> s/%v/%q/

Done.