commit visualization in commit details page #254

karthikjeeyar · 2022-11-09T14:56:02Z

Fixes

https://issues.redhat.com/browse/HACBS-826

Description

Adds commit visualization in commit details page.

Type of change

Feature

Screen shots / Gifs for design review

How to test or reproduce?

Yamls to create test setup:

Test and Environments are here - https://gist.github.com/karthikjeeyar/ce5a1038209565e5ecf0f3a9719609be
Release Setup yamls - https://github.com/karthikjeeyar/hacbs-demos/tree/main/m7/release

karthikjeeyar · 2022-11-09T15:04:04Z

cc: @christianvogt @jeff-phillips-18

karthikjeeyar · 2022-11-14T12:20:53Z

cc: @Ranelim @MariaLeonova

MariaLeonova · 2022-11-14T13:09:18Z

@Ranelim @andrew-ronaldson let's update the icons as part of this PR?
@karthikjeeyar what format would you want them in? I think we're good with the github icons, while the other two (pipeline, environment) need to be updated.

karthikjeeyar · 2022-11-15T15:21:30Z

@MariaLeonova If it is not a patternfly icons, you can provide me in SVG format.

MariaLeonova · 2022-11-15T15:53:30Z

@karthikjeeyar this one is for pipelines (attached)

this one is for Environments

Octokitty can stay the same.

Thank you!

jeff-phillips-18 · 2022-11-15T18:16:17Z

@karthikjeeyar I have added the pipeline SVG as part of #248
I use the PF ServerIcon for environments.

christianvogt · 2022-11-15T20:23:31Z

When testing this change the sizing of other visualizations seems broken. At the same time I cannot reproduce this on console.dev:

karthikjeeyar · 2022-11-16T19:12:35Z

Rebased, Updated Icons and fixed the size issues in other visualisations.

christianvogt · 2022-11-16T21:16:02Z

src/hacbs/components/topology/factories/VisualizationFactory.tsx

+      if (fitToScreen) {
+        controller.getGraph().fit(90);
+      }


I don't see fitToScreen being used by any caller of this function.

Furthermore if fitToScreen changes, this effect does a whole lot more than what's needed to fit to screen.

If you still need this prop, move this to a separate effect.

I removed it as it is not used in other graphs

christianvogt · 2022-11-16T21:16:51Z

src/hacbs/components/Commits/tabs/CommitDetails/CommitComponentFactory.tsx

