Add connections section to project details #3183

emilys314 · 2024-09-10T19:01:40Z

Closes https://issues.redhat.com/browse/RHOAIENG-11554

Description

Replaces the "Data connections" section if the new connections type flag is on. Right now it's backwards compatible with existing data connections for viewing. But add / edit / delete will come in later PRs.

How Has This Been Tested?

make sure the disableConnectionTypes flag is set to false
go to a project, and select the "Connections" tab

Test Impact

Some basics tests added

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

openshift-ci · 2024-09-10T19:01:44Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2024-09-10T19:43:45Z

Codecov Report

Attention: Patch coverage is 93.54839% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.29%. Comparing base (917b4af) to head (90c37b7).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
...screens/detail/connections/ConnectionsTableRow.tsx	88.88%	2 Missing ⚠️
...eens/detail/connections/connectionsTableColumns.ts	83.33%	1 Missing ⚠️
...jects/screens/detail/connections/useConnections.ts	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3183      +/-   ##
==========================================
+ Coverage   85.27%   85.29%   +0.01%     
==========================================
  Files        1258     1263       +5     
  Lines       27689    27751      +62     
  Branches     7372     7390      +18     
==========================================
+ Hits        23611    23669      +58     
- Misses       4078     4082       +4

Files with missing lines	Coverage Δ
.../src/components/table/TableRowTitleDescription.tsx	`100.00% <100.00%> (+5.88%)`	⬆️
frontend/src/concepts/connectionTypes/types.ts	`100.00% <100.00%> (ø)`
frontend/src/concepts/design/utils.ts	`90.22% <100.00%> (+0.30%)`	⬆️
...ntend/src/pages/projects/ProjectDetailsContext.tsx	`93.65% <100.00%> (+0.10%)`	⬆️
...c/pages/projects/screens/detail/ProjectDetails.tsx	`96.66% <100.00%> (+0.37%)`	⬆️
...cts/screens/detail/connections/ConnectionsList.tsx	`100.00% <100.00%> (ø)`
...ts/screens/detail/connections/ConnectionsTable.tsx	`100.00% <100.00%> (ø)`
...rontend/src/pages/projects/screens/detail/const.ts	`100.00% <ø> (ø)`
...rontend/src/pages/projects/screens/detail/types.ts	`100.00% <100.00%> (ø)`
...eens/detail/connections/connectionsTableColumns.ts	`83.33% <83.33%> (ø)`
... and 2 more

... and 12 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

christianvogt · 2024-09-11T17:29:51Z

frontend/src/pages/projects/screens/detail/ProjectDetails.tsx

+        ...(connectionTypesEnabled
+          ? [
+              {
+                id: ProjectSectionID.CONNECTIONS,
+                title: 'Connections',
+                component: <ConnectionsList />,
+              },
+            ]
+          : [
+              {
+                id: ProjectSectionID.DATA_CONNECTIONS,
+                title: 'Data connections',
+                component: <DataConnectionsList />,
+              },
+            ]),


For now keep both present as I want to use the existing data connection tab for testing.

christianvogt · 2024-09-11T17:41:19Z

frontend/src/pages/projects/screens/detail/connections/ConnectionsTableRow.tsx

+      />
+    </Td>
+    <Td dataLabel="Type">{obj.metadata.annotations['opendatahub.io/connection-type']}</Td>
+    <Td dataLabel="Compatibility">-</Td>


Omit compatibility as a whole until we're ready to support this feature from the admin side as well.

christianvogt · 2024-09-11T17:42:14Z

frontend/src/pages/projects/screens/detail/connections/__tests__/ConnectionsTable.spec.tsx

+const connections: Connection[] = [
+  {
+    kind: 'Secret',
+    apiVersion: 'v1',
+    metadata: {
+      name: 'connection1',
+      namespace: 'ds-project',
+      labels: { 'opendatahub.io/dashboard': 'true', 'opendatahub.io/managed': 'true' },
+      annotations: {
+        'opendatahub.io/connection-type': 's3',
+        'openshift.io/display-name': 'connection1',
+        'openshift.io/description': 'desc1',
+      },
+    },
+    data: {},
+  },
+];


What about using mockDataConnection instead or creating your own new mock util?

The old type of DataConnection doesn't fully line up. I can relax some of the properties in Connection and make them nullish which helps. But then the data property is a pain to properly convert, since it's an enum of the AwsKeys.

I could just do the following (but the description annotation doesn't exist in DataConnection):

export const mockConnection = (args: MockDataConnectionType): Connection => mockDataConnection(args).data as unknown as Connection;

(And yes the .data is needed because that is actually the SecretKind)

Let's make a new mock util for these connection and then we can eventually remove the old data connection mock and types.

Updated. I made frontend/src/__mocks__/mockConnection.ts

(I'm still not 100% sure on if it should be it's own file or part connection types, like mockConnectionType.ts, but it is it's own file)

frontend/src/pages/projects/screens/detail/connections/types.ts

christianvogt · 2024-09-11T17:50:38Z

frontend/src/pages/projects/screens/detail/connections/ConnectionsTableRow.tsx

+        description={obj.metadata.annotations['openshift.io/description']}
+      />
+    </Td>
+    <Td dataLabel="Type">{obj.metadata.annotations['opendatahub.io/connection-type']}</Td>


We're going to want to resolve this type to a connection type to pull its display name. Fallback should be the annotation value if the type fails to resolve.

Updated. I decided to just pull all the connection types upfront on going to this subsection, since it's going to be used a lot anyways. Then pass it down for wherever it's needed.
So that network request has been added to the pages isLoading and loadError, which the page could technically work even if it failed, but it probably makes sense to have still.

christianvogt · 2024-09-12T20:15:41Z

/lgtm
/approve

openshift-ci · 2024-09-12T20:15:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt

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:

~~frontend/OWNERS~~ [christianvogt]

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

Add connections section to project details

5c2ff28

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Sep 10, 2024

emilys314 marked this pull request as ready for review September 10, 2024 19:20

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Sep 10, 2024

openshift-ci bot requested review from lucferbux and manaswinidas September 10, 2024 19:20

christianvogt reviewed Sep 11, 2024

View reviewed changes

emilys314 force-pushed the project-details-connections-list branch from 3d0fe83 to c059291 Compare September 11, 2024 21:06

Use connection type display name if available and type path changes

90c37b7

emilys314 force-pushed the project-details-connections-list branch from c059291 to 90c37b7 Compare September 11, 2024 22:21

openshift-ci bot assigned christianvogt Sep 12, 2024

openshift-ci bot added the lgtm label Sep 12, 2024

openshift-ci bot added the approved label Sep 12, 2024

openshift-merge-bot bot merged commit 06e065b into opendatahub-io:main Sep 12, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connections section to project details #3183

Add connections section to project details #3183

emilys314 commented Sep 10, 2024

openshift-ci bot commented Sep 10, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading

christianvogt Sep 11, 2024

christianvogt Sep 11, 2024

christianvogt Sep 11, 2024

emilys314 Sep 11, 2024

christianvogt Sep 11, 2024

emilys314 Sep 11, 2024

christianvogt Sep 11, 2024

emilys314 Sep 11, 2024

christianvogt commented Sep 12, 2024

openshift-ci bot commented Sep 12, 2024

Add connections section to project details #3183

Add connections section to project details #3183

Conversation

emilys314 commented Sep 10, 2024

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Sep 10, 2024

codecov bot commented Sep 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Sep 12, 2024

openshift-ci bot commented Sep 12, 2024

codecov bot commented Sep 10, 2024 •

edited

Loading