Changing cache node color to green #2897
Conversation
dpanshug
force-pushed
the
cache-node
branch
2 times, most recently
from
aeeb8ab to
de1425f
Compare
There was a problem hiding this comment.
Updates to the styling look great @dpanshug - I have a question regarding the JIRA ticket - according @yannnz 's comments in https://issues.redhat.com/browse/RHOAIENG-7144 , would we want to show "Skipped" as the status in the drawer? Currently we are showing "Succeeded" for those skipped tasks.
The suggestion is to show the graph in green color while keep the cached icons.
Reason for this approach:
With the green color in the graph, the user will understand it is succeed.
The cached icons for the first time user will make them thinking it is not a common completed status. Then in the drawer, the user will find out the icon status represent cached.
Otherwise LGTM.
Yes you are correct Jenny. @dpanshug Can you make sure this is reflected |
@Gkrumbach07 @jenny-s51 That's what i was bothered about from start. We are not using custom nodes in the pipeline topology, the badge color and status both are defined in the same function Should I move with the custom node? but that will need to refactor a lot of things, or is there any other solution we can think of. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2897 +/- ##
==========================================
+ Coverage 78.47% 78.49% +0.01%
==========================================
Files 1127 1127
Lines 23925 23931 +6
Branches 6029 6033 +4
==========================================
+ Hits 18775 18784 +9
+ Misses 5150 5147 -3
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
@dpanshug Is there just a way to save that it is cached in the node so that later on we can read from that? |
|
@dpanshug This solution should work diff --git a/frontend/src/concepts/pipelines/topology/parseUtils.ts b/frontend/src/concepts/pipelines/topology/parseUtils.ts
index a45733fb..775bd58b 100644
--- a/frontend/src/concepts/pipelines/topology/parseUtils.ts
+++ b/frontend/src/concepts/pipelines/topology/parseUtils.ts
@@ -315,8 +315,8 @@ export const translateStatusForNode = (
return RunStatus.InProgress;
case ExecutionStateKF.COMPLETE:
case RuntimeStateKF.SUCCEEDED:
- case ExecutionStateKF.CACHED:
return RunStatus.Succeeded;
+ case ExecutionStateKF.CACHED:
case RuntimeStateKF.SKIPPED:
return RunStatus.Skipped;
case RuntimeStateKF.RUNTIME_STATE_UNSPECIFIED:
diff --git a/frontend/src/concepts/topology/customNodes/StandardTaskNode.tsx b/frontend/src/concepts/topology/customNodes/StandardTaskNode.tsx
index 4f31022c..52a715c3 100644
--- a/frontend/src/concepts/topology/customNodes/StandardTaskNode.tsx
+++ b/frontend/src/concepts/topology/customNodes/StandardTaskNode.tsx
@@ -39,6 +39,13 @@ const StandardTaskNode: React.FunctionComponent<StandardTaskNodeProps> = ({
/>
) : null;
+ const getNodeStatus = () => {
+ if (data?.pipelineTask.status?.state === ExecutionStateKF.CACHED) {
+ return RunStatus.Succeeded;
+ }
+ return data?.runStatus;
+ };
+
return (
<g ref={hoverRef as React.LegacyRef<SVGGElement>}>
<TaskNode
@@ -46,7 +53,7 @@ const StandardTaskNode: React.FunctionComponent<StandardTaskNodeProps> = ({
onSelect={onSelect}
selected={selected}
scaleNode={hover && detailsLevel !== ScaleDetailsLevel.high}
- status={data?.runStatus}
+ status={getNodeStatus()}
customStatusIcon={
data?.pipelineTask.status?.state === ExecutionStateKF.CACHED ? (
<AngleDoubleRightIcon />
|
dpanshug
force-pushed
the
cache-node
branch
2 times, most recently
from
1e6cdfd to
68f4f14
Compare
|
/lgtm this solution looks to work. and tested live |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
RHOAIENG-7144
Description
Dashboard shows cached pipeline steps in gray (no info about what it means). Changed the cache (successfull) node color to green, keeping the cache icon.
How Has This Been Tested?
Duplicate a pipeline run to see the cache node.
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@yannnz