Code review comment for lp:~waigani/juju-core/lxc-autorestart-setting

Revision history for this message
Jesse Meek (waigani) wrote :

Please take a look.

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go
File container/lxc/lxc.go (right):

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go#newcode47
container/lxc/lxc.go:47: autorestart bool
On 2014/03/10 03:56:48, thumper wrote:
> normally camel case: "autoRestart"

> yes logdir is wrong

Done.

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go#newcode70
container/lxc/lxc.go:70: }
On 2014/03/17 17:42:23, hduran wrote:
> Wouldn't this whole block be equivalent to:

> autoRestart := conf["autoRestart"] == "" || conf["autoRestart"] ==
"true"

> it seems a bit odd to me to do if true then true if false then false,
but it is
> just an opinion.

Done. Much better. I didn't know you could do that in go, but now I
think about it - of course you can.

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go#newcode80
container/lxc/lxc.go:80: func (manager *containerManager) AutoRestart()
bool {
On 2014/03/10 03:56:48, thumper wrote:
> this doesn't need to be exported

Done. It was a leftover from debugging.

https://codereview.appspot.com/73390043/

« Back to merge proposal