config: Require strictly-positive timeout values #764
Conversation
|
LGTM |
|
I don't think the pointer should be removed here |
Because? We removed similar unnecessary pointers in #653. |
|
This was done for similar reasons (which I presume are nulls being supported) in image-spec recently, it seems like a valid place here: opencontainers/image-spec#633 Just seagulling, hope it's ok. |
|
On Thu, Apr 13, 2017 at 08:26:40AM -0700, Erik Hollensbe wrote:
This was done for similar reasons (which I presume are nulls being
supported) in image-spec recently, it seems like a valid place here:
opencontainers/image-spec#633
In that case (and also in opencontainers/image-spec#625 and
opencontainers/image-spec#626), you need a pointer for the optional
property, because of golang/go#11939. However, in this case ‘Timeout’
is an int, and (unlike structs) those *do* have zero values that
omitempty supports. So the non-pointer int property works, and is in
line with our current policy [1] (since #653 / #656). Maybe the
policy is closer to:
New properties should not have pointer Go types when the zero value
works with omitempty and is a no-op or invalid. For existing
properties, maintainers will decide on a case-by-case basis with no
recognizable pattern, so don't worry too much about trying to guess
what they want.
;)
[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0-rc5/style.md#optional-settings-should-not-have-pointer-go-types
|
specs-go/config.go
Outdated
| @@ -126,7 +126,7 @@ type Hook struct { | |||
| Path string `json:"path"` | |||
| Args []string `json:"args,omitempty"` | |||
| Env []string `json:"env,omitempty"` | |||
| Timeout *int `json:"timeout,omitempty"` | |||
There was a problem hiding this comment.
Add back the pointer to this field so we can validate a actual user provided 0 value.
There was a problem hiding this comment.
Add back the pointer to this field so we can validate a actual user provided 0 value.
This seems like it would be an issue with the string properties de-pointerized by #653. I think that validation tools should be using the JSON Schema or interface{} parsing to check this sort of thing (pull request to get that to happen in opencontainers/runtime-tools#197). If the goal is to be able to distinguish “unset” from “set to an invalid/no-op value”, then we do want a pointer here, but probably also want to get the pointer back for CgroupsPath, etc. Or is there a reason why we want to be able to easily identify “timeout set to 0” in Go but don't need that for “cgroupsPath set to an empty string”?
If the timeout value was zero, the hook would always error, and there doesn't seem to be much point to that. And I'm not sure what negative timeouts would mean. By adding this restriction, we do not limit useful hook entries, and we give the validation code grounds to warn users attempting to validate configs which are poorly defined or have useless hook entries. I'd like to remove the pointer from the Go type to comply with our anti-pointer zero-value style (style.md) now that Go's zero-value is clearly invalid. However, there has been maintainer resistance to removing the pointer [1] (although I don't think this is consistent with previous maintainer statements that we don't need pointers when the zero value is invalid [2]). In order to land the normative spec change, this commit keeps the current *int for Hook.Timeout and punts a consistent policy to future work. [1]: opencontainers#764 (comment) [2]: opencontainers#653 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>
wking
force-pushed
the
strictly-positive-timeout
branch
from
8510055
to
ecf7314
Compare
wking
force-pushed
the
strictly-positive-timeout
branch
from
0de1973
to
ecf7314
Compare
|
LGTM |
|
LGTM |
If the timeout value was zero, the hook would always error, and there doesn't seem to be much point to that. And I'm not sure what negative timeouts would mean. By adding this restriction, we do not limit useful hook entries, and we give the validation code grounds to warn users attempting to validate configs which are poorly defined or have useless hook entries.
Removing the pointer is not strictly required, but keeps up with our anti-pointer zero-value style now that Go's zero-value is clearly invalid.