Code review comment for lp:~thumper/juju-core/make-syslog-port-configurable

Revision history for this message
Andrew Wilkins (axwalk) wrote :

I see the value of a single round-trip to the API, but it seems a bit
ugly. I think someone with a better idea about the API design will have
to weigh in.

https://codereview.appspot.com/29990045/diff/1/log/syslog/config.go
File log/syslog/config.go (right):

https://codereview.appspot.com/29990045/diff/1/log/syslog/config.go#newcode135
log/syslog/config.go:135:
fmt.Sprint(slConfig.Port)

https://codereview.appspot.com/29990045/diff/1/state/address.go
File state/address.go (right):

https://codereview.appspot.com/29990045/diff/1/state/address.go#newcode125
state/address.go:125:
Difficult to parse, IMHO. I'd prefer
     stateAddresses = appendPort(addrs, config.StatePort())
     apiAddresses = appendPort(addrs, config.APIPort())
     return stateAddresses, apiAddresses, config.SyslogPort(), nil

https://codereview.appspot.com/29990045/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/29990045/diff/1/state/api/params/params.go#newcode526
state/api/params/params.go:526:
I know I'm being pedantic, but a port isn't exactly an address. Multiple
API calls isn't great, though. It would be nice if we could make bulk
calls of an arbitrary sequence of API methods.

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go
File worker/deployer/addresser.go (right):

https://codereview.appspot.com/29990045/diff/1/worker/deployer/addresser.go#newcode15
worker/deployer/addresser.go:15:
This seems like the wrong place to be talking about the API. This should
just be about domain objects, I think.

https://codereview.appspot.com/29990045/

« Back to merge proposal