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
Bug 1875381: Manual cherrypick fix log whitescreen (release-4.4) #6513
Bug 1875381: Manual cherrypick fix log whitescreen (release-4.4) #6513
Conversation
@abhinandan13jan: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@abhinandan13jan: No Bugzilla bug is referenced in the title of this pull request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@abhinandan13jan: This pull request references Bugzilla bug 1875381, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@abhinandan13jan: This pull request references Bugzilla bug 1875381, which is valid. The bug has been moved to the POST state. 6 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
/lgtm
This will be needed for 4.4 to avoid a breaking (whitescreen) functionality in the Console.
/bugzilla refresh It's actually high severity, updating... |
@andrewballantyne: This pull request references Bugzilla bug 1875381, which is valid. 6 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm cancel @abhinandan13jan You missed an item: https://github.com/openshift/console/pull/6513/files?file-filters%5B%5D=#diff-ed4cae45a21455c6bb1be585d07cc4e4R60 |
6ef0a4c
to
67c9ce2
Compare
const taskRuns = Object.keys(taskRunFromYaml).sort((a, b) => { | ||
if (_.get(taskRunFromYaml, [a, 'status', 'completionTime'], false)) { | ||
return taskRunFromYaml[b].status.completionTime && | ||
return taskRunFromYaml[b].status?.completionTime && | ||
new Date(taskRunFromYaml[a].status.completionTime) > | ||
new Date(taskRunFromYaml[b].status.completionTime) | ||
? 1 | ||
: -1; | ||
} | ||
return taskRunFromYaml[b].status.completionTime || | ||
new Date(taskRunFromYaml[a].status.startTime) > | ||
new Date(taskRunFromYaml[a].status?.startTime) > | ||
new Date(taskRunFromYaml[b].status.startTime) | ||
? 1 |
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.
You didn't quite implement this the same way, but I see .startTime
is being optional in 4.5 & 4.6
Can we not just mirror what we have there so there are no surprises in this backport. Optionality should be pretty safe here.
From #6053
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.
updated
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.
So the change here is now bigger then the origin PR? I think this one is good, but should we change the master also and replace status.startTime
with status?.startTime
?? 🤔
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.
@jerolimov The master works fine and tested well along the scenarios. This is just an overprotection so that things do not break.
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.
changed it to be same as master
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.
I think this is fine... I'll let @jerolimov comment as he had a pending question that he has not responded to the answer.
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.
Its now similar to master so this cherrypick is for me. 👍
67c9ce2
to
2b9300b
Compare
/test frontend |
948eba3
to
783b8d9
Compare
/retest |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinandan13jan, andrewballantyne, jerolimov 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 |
@abhinandan13jan: All pull requests linked via external trackers have merged: Bugzilla bug 1875381 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Adresses
https://issues.redhat.com/browse/ODC-4320
Issue
Pipeline Conditions WhiteScreen the Log View
Solution
Added null check
ScreenShot
Tests
Tests unaltered
Browser
Chrome, Firefox