Code review - Issue 77820044: Add EnvironCapability to state.Policyhttps://codereview.appspot.com/2014-04-03T03:42:37+00:00rietveld
Message from unknown
2014-03-19T14:51:49+00:00axwurn:md5:4402e6bb7198fc97546516c84af57e21
Message from axwalk@gmail.com
2014-03-19T14:52:09+00:00axwurn:md5:b753875ac5dade73b7b27da0cbc5763c
Please take a look.
Message from fwereade@gmail.com
2014-03-19T15:15:17+00:00fwereadeurn:md5:91b8608232370ee6c1d9c0d074e49133
LGTM. Don't quite love the "Base" name but I guess its semantic payload is close enough to reality that I'm not going to fight it. If you can think of something better, though...
Message from horacio.duran@canonical.com
2014-03-19T18:37:43+00:00hduranurn:md5:bc4887d2042290f5eacbf83bd59b3f0b
On 2014/03/19 15:15:17, fwereade wrote:
> LGTM. Don't quite love the "Base" name but I guess its semantic payload is close
> enough to reality that I'm not going to fight it. If you can think of something
> better, though...
You could user Common instead of Base altough I am not sure if it represents correctly reality
Message from axwalk@gmail.com
2014-03-20T03:32:47+00:00axwurn:md5:26c4b5a714b4e1bad2f7c958f8c0be93
On 2014/03/19 18:37:43, hduran wrote:
> On 2014/03/19 15:15:17, fwereade wrote:
> > LGTM. Don't quite love the "Base" name but I guess its semantic payload is
> close
> > enough to reality that I'm not going to fight it. If you can think of
> something
> > better, though...
>
> You could user Common instead of Base altough I am not sure if it represents
> correctly reality
Kind of, but not really. Some providers may override the behaviour.
After discussing with rogpeppe on IRC, I decided to replace EnvironBase
with two types: NopPrechecker and DoesSupportUnitPlacement. They're
more specific, and have non-generic names to imply non-changing behaviour.
If we just use "DefaultPrechecker" then someone may change the default
behaviour without considering how it affects one of the providers.
Message from unknown
2014-03-20T03:35:56+00:00axwurn:md5:e6b50556afebe1b2372776174eadf400
Message from axwalk@gmail.com
2014-03-20T03:36:02+00:00axwurn:md5:ded77bb05b27eb36be7b51f565da9dcc
Please take a look.
Message from fwereade@gmail.com
2014-03-24T09:45:56+00:00fwereadeurn:md5:318ab0ab4c778944372d99478deddac8
LGTM modulo further naming quibbles which I will leave to your judgment.
https://codereview.appspot.com/77820044/diff/20001/provider/azure/environ.go
File provider/azure/environ.go (right):
https://codereview.appspot.com/77820044/diff/20001/provider/azure/environ.go#newcode55
provider/azure/environ.go:55: common.DoesSupportUnitPlacement
Not honestly loving these names either (although they're certainly better). Would you be OK tacking "Policy" onto those names to make their intent a little clearer to the reader? Alternatively, tell me that's a stupid idea, and go ahead with what you have.
Message from unknown
2014-03-24T14:46:34+00:00axwurn:md5:a242fe41f15d6ab277c62713c71365b9
Message from axwalk@gmail.com
2014-03-24T14:46:38+00:00axwurn:md5:d78555086555edfc964d24d18a35f21c
Please take a look.
https://codereview.appspot.com/77820044/diff/20001/provider/azure/environ.go
File provider/azure/environ.go (right):
https://codereview.appspot.com/77820044/diff/20001/provider/azure/environ.go#newcode55
provider/azure/environ.go:55: common.DoesSupportUnitPlacement
On 2014/03/24 09:45:57, fwereade wrote:
> Not honestly loving these names either (although they're certainly better).
> Would you be OK tacking "Policy" onto those names to make their intent a little
> clearer to the reader? Alternatively, tell me that's a stupid idea, and go ahead
> with what you have.
Renamed NopPrechecker to NopPrecheckerPolicy, DoesSupportUnitPlacement to SupportsUnitPlacementPolicy.
Message from fwereade@gmail.com
2014-03-27T12:29:50+00:00fwereadeurn:md5:f48954d534b5a6c04b6fdb0db9dc6d28
On 2014/03/24 14:46:38, axw wrote:
> Please take a look.
>
> https://codereview.appspot.com/77820044/diff/20001/provider/azure/environ.go
> File provider/azure/environ.go (right):
>
> https://codereview.appspot.com/77820044/diff/20001/provider/azure/environ.go#newcode55
> provider/azure/environ.go:55: common.DoesSupportUnitPlacement
> On 2014/03/24 09:45:57, fwereade wrote:
> > Not honestly loving these names either (although they're certainly better).
> > Would you be OK tacking "Policy" onto those names to make their intent a
> little
> > clearer to the reader? Alternatively, tell me that's a stupid idea, and go
> ahead
> > with what you have.
>
> Renamed NopPrechecker to NopPrecheckerPolicy, DoesSupportUnitPlacement to
> SupportsUnitPlacementPolicy.
If that's all you did this still LGTM
Message from unknown
2014-04-03T03:42:18+00:00axwurn:md5:3414ee75b4f72301b783799a098ac80d
Message from axwalk@gmail.com
2014-04-03T03:42:37+00:00axwurn:md5:9c36e323a697a3520ef7a3b503354856
Please take a look.