-
Notifications
You must be signed in to change notification settings - Fork 30
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 a bug in Pipeline Management table where sometimes pipeline action buttons do not send requests #1008
Conversation
✅ Meticulous spotted zero visual differences across 6 screens tested: view results. Last updated for commit 67c4324. This comment will update as new commits are pushed. |
Steps to reproduce the issue:
|
I can't seem to get out of failed state |
this bug still seems to be there for me in 5d0319b Screen.Recording.2023-11-10.at.11.58.14.AM.mov |
Can reproduce, digging again |
…o not send requests Signed-off-by: George <bulakh.96@gmail.com>
5d0319b
to
67c4324
Compare
I'll fix it in a separate PR, it's not related to this fix |
* @returns | ||
*/ | ||
const consolidatePipelineStatus = (newPipeline: Pipeline, oldPipeline: Pipeline | undefined) => { | ||
const current_status = |
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.
can we make this more readable? e.g. something like
const current_status = | |
const isNotDesired = newPipeline.state.current_status !== toClientPipelineStatus(newPipeline.state.desired_status) | |
const isNotFailed = newPipeline.state.current_status !== PipelineStatus.FAILED | |
const oldHasStatus = oldPipeline?.state.current_status | |
const oldWasTransitioning = oldHasStatus && [PipelineStatus.STARTING, PipelineStatus.PAUSING, PipelineStatus.SHUTTING_DOWN].includes(oldPipeline.state.current_status) | |
const current_status = | |
isNotDesired && | |
isNotFailed && | |
oldWasTransitioning | |
? oldPipeline.state.current_status | |
: newPipeline.state.current_status | |
return updatePipelineStatus('current_status', current_status)(newPipeline) |
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.
Let me see
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.
Nice idea, added
The issue was that when checking if start/pause/shutdown request is redundant, a stale request cache was used to determine current pipeline status. Since the previous mutation the cache for
.pipelineStatus()
query was invalidated, but by defaultgetQueryData()
returns both valid and stale cache. By adding { stale: false } filter, we ensure that only valid status data is used for the check. Also we apply status consolidation viaconsolidatePipelineStatus()
to.pipelineStatus()
query, just like to.pipelines()
query, to ensure identical behavior.