-
Notifications
You must be signed in to change notification settings - Fork 178
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
Add error state when pipeline version is deleted #2915
Conversation
/retest |
e38bc49
to
5c4db9f
Compare
cc @yannnz |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2915 +/- ##
==========================================
+ Coverage 78.52% 78.56% +0.04%
==========================================
Files 1139 1139
Lines 24171 24176 +5
Branches 6099 6101 +2
==========================================
+ Hits 18980 18994 +14
+ Misses 5191 5182 -9
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
ApplicationsPage is a global component, so rather than adding Pipeline specific props to it, you should handle the versionLoaded condition inside the PipelineRunDetails.
See the suggestions below to implement this.
frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDetails.tsx
Outdated
Show resolved
Hide resolved
frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDetails.tsx
Show resolved
Hide resolved
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, just a small change
frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDetails.tsx
Outdated
Show resolved
Hide resolved
After this if we redirect to the runs of a deleted pipeline version, we see the loading state of the pipeline in the breadcrumb. How should we handle this? @Gkrumbach07 @yannnz |
@dpanshug I think this is a bug. We don't need to show pipeline name in the breadcrumb, since the path to this page is from Selected Experiment to this selected Run. I remeber @DaoDaoNoCode is working on fix it. |
@Gkrumbach07 to display Pipeline spec tab, we need to have version. https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDetails.tsx#L173 |
@ppadti I would suggest to follow the same design pattern for Run graph tab since we are not able to get the data of the spec for a deleted pipeline: displaying error message as the statement. |
Hi @kaedward , could you help to review the error state for the Pipeline spec tab? In the mock, I just updated the "Run pipeline graph" to "Run pipeline spec". |
@yannnz, I think I made a suggestion for the run graph message, but reading it back now, I don't think it's super clear because "error loading BLANK" implies that the info can be loaded if an action is taken. If the item has been deleted, the info will never be available to load. So what do you think of these alternate messages? "Pipeline run graph unavailable "Pipeline spec unavailable |
Thanks @kaedward , sounds good to me.
|
|
@yannnz I have updated the PR and screenshots. Please take a look. |
Looks good to me. |
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.
Works fine, i can see the error message when the version is deleted.
/lgtm
this looks good /approve |
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: dpanshug, Gkrumbach07 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 |
Closes: RHOAIENG-6683
Description
This PR aims to add error state in pipeline run details page when pipeline version is deleted.
How Has This Been Tested?
Test Impact
Added cypress test.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main