-

https://code.launchpad.net/~wwitzel3/juju-core/008-ha-rsyslog/+merge/217208

(do not edit description out of merge proposal)

[wwitzel, 2014-05-08 10:02:04]

Please take a look.

[wwitzel, 2014-05-08 11:17:23]

Please take a look.

[fwereade, 2014-05-08 11:57:04]

Tests?

And please set descriptions on your CLs :).

Code looks basically fine, but I'm probably not qualified to detect problems
there; so long as it's live tested, and the code that's been live-tested grows
unit tests to match the verified behaviour, I'll be fine.

https://codereview.appspot.com/100270043/diff/20001/utils/syslog/config.go
File utils/syslog/config.go (right):

https://codereview.appspot.com/100270043/diff/20001/utils/syslog/config.go#ne...
utils/syslog/config.go:77: // Each forwarding rule must be repeated in full, for
every state server and
s/full,/full/
s/server/server,/

https://codereview.appspot.com/100270043/diff/20001/utils/syslog/config.go#ne...
utils/syslog/config.go:229: "bootstrapHosts": bootstrapHosts,
please drop the "bootstrap" terminology, and use "stateServer" instead
throughout.

[wwitzel3, 2014-05-08 14:17:10]

I really don't know of a better way to unit test this other than what is already
happening. The test/syslogconf.go file contains an expected result, I've updated
that expect result so that all of the config_test.go tests pass.

https://codereview.appspot.com/100270043/diff/20001/utils/syslog/config.go
File utils/syslog/config.go (right):

https://codereview.appspot.com/100270043/diff/20001/utils/syslog/config.go#ne...
utils/syslog/config.go:77: // Each forwarding rule must be repeated in full, for
every state server and
On 2014/05/08 11:57:04, fwereade wrote:
> s/full,/full/
> s/server/server,/

Done.

https://codereview.appspot.com/100270043/diff/20001/utils/syslog/config.go#ne...
utils/syslog/config.go:229: "bootstrapHosts": bootstrapHosts,
On 2014/05/08 11:57:04, fwereade wrote:
> please drop the "bootstrap" terminology, and use "stateServer" instead
> throughout.

Done.

[wwitzel, 2014-05-08 14:19:00]

Please take a look.

[fwereade, 2014-05-08 16:21:37]

I feel like we should at least have a test with multiple state server addresses,
and it didn't look like we had that. With that, this would LGTM.