-
Notifications
You must be signed in to change notification settings - Fork 294
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
Added support of nine digit nanoseconds format. #1370
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ndallmeyer The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @ndallmeyer! |
could you please merge my testcase |
… format. Added unit tests.
Thanks, based on your tests I've implemented tests that cover all edge cases. |
|
||
private static string FormatDateTimeOffsetToSevenDigitsNanoseconds(string dateTime) | ||
{ | ||
var isUTC = dateTime.EndsWith("Z"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too complex
why not just trim extra input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As already pointed out by @anekdoti in #1350 the lastTransitionTimestamp should be deserializable as a Golang Time struct that follows RFC3339 where an arbitrary number of nansecond digits from 0 to 9 is valid. Therefore trimming alone would not help here.
This method allows an arbitrary number of nanosecond digits from 0 to 9.
check my impl in #1351 |
Hello @tg123 , thank you for your PR #1351. It solves the specific case that made me open issue #1350 where we observed an error with a timestamp with 9 nanosecond digits. However, as also described in my issue, timestamps can have an arbitrary number (up to 9) of nanosecond digits by the Golang implementation. For this reason, your PR only solves certain special cases whereas the PR on hand by @ndallmeyer solves the general case as by Kubernetes API conventions. To avoid further and foreseeable problems with deserializing timestamps, I would therefore strongly suggest to follow the general approach by @ndallmeyer . |
@anekdoti could you please add bad test cases? |
Hi @tg123 , recently added test cases as comment to your PR. Also fixed a bug, that made the pipeline unhappy. |
Added support for up to nine digit nanoseconds format. The nanoseconds are truncated to 7 decimal places. See #1350