-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
remote events: convert TimeNano properly #12644
remote events: convert TimeNano properly #12644
Conversation
PR is complaining about a lack of test coverage. You can look into adding a test (maybe check that the year in the timestamp matches the current year?) or just add |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: leahneukirchen, mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Note that it only affects remote events, I'm not sure where this can be tested. |
Thanks @leahneukirchen |
Maybe add a test here: podman/test/e2e/events_test.go Line 64 in f45070e
I think this should work: diff --git a/test/e2e/events_test.go b/test/e2e/events_test.go
index 39f495460..0350bf93c 100644
--- a/test/e2e/events_test.go
+++ b/test/e2e/events_test.go
@@ -62,6 +62,8 @@ var _ = Describe("Podman events", func() {
result.WaitWithDefaultTimeout()
Expect(result).Should(Exit(0))
Expect(len(result.OutputToStringArray())).To(BeNumerically(">=", 1), "Number of events")
+ date := time.Now().Format("2000-01-01")
+ Expect(result.OutputToStringArray()).To(ConsistOf(HavePrefix(date)))
})
It("podman events with an event filter and container=cid", func() {
``´ |
I think this test suite doesn't test for |
It will, we have several suites for the remote client (Any of the tests marked |
Ok, I can add this then. Give me a few days, tho. |
ac65492
to
9953d74
Compare
@leahneukirchen friendly ping |
9953d74
to
102d6b9
Compare
There only seems to be a way to test one log line to have the right prefix, but I guess that works too. |
LGTM |
These failures are unrelated I think? |
Restarted, if they're still red in a few hours I'll look deeper and see if they're related |
@leahneukirchen, could you do another rebase? CI should be green now. Apologies! |
e.TimeNano contains nanoseconds since epoch, not just the nanoseconds after e.Time. time.Unix supports nanoseconds > 999999999 and converts them to seconds, so just passing e.TimeNano is enough. Signed-off-by: Leah Neukirchen <[email protected]>
102d6b9
to
5aedcb3
Compare
/lgtm |
e.TimeNano contains nanoseconds since epoch, not just the nanoseconds
after e.Time.
time.Unix supports nanoseconds > 999999999 and converts them to seconds,
so just passing e.TimeNano is enough.
Closes #12629.