fix(KFLUXUI-253): tekton results should filter deleted record out #55
Conversation
testcara
force-pushed
the
KFLUXUI-253
branch
2 times, most recently
from
6cf087c to
cfa1ecd
Compare
|
/retest |
testcara
changed the title
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (33.33%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
==========================================
- Coverage 80.10% 80.06% -0.05%
==========================================
Files 577 544 -33
Lines 21495 21159 -336
Branches 5064 5317 +253
==========================================
- Hits 17219 16941 -278
+ Misses 4252 4193 -59
- Partials 24 25 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 34 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
testcara
force-pushed
the
KFLUXUI-253
branch
3 times, most recently
from
0b59a41 to
7c5062b
Compare
testcara
requested review from
caugello,
CryptoRodeo,
sahil143 and
JoaoPedroPP
src/utils/tekton-results.ts
Outdated
| // When pipelines are running, the etcd would keep their results. | ||
| // deleting pipelines frome ectd would make unknown tekton results. | ||
| // Just to get meaningful test runs, we need to filter 'unknown' out. | ||
| const filteredPipelineRuns = originalPipelineRuns.filter((pipelinerun) => { | ||
| return pipelinerun.status?.conditions?.every((c) => c.status !== 'Unknown') ?? true; | ||
| }); | ||
| return [filteredPipelineRuns, list]; | ||
| }; |
There was a problem hiding this comment.
Is it possible that an unknown status will appear for the pipeline runs that were not deleted? In that case, It's not a good idea to filter out the Unknown status pipeline runs.
There was a problem hiding this comment.
Can you specify why it is not good idea?
There was a problem hiding this comment.
I just improved the filter condition to ensure it is more accurate and added more explanations. Could you help to review again? Thank you!
testcara
changed the title
src/utils/tekton-results.ts
Outdated
| const filteredPipelineRuns = originalPipelineRuns.filter((pipelinerun) => { | ||
| return ( | ||
| pipelinerun.status?.conditions?.every( | ||
| (c) => !(c.status === 'Unknown' && c.type === 'Succeeded' && c.reason === 'Running'), | ||
| ) ?? true | ||
| ); | ||
| }); | ||
| return [filteredPipelineRuns, list]; | ||
| }; |
There was a problem hiding this comment.
It's not a good idea to just filter out the Unknown status because When a pipeline run is running, its Condition Succeeded status is initially unknown. If the pipeline is deleted while in this state, it is archived to Tekton results with the same 'unknown' status. So we will end up filtering out the Running pipelines also.
There was a problem hiding this comment.
For the regular running pipeline, it would be shown running well. see the screenshot:
We will not filter out regular running pipelines. You could try to rerun one test run like https://localhost:8080/workspaces/wlin-tenant/applications/test-unstopped-pipeline/pipelineruns/test-28bg4 to check the whole process.
The filter will not affect any regular process.
Just when we delete the running pipeline, the pipeline would be gone and this is the expected behavior.
BTW, do you have better idea, if you would give more suggestion, let me do more improvement with better solution.
Is it possible we ask other team to add some item in the general status of the pipeline?
There was a problem hiding this comment.
Make sense, only Tekton results are being filtered here. can you add a few test cases for this with running status condition and other status condition.
| const filteredPipelineRuns = originalPipelineRuns.filter((pipelinerun) => { | ||
| return ( | ||
| pipelinerun.status?.conditions?.every( | ||
| (c) => !(c.status === 'Unknown' && c.type === 'Succeeded' && c.reason === 'Running'), |
There was a problem hiding this comment.
do not we have enum for this?
src/utils/tekton-results.ts
Outdated
| const filteredPipelineRuns = originalPipelineRuns.filter((pipelinerun) => { | ||
| return ( | ||
| pipelinerun.status?.conditions?.every( | ||
| (c) => !(c.status === 'Unknown' && c.type === 'Succeeded' && c.reason === 'Running'), | ||
| ) ?? true | ||
| ); | ||
| }); | ||
| return [filteredPipelineRuns, list]; | ||
| }; |
There was a problem hiding this comment.
Make sense, only Tekton results are being filtered here. can you add a few test cases for this with running status condition and other status condition.
testcara
dismissed stale reviews from CryptoRodeo and StanislavJochman
via
0a173dc
testcara
force-pushed
the
KFLUXUI-253
branch
from
f901aa6 to
0a173dc
Compare
|
@sahil143 , more status have been added to the test. Thank you for your review. :-) |
Fixes
https://issues.redhat.com/browse/KFLUXUI-253
Description
When we delete one pipeline task run manually before it is completed,UI should not always keep the obsoleted run there. This patch would ignore the meaningless unknown tekton results to ensure there is no deleted but 'in process' runs on UI.
Type of change
Screen shots / Gifs for design review
Without the patch, the UI would list deleted 'running' pipelineruns.
With the patch, the UI would ignore the deleted 'running' pipelineruns.
How to test or reproduce?
Browser conformance: