Add connection modal #3217

emilys314 · 2024-09-18T13:47:42Z

https://issues.redhat.com/browse/RHOAIENG-11555

Description

Creates a new connection in the form of a secret on the project.

SCREENSHOTS

result:

How Has This Been Tested?

If you have a connection type named s3, then you can view the results in data connections. otherwise check the secrets to verify the creation

Test Impact

cypress and jest 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.

@simrandhaliw

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-18T13:47:47Z

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

openshift-ci · 2024-09-18T13:47:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign manosnoam for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

frontend/OWNERS

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

codecov · 2024-09-18T16:17:55Z

Codecov Report

Attention: Patch coverage is 89.74359% with 16 lines in your changes missing coverage. Please review.

Project coverage is 85.64%. Comparing base (4cec257) to head (de17325).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...eens/detail/connections/ManageConnectionsModal.tsx	89.39%	7 Missing ⚠️
...onnectionTypes/ConnectionTypeDetailsHelperText.tsx	78.94%	4 Missing ⚠️
...rc/concepts/connectionTypes/ConnectionTypeForm.tsx	88.88%	4 Missing ⚠️
frontend/src/concepts/connectionTypes/utils.ts	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3217      +/-   ##
==========================================
+ Coverage   85.50%   85.64%   +0.13%     
==========================================
  Files        1280     1282       +2     
  Lines       28190    28365     +175     
  Branches     7535     7601      +66     
==========================================
+ Hits        24103    24292     +189     
+ Misses       4087     4073      -14

Files with missing lines	Coverage Δ
...ts/connectionTypes/ConnectionTypePreviewDrawer.tsx	`100.00% <ø> (ø)`
...onnectionTypes/fields/ConnectionTypeFormFields.tsx	`100.00% <100.00%> (+4.34%)`	⬆️
frontend/src/concepts/connectionTypes/types.ts	`100.00% <ø> (ø)`
...cts/screens/detail/connections/ConnectionsList.tsx	`100.00% <100.00%> (ø)`
frontend/src/concepts/connectionTypes/utils.ts	`93.90% <93.33%> (-0.13%)`	⬇️
...onnectionTypes/ConnectionTypeDetailsHelperText.tsx	`78.94% <78.94%> (ø)`
...rc/concepts/connectionTypes/ConnectionTypeForm.tsx	`88.88% <88.88%> (ø)`
...eens/detail/connections/ManageConnectionsModal.tsx	`89.39% <89.39%> (ø)`

... and 15 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 4cec257...de17325. Read the comment docs.

christianvogt

Connection preview displays the resource name header with no content. I believe this should be omitted here.

christianvogt · 2024-09-19T18:27:20Z

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

+        />
+      }
+    >
+      <ConnectionTypePreview


This shouldn't be named ConnectionTypePreview anymore since it's being reused here and the preview.
Maybe ConnectionTypeForm

christianvogt · 2024-09-19T18:30:26Z

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

+  }>(connection?.data ?? {});
+
+  // if user changes connection types, don't discard previous entries in case of accident
+  const [previousEntries, setPreviousEntries] = React.useState<{


I think the previous entries could simply be a ref instead because we don't need to react to this state change.

christianvogt · 2024-09-19T18:30:59Z

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

+        setNameDescData('name', '');
+        setNameDescData('description', '');
+        setConnectionValues({});
+      }
+      // load saved values?
+      if (type?.metadata.name && type.metadata.name in previousEntries) {
+        setNameDescData('name', previousEntries[type.metadata.name].nameDesc.name);
+        setNameDescData('description', previousEntries[type.metadata.name].nameDesc.description);


When switching connection types, the connection name and connection description are also wiped and restored. The name and description should remain when switching types.

christianvogt · 2024-09-19T18:33:58Z

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

+  connection?: Connection;
+  connectionTypes?: ConnectionTypeConfigMapObj[];
+  project?: ProjectKind;


These should all be required fields.

I think connection?: Connection can remain optional, since this is the scenario for a new connection. If it's edit, then connection will be a filled out value.

I can update the other 2 though for sure

christianvogt · 2024-09-19T18:37:12Z

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

+  connection?: Connection;
+  connectionTypes?: ConnectionTypeConfigMapObj[];
+  project?: ProjectKind;
+  onClose: (refresh?: boolean) => void;


Instead of naming the param refresh i think it would be better to describe what happened instead of what's expected of the caller to do. Other modals use submitted.

christianvogt · 2024-09-19T18:55:20Z

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

+            const connectionValuesAsStrings = Object.fromEntries(
+              Object.entries(connectionValues).map(([key, value]) => [key, String(value)]),
+            );


We need to discuss how dropdown values should be serialized / deserialized. Especially for multi dropdown. A multi drop down item containing a comma will be serialized as a string in a way that loses the ability to parse back the value.

christianvogt · 2024-09-19T18:55:45Z

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

+            const update: Connection = {
+              apiVersion: 'v1',
+              kind: 'Secret',
+              metadata: {
+                name: nameDescData.k8sName.value || translateDisplayNameForK8s(nameDescData.name),
+                namespace: project?.metadata.name ?? '',
+                labels: {
+                  'opendatahub.io/dashboard': 'true',
+                  'opendatahub.io/managed': 'true',
+                },
+                annotations: {
+                  'opendatahub.io/connection-type': selectedConnectionType?.metadata.name ?? '',
+                  'openshift.io/display-name': nameDescData.name,
+                  'openshift.io/description': nameDescData.description,
+                },
+              },
+              stringData: connectionValuesAsStrings,
+            };


Move to our utils in concepts.

christianvogt · 2024-09-19T18:58:09Z

frontend/src/concepts/connectionTypes/ConnectionTypePreview.tsx

+  setConnectionNameDesc,
+  connectionValues,
+  setConnectionValues,
+}) => {
  const connectionTypeName = obj && obj.metadata.annotations?.['openshift.io/display-name'];


I think if this is not a preview, we want to fall back to the metadata name therefore using the util getDisplayNameFromK8Resource

christianvogt · 2024-09-19T18:58:43Z

frontend/src/concepts/connectionTypes/ConnectionTypeDetailsHelperText.tsx

+  const displayName =
+    connectionType && connectionType.metadata.annotations?.['openshift.io/display-name'];


I think if this is not a preview, we want to fall back to the metadata name therefore using the util getDisplayNameFromK8Resource

christianvogt · 2024-09-19T18:59:23Z

frontend/src/concepts/connectionTypes/ConnectionTypeDetailsHelperText.tsx

+import CategoryLabel from './CategoryLabel';
+
+type Props = {
+  connectionType?: ConnectionTypeConfigMapObj;


This should be a required prop.

christianvogt · 2024-09-20T13:19:49Z

frontend/src/concepts/connectionTypes/ConnectionTypeDetailsHelperText.tsx

+                      ))}
+                    </LabelGroup>
+                  ) : (
+                    <UnspecifiedValue />


A connection type should always have at least one category but good to have fall backs.
When not in preview we can omit the category altogether or render -. But UnspecifiedValue is for preview only.

Updated with a isPreview ? <UnspecifiedValues /> : "-"

I don't really like the 2 ternaries being chained but yeah

christianvogt · 2024-09-20T13:34:29Z

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

+  const isValid = React.useMemo(
+    () =>
+      !!selectedConnectionType &&
+      !!nameDescData.name &&
+      !selectedConnectionType.data?.fields?.find(
+        (field) =>
+          isConnectionTypeDataField(field) &&
+          field.required &&
+          !connectionValues[field.envVar] &&
+          field.type !== ConnectionTypeFieldType.Boolean,
+      ),
+    [selectedConnectionType, nameDescData, connectionValues],
+  );


Individual form fields have an onValidate callback. We need to hook into those such that errors are propagated to here.
You can test this with invalid URIs.

frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx

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

emilys314 force-pushed the project-connection-add-modal branch from dfb9dd5 to 71d923a Compare September 18, 2024 15:35

emilys314 force-pushed the project-connection-add-modal branch 4 times, most recently from bc21fd9 to ac0bad6 Compare September 19, 2024 13:12

emilys314 marked this pull request as ready for review September 19, 2024 14:42

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

openshift-ci bot requested review from dpanshug and pnaik1 September 19, 2024 14:43

christianvogt reviewed Sep 19, 2024

View reviewed changes

emilys314 added 3 commits September 20, 2024 09:30

Add connection modal

623cf58

prefill default values if type preselected

1ff42d6

PR updates

66cb7f8

emilys314 force-pushed the project-connection-add-modal branch from 5ca2473 to 66cb7f8 Compare September 20, 2024 13:31

christianvogt reviewed Sep 20, 2024

View reviewed changes

frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx Show resolved Hide resolved

Check fields for validation

92f5b94

christianvogt mentioned this pull request Sep 20, 2024

adjust resource name form spacing and help icon #3234

Open

6 tasks

Set form state with function instead of values

de17325

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connection modal #3217

Add connection modal #3217

emilys314 commented Sep 18, 2024 •

edited

Loading

openshift-ci bot commented Sep 18, 2024

openshift-ci bot commented Sep 18, 2024

codecov bot commented Sep 18, 2024 •

edited

Loading

christianvogt left a comment

christianvogt Sep 19, 2024

christianvogt Sep 19, 2024

christianvogt Sep 19, 2024

christianvogt Sep 19, 2024

emilys314 Sep 19, 2024

christianvogt Sep 19, 2024

christianvogt Sep 19, 2024

christianvogt Sep 19, 2024

christianvogt Sep 19, 2024

christianvogt Sep 19, 2024

christianvogt Sep 19, 2024

christianvogt Sep 20, 2024

emilys314 Sep 20, 2024

christianvogt Sep 20, 2024

		const displayName =
		connectionType && connectionType.metadata.annotations?.['openshift.io/display-name'];

Add connection modal #3217

Are you sure you want to change the base?

Add connection modal #3217

Conversation

emilys314 commented Sep 18, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Sep 18, 2024

openshift-ci bot commented Sep 18, 2024

codecov bot commented Sep 18, 2024 • edited Loading

Codecov Report

christianvogt left a 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

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

emilys314 commented Sep 18, 2024 •

edited

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading