Skip to content

Change all duration fields in the API to time.Duration #6013

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bgrant0607 opened this issue Mar 26, 2015 · 5 comments
Closed

Change all duration fields in the API to time.Duration #6013

bgrant0607 opened this issue Mar 26, 2015 · 5 comments
Labels
area/api Indicates an issue on api area. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@bgrant0607
Copy link
Member

We have a number of fields in the API that represent an integer number of seconds, such as TimeoutSeconds. We should push the units into the value instead, by using time.Duration. That would be similar to the approach used for resource quantities.

I don't consider this blocking for v1beta3, but seems like a worthwhile change to make in the future.

cc @lavalamp @smarterclayton

@bgrant0607 bgrant0607 added area/api Indicates an issue on api area. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Mar 26, 2015
@thockin
Copy link
Member

thockin commented Mar 27, 2015

caution when using non-dumb-struct types in the API. For example, they can
not always be fuzzed or compared generically. See util.Time.

On Thu, Mar 26, 2015 at 1:55 PM, Brian Grant [email protected]
wrote:

We have a number of fields in the API that represent an integer number of
seconds, such as TimeoutSeconds. We should push the units into the value
instead, by using time.Duration. That would be similar to the approach used
for resource quantities.

I don't consider this blocking for v1beta3, but seems like a worthwhile
change to make in the future.

cc @lavalamp https://github.com/lavalamp @smarterclayton
https://github.com/smarterclayton


Reply to this email directly or view it on GitHub
#6013.

@lavalamp
Copy link
Member

Yeah we'll have to make a wrapper type to make json encode/decode call Duration.String() and ParseDuration().

@lavalamp
Copy link
Member

I filed golang/go#10275 on the off chance that we code get this working by default.

@smarterclayton
Copy link
Contributor

And unfortunately when we do that JSON starts showing the field automatically even with omitempty because the JSON encoder has a structured list of types. So you get a lot of "foo": "", showing up. Definitely hit the limits of the JSON stdlib now.

----- Original Message -----

I filed golang/go#10275 on the off chance that we
code get this working by default.


Reply to this email directly or view it on GitHub:
#6013 (comment)

@bgrant0607 bgrant0607 added team/api and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Sep 16, 2015
@0xmichalis 0xmichalis added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed team/api (deprecated - do not use) labels Mar 20, 2017
@smarterclayton
Copy link
Contributor

We decided that we would not use duration because the encoding isn't stable. Closing this and we can reopen the discussion on the next time API field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

5 participants