|
|
|
state: make Addresses use Machine.Addresses
... and APIAddresses likewise.
This requires changing some tests so that there is
actually a machine that pretends to be the state server.
https://code.launchpad.net/~rogpeppe/juju-core/451-state-addresses/+merge/191686
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : state: make Addresses use Machine.Addresses #
Total comments: 8
Patch Set 3 : state: make Addresses use Machine.Addresses #Patch Set 4 : state: make Addresses use Machine.Addresses #
Total messages: 4
|
rog
Please take a look.
|
12 years, 2 months ago (2013年10月17日 17:58:40 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
LGTM with a couple very minor nitpicks. https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addr... File state/apiserver/common/addresses.go (right): https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addr... state/apiserver/common/addresses.go:12: // It is implemented by state.State. Not a fan of having the comment on the interface tell you what implements the interface. It's way too far from the implementation, and therefor easy for it to get out of date. Also, it just seems contrary the idea of implicit implementations. https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addr... state/apiserver/common/addresses.go:23: st AddressAndCertGetter st is a terrible name for an AddressAndCertGetter. I get that the only thing that implements it is state, but that's information external to the type, and makes the code in this file more difficult to read. https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addr... state/apiserver/common/addresses.go:28: func NewAddresser(st AddressAndCertGetter) *Addresser { as above https://codereview.appspot.com/14619045/diff/4001/state/apiserver/provisioner... File state/apiserver/provisioner/provisioner_test.go (right): https://codereview.appspot.com/14619045/diff/4001/state/apiserver/provisioner... state/apiserver/provisioner/provisioner_test.go:50: for i := 1; i < 3; i++ { can you just make this 0 to 2 instead of 1 to 3? confuses the heck out of me as-is.
Please take a look. https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addr... File state/apiserver/common/addresses.go (right): https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addr... state/apiserver/common/addresses.go:12: // It is implemented by state.State. On 2013年10月18日 13:07:33, nate.finch wrote: > Not a fan of having the comment on the interface tell you what implements the > interface. It's way too far from the implementation, and therefor easy for it to > get out of date. Also, it just seems contrary the idea of implicit > implementations. Done. https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addr... state/apiserver/common/addresses.go:23: st AddressAndCertGetter On 2013年10月18日 13:07:33, nate.finch wrote: > st is a terrible name for an AddressAndCertGetter. I get that the only thing > that implements it is state, but that's information external to the type, and > makes the code in this file more difficult to read. Done. https://codereview.appspot.com/14619045/diff/4001/state/apiserver/common/addr... state/apiserver/common/addresses.go:28: func NewAddresser(st AddressAndCertGetter) *Addresser { On 2013年10月18日 13:07:33, nate.finch wrote: > as above Done. https://codereview.appspot.com/14619045/diff/4001/state/apiserver/provisioner... File state/apiserver/provisioner/provisioner_test.go (right): https://codereview.appspot.com/14619045/diff/4001/state/apiserver/provisioner... state/apiserver/provisioner/provisioner_test.go:50: for i := 1; i < 3; i++ { On 2013年10月18日 13:07:33, nate.finch wrote: > can you just make this 0 to 2 instead of 1 to 3? confuses the heck out of me > as-is. Done.