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
Fix pipelinerun visualization inprogress crash #7438
Fix pipelinerun visualization inprogress crash #7438
Conversation
/test analyze |
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.
Crash is solved. 👍
The update is visible to the user, so that visualization comes after the data is available.
/lgtm
/assign @andrewballantyne
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 we actually want the solution to be in frontend/packages/pipelines-plugin/src/components/pipelineruns/detail-page-tabs/PipelineRunVisualization.tsx
... this is a problem with our pulling the Pipeline logic from the Pipeline Run... which is a matter of time.
Making the change in frontend/packages/pipelines-plugin/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualization.tsx
sets us up for an awkward situation of allowing the creation of a PipelineVisualization without a Pipeline -- and that's not ideal.
The solution should also be a Loading indicator not a null as when I reproduce this scenario and get a null
back from the Pipeline it removes the graph and then pops back in without warning. Probably a better use experience to spin.
Definitely solved the crash... so this is an okay solution to the problem, however...
This is probably not the best user experiernce. /lgtm cancel |
cff97ff
to
1574bde
Compare
1574bde
to
919a454
Compare
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.
cc @bgliwa01
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!
/lgtm Slight improvement on the solution @jerolimov lgtm'ed -- we talked about this solution and he was in agreement. Let's get this merged 😄 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, bgliwa01, jerolimov, karthikjeeyar 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 |
Fixes:
https://issues.redhat.com/browse/ODC-5160
Analysis / Root cause:
When starting a Pipeline multiple times the console crashes sometimes with this JavaScript error
Solution Description:
If pipeline run is in progress and doesn't have enough information(pipelineSpec) to form the pipeline, then do not render the visualization graph.
Screen shots / Gifs for design review:
Unit test coverage report:
Test setup:
start last run
option.Browser conformance:
/kind bug
/assign @jerolimov