Handle ps container created field as a time.Time #8427
Conversation
In the current code we were translating the created time from a time.Time to a unix epoch, this was leading to a loss of precession, and some unexpected results where the sorting order of containers was misordered because of the precession loss. If we pass around created as time.Time, we do not loose the precission. Fixes: containers#8414 Signed-off-by: Daniel J Walsh <[email protected]>
openshift-ci-robot
added
the
approved
|
@edsantiago PTAL |
| @@ -301,5 +301,5 @@ func (a SortPSContainers) Swap(i, j int) { a[i], a[j] = a[j], a[i] } | |||
| type SortPSCreateTime struct{ SortPSContainers } | |||
|
|
|||
| func (a SortPSCreateTime) Less(i, j int) bool { | |||
| return a.SortPSContainers[i].Created > a.SortPSContainers[j].Created | |||
| return a.SortPSContainers[i].Created.Before(a.SortPSContainers[j].Created) | |||
There was a problem hiding this comment.
I'm a bit confused. Was the ">" correct for the original version of this "Less()" function? If so, does this need to be After rather than Before, or are you correcting a bad sort?
Back to my tea cup.
There was a problem hiding this comment.
There was a problem hiding this comment.
@jwhonce "I'm a mistake in original code", I wasn't aware you've gone full Tron in this project. That's a bit scary! 😄
Thx for the 411
| @@ -14,7 +15,7 @@ type ListContainer struct { | |||
| // Container command | |||
| Command []string | |||
| // Container creation time | |||
| Created int64 | |||
| Created time.Time | |||
There was a problem hiding this comment.
I don't believe this is going over the wire. The time.Time came over the wire though.
There was a problem hiding this comment.
But this may be a breaking change for anyone who has vendored in /pkg/ directories. So we might have to wait to fix this until 3.0.
| @@ -301,5 +301,5 @@ func (a SortPSContainers) Swap(i, j int) { a[i], a[j] = a[j], a[i] } | |||
| type SortPSCreateTime struct{ SortPSContainers } | |||
|
|
|||
| func (a SortPSCreateTime) Less(i, j int) bool { | |||
| return a.SortPSContainers[i].Created > a.SortPSContainers[j].Created | |||
| return a.SortPSContainers[i].Created.Before(a.SortPSContainers[j].Created) | |||
There was a problem hiding this comment.
|
LGTM |
|
@vrothberg Do you agree this is a breaking change, wait for 3.0> |
|
Well, the good news is, in one hour of loop testing I got no failures. LGTM. BTW I have a test for it in the works, will submit PR some time later, possibly with a |
|
We are holding up the merge of this PR until after 2.2 release. |
|
@containers/podman-maintainers PTAL This is ready to merge. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jwhonce, rhatdan, saschagrunert 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 |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
In the current code we were translating the created time
from a time.Time to a unix epoch, this was leading to a loss
of precession, and some unexpected results where the sorting
order of containers was misordered because of the precession loss.
Fixes: #8414
If we pass around created as time.Time, we do not loose the precission.
Signed-off-by: Daniel J Walsh [email protected]