fix: proper sorting resources by age column (#2182 followup) #2414
Conversation
MJ111
force-pushed
the
fix/cluster-issues-sort-by-age
branch
from
789773b
to
fd32f2e
Compare
|
Don't know why Could you guys help me? |
|
@MJ111 I just noticed that on some other PRs. So I think CI is borked currently. |
|
@MJ111 can you please rebase against master? |
| @@ -37,21 +38,22 @@ export class ClusterIssues extends React.Component<Props> { | |||
| private sortCallbacks = { | |||
| [sortBy.type]: (warning: IWarning) => warning.kind, | |||
| [sortBy.object]: (warning: IWarning) => warning.getName(), | |||
| [sortBy.age]: (warning: IWarning) => warning.age || "", | |||
| [sortBy.age]: (warning: IWarning) => warning.getTimeDiffFromNow(), | |||
There was a problem hiding this comment.
Doesn't feel quite right, getTimeDiffFromNow() calls Date.now(), which means that for sorting a different "now" is used each time this is called. Could cause problems? Just use a static timeDiffFromNow in IWarning? Or we really just need something that converts the age string to a number (and done when warnings is populated)?
There was a problem hiding this comment.
So age is getAge() which calls getTimeDiffFromNow() anyway.
There was a problem hiding this comment.
Right, but it was not getting called during sorting. And also that means we shouldn't need to use getTimeDiffFromNow() directly.
There was a problem hiding this comment.
Though we also don't want it use a different "now" when creating it. So probably the best solution would be to have a warning.getTimeDiffFrom(firstLoad) where this.firstLoad = Date.now() is called in the componentDidMount method.
There was a problem hiding this comment.
Probably beyond the scope of this PR.
If this PR is in line with what we do elsewhere then it's OK. Though I think it could be changed easily enough so that it is using a pre-calculated timeDiffFromNow value in the sort callback.
There was a problem hiding this comment.
Agreed as out of scope, though it shouldn't change the sorting order (too much).
MJ111
force-pushed
the
fix/cluster-issues-sort-by-age
branch
from
fd32f2e
to
545dd86
Compare
|
@Nokel81 sorry for the late response. I rebased the branch. |
Nokel81
added
the
merge/master
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
sorting resources by age in cluster-issues is broken (previously sorted by iso-string timestamp) Signed-off-by: MinJeong Kim <min7859@gmail.com>
Nokel81
force-pushed
the
fix/cluster-issues-sort-by-age
branch
from
545dd86
to
79659e5
Compare
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com>
fix lint Signed-off-by: Jim Ehrismann <jehrismann@mirantis.com>
Co-authored-by: Jim Ehrismann <40840436+jim-docker@users.noreply.github.com>
Nokel81
removed
the
merge/master
Co-authored-by: Jim Ehrismann <40840436+jim-docker@users.noreply.github.com>
Hello guys, thank you for the awesome project! I'm actively using lens in production.
I recently encountered this bug and searched #2182 PR was merged but omitted this part.
So, As followup, I added this PR.
If you have question, feel free to comment. Thanks.
Fixes:
sorting resources by age is broken (previously sorted by iso-string timestamp) in cluster-issues
AS-IS
TO-BE
