Convert seconds to nanoseconds for juju.unit.run #237
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
June 5, 2018 10:12
The juju.unit.run docstring does not specify what units the timeout value should be in. This led to issue juju#225. Having looked into it, juju is expecting it to be in nanoseconds. I don't think nanoseconds is particularly intuitive so I have updated the function to take the timeout in seconds and convert it internally to nanoseconds. I appreciate this is a breaking change for anyone currently specifying the timeout in nanoseconds so I would quite understand if this change is nack'd in which case I will submit a subsequent pr which simply updates the doc string. Closes juju#225.
tvansteenburgh
approved these changes
Jun 5, 2018
There was a problem hiding this comment.
It does break back-compat but I think it's worth it in this case. #225 shows that users will intuitively expect this value to be in seconds. Plus, libjuju is still pre-1.0, so technically there are no api guarantees yet anyway.
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The juju.unit.run docstring does not specify what units the timeout
value should be in. This led to issue #225. Having looked into it,
juju is expecting it to be in nanoseconds. I don't think
nanoseconds is particularly intuitive so I have updated the function
to take the timeout in seconds and convert it internally to
nanoseconds.
I appreciate this is a breaking change for anyone currently specifying
the timeout in nanoseconds so I would quite understand if this change
is nack'd in which case I will submit a subsequent pr which simply
updates the doc string. Closes #225.