Code review comment for lp:~themue/juju-core/go-state-unit-status

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Not there yet.

https://codereview.appspot.com/6454113/diff/2002/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode36
state/unit.go:36: // UnitStatus represents the status of the unit's
agent.
s/'s//

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode39
state/unit.go:39: const (
As discussed on IRC with William:

http://paste.ubuntu.com/1148913/

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode54
state/unit.go:54: // UnitInfo allows to add additional info, e.g. an
error reason
This seems like a better plan:

Aug 14 11:53:31 <fwereade_> TheMue, so, agreed, I think:
SetStatus(UnitStatus, string) error; Status() (UnitStatus, string,
error); ?

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode200
state/unit.go:200: info, err := getConfigString(u.st.zk, u.zkPath(),
"status-info",
What if the status changed since you last looked?

Grab the config node once and use it rather than reloading it every time
you feel the need to look at its content.

https://codereview.appspot.com/6454113/diff/2002/state/unit.go#newcode229
state/unit.go:229: return setConfigString(u.st.zk, u.zkPath(),
"status-info", info,
Ditto.

https://codereview.appspot.com/6454113/

« Back to merge proposal