Skip to content
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

Use TOML-compatible duration type. #1350

Merged
merged 3 commits into from Apr 3, 2017
Merged

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented Mar 27, 2017

This PR introduces a new time.Duration-based type capable of deserializing durations in TOML specs. Without providing a unit specifier, it defaults to seconds for backwards compatibility reasons and the fact that most of our duration specifiers are at the order of seconds.

TODO: Switch glide.yaml back to github.com/containous/flaeg once containous/flaeg#34 merges. Done.

Fixes #1342.

@timoreimann timoreimann force-pushed the toml-compatible-duration-type branch from a9807a6 to 9d700f7 Compare March 28, 2017 07:53
@timoreimann timoreimann changed the title [WIP] TOML-compatible duration type. [DO NOT MERGE] TOML-compatible duration type. Mar 28, 2017
@timoreimann
Copy link
Contributor Author

@containous/traefik Please review.

Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM but this would mean ppl that have this option specified as an int 10 would need to update their configuration, right ?

@timoreimann
Copy link
Contributor Author

@vdemeester:

SGTM but this would mean ppl that have this option specified as an int 10 would need to update their configuration, right ?

No, precisely not: The custom duration type assumes second scale if you don't specify a unit.

See also the description in the PR, the docs, and the source of truth. :)

@vdemeester
Copy link
Contributor

@timoreimann thanks 👼 sorry I'm not fully awake this morning it seems 😂

@timoreimann
Copy link
Contributor Author

@vdemeester haha no worries. Nothing that an extra shot of caffeine can't fix. :-)

@timoreimann timoreimann changed the title [DO NOT MERGE] TOML-compatible duration type. [DO NOT MERGE] Use TOML-compatible duration type. Mar 28, 2017
@dtomcej
Copy link
Contributor

dtomcej commented Mar 28, 2017

LGTM, But I might make it clearer in the docs that not providing a unit will result in the value being parsed as seconds. Other proxies use raw values as milliseconds, so that would be a distinction worth making.

@timoreimann
Copy link
Contributor Author

timoreimann commented Mar 28, 2017

@dtomcej I tried to document that the value defaults to seconds without a unit. Do you think it should be made clearer? If so, could you provide a better phrasing please?

docs/toml.md Outdated
@@ -56,11 +57,13 @@
# Backends throttle duration: minimum duration in seconds between 2 events from providers
# before applying a new configuration. It avoids unnecessary reloads if multiples events
# are sent in a short amount of time.
# Can be provided in a format supported by [time.ParseDuration](https://golang.org/pkg/time/#ParseDuration) or as digits
# only. In the latter case, seconds are assumed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or as digits only -> or as raw values (digits)
In the latter case, seconds are assumed. -> If no units are provided, the value is parsed assuming seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dtomcej Updated the docs.

@timoreimann timoreimann changed the title [DO NOT MERGE] Use TOML-compatible duration type. Use TOML-compatible duration type. Mar 28, 2017
@timoreimann timoreimann added this to the 1.3 milestone Mar 28, 2017
@timoreimann timoreimann added the kind/enhancement a new or improved feature. label Mar 28, 2017
@timoreimann timoreimann force-pushed the toml-compatible-duration-type branch 2 times, most recently from 0b3dfdd to 43ad187 Compare March 30, 2017 21:54
Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks @timoreimann

@timoreimann timoreimann force-pushed the toml-compatible-duration-type branch from 43ad187 to 056fe9a Compare April 3, 2017 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants