-
Notifications
You must be signed in to change notification settings - Fork 167
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
Improve Pipeline logs error states #2300
Improve Pipeline logs error states #2300
Conversation
WIP to add tests and screenshots for cleaned-up pods. |
19da91e
to
ae4ba27
Compare
@yih-wang can you check the error states? Do we want to show the toolbar in any of the states above? |
ae4ba27
to
ee092af
Compare
@manaswinidas No, I don't think we will include any error state in the toolbar. The only two ways we display the error messages are:
|
Oops sorry that I misread the question... |
@yih-wang it's highly unlikely that some step logs may be fetched while some cannot, in case of network issues. |
@manaswinidas Then I think we should still show the toolbar in the 'No internet' error case to provide the ability to switch to other steps. |
ee092af
to
6eb59ca
Compare
@yih-wang But we can't retrieve logs when there is no internet connection, even if we are able to switch the steps using the dropdown or the download dropdown, it's doing nothing because there is no internet. Here's a screen recording to demonstrate the same. Do we still show the toolbar in this case? Screen.Recording.2024-01-08.at.11.43.27.PM.mov |
df0ae8a
to
9e76f19
Compare
@manaswinidas Oops, read your previous message again and realized you were saying it's unlikely that some steps have logs while others do not... Then yes you are right, we don't show the toolbar in the network issue case too. |
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.
on error, you are still polling, i assume this is intended?
frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTabStatus.tsx
Outdated
Show resolved
Hide resolved
frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTabStatus.tsx
Outdated
Show resolved
Hide resolved
febc1d8
to
44e0dfa
Compare
@yih-wang Can you have a final look at the screenshots? |
@manaswinidas Didn't we combine case 1 (failed/cleaned-up pods) and case 3 (failed pod) you show in the screenshots? |
@yih-wang I did according to this discussion we had last month |
The error messages look good to me. |
@yih-wang Thanks for pointing it out. I updated it just now. |
code looks good to me. /lgtm just need @yih-wang approval and an advisor |
44e0dfa
to
a91976f
Compare
New changes are detected. LGTM label has been removed. |
Rebased, cleaned up a few nits after the last merge. |
/lgtm all still works |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: mturley 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 |
991cd37
into
opendatahub-io:f/pipelines-enhancement
JIRA:
RHOAIENG-254
RHOAIENG-255
500 Internal Error state will be handled by RHOAIENG-1067
Description
Cleaned-up pods(Error message according to this Slack message):
No internet(Error message according to this Slack message):
Failed pod(Error message according to this Slack message):
How Has This Been Tested?
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main