+export const commitComponentFactory: ComponentFactory = (kind: ModelKind, type: string) => {
+  switch (kind) {
+    case ModelKind.graph:
+      return withPanZoom()(GraphComponent);


pan zoom is fine for now until UX gives a final decision if this needs to be removed in lieu of scroll bars.

Inconsistent with the application overview graph. Whichever way is finally decided, these should be made consistent.

christianvogt · 2022-11-16T21:20:26Z

...hacbs/components/ApplicationDetails/tabs/overview/visualization/utils/visualization-utils.ts

+        : (resources as []).filter((r) => {
+            return !get(r, runAfterResourceKey);
+          });


r is of type never here.
Avoid casting [] by doing proper checks.

Typescript throws an error when we use Union of array types for methods like filter(), find(), every() and there is an open issue on Typescript to provide a fix for this. :(

Ah that's too bad :(
Ok work around is fine for now. Thanks!

christianvogt · 2022-11-16T21:36:39Z

...cbs/components/ApplicationDetails/tabs/overview/visualization/hooks/useCommitWorkflowData.ts

+    (bp) => bp.metadata.labels[PipelineRunLabel.COMMIT_COMPONENT_LABEL],
+  );
+
+  const commitWorkflowData: { [key: string]: any } = components.reduce(


There's opportunity to memoize some values here such that we don't need to recompute everything if only a single value changes.

That being said, it's something that can be improved on in future PRs and not needed right away.

codecov · 2022-11-17T16:47:17Z

Codecov Report

Merging #254 (b551da6) into main (4fc123e) will increase coverage by 0.47%.
The diff coverage is 87.20%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #254      +/-   ##
==========================================
+ Coverage   73.27%   73.74%   +0.47%     
==========================================
  Files         421      426       +5     
  Lines        8651     8933     +282     
  Branches     2253     2366     +113     
==========================================
+ Hits         6339     6588     +249     
- Misses       2180     2211      +31     
- Partials      132      134       +2

Impacted Files	Coverage Δ
...mponents/Commits/__data__/pipeline-with-commits.ts	`100.00% <ø> (ø)`
...rc/hacbs/components/topology/utils/create-utils.ts	`100.00% <ø> (ø)`
...mits/tabs/CommitDetails/CommitComponentFactory.tsx	`21.05% <21.05%> (ø)`
...s/overview/visualization/utils/node-icon-utils.tsx	`95.23% <50.00%> (-4.77%)`	⬇️
...Commits/tabs/CommitDetails/CommitVisualization.tsx	`60.00% <60.00%> (ø)`
...verview/visualization/nodes/CommitWorkflowNode.tsx	`69.23% <69.23%> (ø)`
...rview/visualization/hooks/useCommitWorkflowData.ts	`93.39% <93.39%> (ø)`
...verview/visualization/utils/visualization-utils.ts	`99.24% <98.93%> (+3.79%)`	⬆️
...view/visualization/hooks/__data__/workflow-data.ts	`100.00% <100.00%> (ø)`
...cationDetails/tabs/overview/visualization/types.ts	`100.00% <100.00%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fc123e...b551da6. Read the comment docs.

jeff-phillips-18 · 2022-11-18T12:57:44Z

src/hacbs/components/Commits/tabs/CommitDetails/CommitVisualization.tsx

+        layoutFactory={layoutFactory}
+        model={model}
+        fullHeight
+        controlBar={(controller) => (


Definitely a bit odd that the commits overview graph has the control bar whereas the application overview graph does not.

Yeah, waiting on UX for final update on this and pan zoom feature as well.

jeff-phillips-18 · 2022-11-18T13:04:14Z

src/hacbs/components/Commits/tabs/CommitDetails/CommitVisualization.tsx

+import { layoutFactory, PipelineLayout, VisualizationFactory } from '../../../topology/factories';
+//
+import './CommitVisualization.scss';
+import { commitComponentFactory } from './CommitComponentFactory';


Wonder why the linter didn't pickup the import order here

karthikjeeyar · 2022-11-18T14:33:38Z

/retest

karthikjeeyar · 2022-11-21T07:32:35Z

/retest

openshift-ci · 2022-11-21T08:01:33Z

@karthikjeeyar: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

christianvogt · 2022-11-21T14:09:30Z

/approve

openshift-ci · 2022-11-22T12:05:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18, 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:

~~OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2022

openshift-ci bot requested review from divyanshiGupta and jrichter1 November 9, 2022 14:56

karthikjeeyar changed the title ~~WIP: commit visualization in details page~~ WIP: commit visualization in commit details page Nov 9, 2022

christianvogt added the hacbs label Nov 9, 2022

karthikjeeyar changed the title ~~WIP: commit visualization in commit details page~~ commit visualization in commit details page Nov 14, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2022

karthikjeeyar force-pushed the commit-visualization branch 4 times, most recently from daa9ca6 to 28cca67 Compare November 16, 2022 19:11

karthikjeeyar force-pushed the commit-visualization branch from 28cca67 to a96898b Compare November 16, 2022 19:45

christianvogt reviewed Nov 16, 2022

View reviewed changes

karthikjeeyar force-pushed the commit-visualization branch 2 times, most recently from 188a7e5 to 78e758c Compare November 17, 2022 10:51

add commit visualization

86ac6f6

karthikjeeyar force-pushed the commit-visualization branch from 78e758c to 24e0fcf Compare November 17, 2022 16:23

karthikjeeyar force-pushed the commit-visualization branch 3 times, most recently from 5b29354 to 252ca84 Compare November 18, 2022 11:43

jeff-phillips-18 reviewed Nov 18, 2022

View reviewed changes

karthikjeeyar force-pushed the commit-visualization branch from 252ca84 to 2ecf156 Compare November 18, 2022 13:32

karthikjeeyar force-pushed the commit-visualization branch 2 times, most recently from 8eb9afc to f949036 Compare November 21, 2022 07:27

Add unit tests

b551da6

karthikjeeyar force-pushed the commit-visualization branch from f949036 to b551da6 Compare November 21, 2022 07:43

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2022

jeff-phillips-18 approved these changes Nov 22, 2022

View reviewed changes

openshift-ci bot assigned jeff-phillips-18 Nov 22, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2022

openshift-merge-robot merged commit 246735e into openshift:main Nov 22, 2022

commit visualization in commit details page #254

commit visualization in commit details page #254

Uh oh!

Conversation

karthikjeeyar commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes

Description

Type of change

Screen shots / Gifs for design review

How to test or reproduce?

Uh oh!

karthikjeeyar commented Nov 9, 2022

Uh oh!

karthikjeeyar commented Nov 14, 2022

Uh oh!

MariaLeonova commented Nov 14, 2022

Uh oh!

karthikjeeyar commented Nov 15, 2022

Uh oh!

MariaLeonova commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeff-phillips-18 commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christianvogt commented Nov 15, 2022

Uh oh!

karthikjeeyar commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karthikjeeyar commented Nov 18, 2022

Uh oh!

karthikjeeyar commented Nov 21, 2022

Uh oh!

openshift-ci bot commented Nov 21, 2022

Uh oh!

christianvogt commented Nov 21, 2022

Uh oh!

openshift-ci bot commented Nov 22, 2022

Uh oh!

Uh oh!

karthikjeeyar commented Nov 9, 2022 •

edited

Loading

MariaLeonova commented Nov 15, 2022 •

edited

Loading

jeff-phillips-18 commented Nov 15, 2022 •

edited

Loading

karthikjeeyar commented Nov 16, 2022 •

edited

Loading

codecov bot commented Nov 17, 2022 •

edited

Loading