support editing connection with unmatched connection type #3326

christianvogt · 2024-10-11T17:05:44Z

https://issues.redhat.com/browse/RHOAIENG-14274
https://issues.redhat.com/browse/RHOAIENG-14273

Description

Updates the empty state Add connection button to open the modal. Also disables the Add connection buttons when no connection types are present (note that this should not happen when we install OOTB types).

The annotation opendatahub.io/connection-type value from the Secret is used to reference a connection type. When no matching connection type is found now this reference is displayed as the connection type in the disabled drop down and all fields are displayed in their default state (env vars as labels and fields are all text inputs).

How Has This Been Tested?

Toggle the disableConnectionsTypes feature flag to false.
Navigate to the Data connections tab of the project details.
Create a new data connection.
Navigate to the Connections tab and edit the data connection.
Observe the connection type in the disabled dropdown reads s3.
Observe the form is open and fields are editable and can be saved.
Install the s3 connection type (until we have OOTB types) and use the admin page. It can be enabled or disabled (test both).

S3 connection type

kind: ConfigMap
apiVersion: v1
metadata:
name: s3
namespace: opendatahub
labels:
  opendatahub.io/connection-type: 'true'
  opendatahub.io/dashboard: 'true'
annotations:
  opendatahub.io/enabled: 'true'
  openshift.io/description: 'Connect to storage systems that are compatible with Amazon S3, enabling integration with other S3-compatible services and applications.'
  openshift.io/display-name: S3 compatible object storage - v1
data:
category: '["Object storage"]'
fields: '[{"type":"short-text","name":"Access Key","envVar":"AWS_ACCESS_KEY_ID","required":true,"properties":{}},{"type":"hidden","name":"Secret Key","envVar":"AWS_SECRET_ACCESS_KEY","required":true,"properties":{}},{"type":"short-text","name":"Endpoint","envVar":"AWS_S3_ENDPOINT","required":true,"properties":{}},{"name":"Region","description":"","envVar":"AWS_DEFAULT_REGION","type":"short-text","required":false,"properties":{}},{"type":"short-text","name":"Bucket","envVar":"AWS_S3_BUCKET","required":false,"properties":{}}]'

}

Navigate back to the Connections tab and edit the same connection.
Observe the fields now have proper labels and the connection type selected in the disabled dropdown reads S3 compatible object storage - v1. This time the connection type was used to generate the form.

Test Impact

Add unit test.

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

cc @simrandhaliw

openshift-ci · 2024-10-11T17:05:52Z

[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 ask for approval from christianvogt. 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-10-11T17:50:10Z

Codecov Report

Attention: Patch coverage is 92.42424% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.81%. Comparing base (a15a69c) to head (5680d01).

Files with missing lines	Patch %	Lines
...rc/concepts/connectionTypes/ConnectionTypeForm.tsx	92.68%	3 Missing ⚠️
...cts/screens/detail/connections/ConnectionsList.tsx	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3326      +/-   ##
==========================================
- Coverage   84.82%   84.81%   -0.02%     
==========================================
  Files        1315     1315              
  Lines       29492    29517      +25     
  Branches     8057     8069      +12     
==========================================
+ Hits        25016    25034      +18     
- Misses       4476     4483       +7

Files with missing lines	Coverage Δ
...ncepts/connectionTypes/fields/BooleanFormField.tsx	`62.50% <ø> (-5.93%)`	⬇️
frontend/src/concepts/connectionTypes/utils.ts	`92.92% <100.00%> (+0.19%)`	⬆️
...eens/detail/connections/ManageConnectionsModal.tsx	`93.54% <100.00%> (+0.07%)`	⬆️
...cts/screens/detail/connections/ConnectionsList.tsx	`92.30% <50.00%> (-7.70%)`	⬇️
...rc/concepts/connectionTypes/ConnectionTypeForm.tsx	`88.73% <92.68%> (+0.49%)`	⬆️

... and 1 file 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 a15a69c...5680d01. Read the comment docs.

emilys314 · 2024-10-11T17:26:30Z

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

@@ -160,15 +158,15 @@ export const ManageConnectionModal: React.FC<Props> = ({
              });
          }}
          error={error}
-          isSubmitDisabled={!isFormValid || !isModified}
+          isSubmitDisabled={!isFormValid || !isModified || !connectionTypeName || isSaving}


nit pick, || !connectionTypeName is duplicated logic that is already done in !isFormValid I believe

emilys314 · 2024-10-11T17:55:52Z

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

+  const [connectionTypes, connectionTypesLoaded, connectionTypesError] =
+    useWatchConnectionTypes(true);


So the <ManageConnectionModal /> will filter out disabled connections from being selectable, unless it's isEdit via enabledConnectionTypes.

<ConnectionTypeForm connectionTypes={isEdit ? connectionTypes : enabledConnectionTypes}

The current behavior in the main branch will pull up the correct field types of a disabled connection type. This will make it act like a missing lookup. If we want to keep the current behavior, the useWatchConnectionTypes doesn't need to be modified.

If we go with the new changes, you should probably remove the references to enabledConnectionTypes in frontend/src/pages/projects/screens/detail/connections/ManageConnectionsModal.tsx. But I don't think we should because the notebook connections modal's parent will also need to be updated. keeping this logic inside the ManageConnectionsModal.tsx encapsulate this logic.

Or we want the new behaviour you can change in the modal

<ConnectionTypeForm connectionTypes={enabledConnectionTypes}

I thought out this when I made the change. However I'm ok changing this back so that we can edit connections who's types have since been disabled.

Is the final verdict to change it back? if so it's still failing the lookup from line 179

options={!isEdit ? enabledConnectionTypes : undefined}

@emilys314 this is working for me as well as one of the unit tests.
I have an s3 connection type which is disabled. I then create a data connection and then edit it from the connections tab. The dropdown is disabled and shows the correct type in the dropdown. Similarly it in the table it also shows the correct type name.

As for line 179. When editing, I made it so that we do not supply options which will result in a disabled dropdown and it will create the single option based on the connection type passed it for editing.

Oh okay sorry, looks like my cluster didn't update or something, it's working now

emilys314 · 2024-10-11T18:01:43Z

frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx

@@ -31,6 +31,8 @@ const getConnectionTypeSelectOptions = (
  isPreview: boolean,
  selectedConnectionType?: ConnectionTypeConfigMapObj,
  connectionTypes?: ConnectionTypeConfigMapObj[],
+  disableTypeSelection?: boolean,


nit pick. Imo this isn't an accurate name for doing logic. Because in the preview form we also disable the type selection. A more accurate boolean would say the existing type reference was missing. (Also i could be wrong, but I don't think this boolean is needed at all for this logic?)

emilys314 · 2024-10-14T12:42:09Z

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

    [connectionTypes],
  );

+  const connectionTypeRef = connection?.metadata.annotations['opendatahub.io/connection-type'];


Was this a ref before? should the name be updated?

Other than that, everything look good

It was not a react ref. I have renamed it to not be confused with React ref.

christianvogt · 2024-10-15T17:29:30Z

/retest

christianvogt · 2024-10-15T17:32:17Z

rebased to re-run checks

openshift-ci · 2024-10-15T19:35:06Z

@christianvogt: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/odh-dashboard-pr-image-mirror	`12ebf0a`	link	true	`/test odh-dashboard-pr-image-mirror`

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-sigs/prow repository. I understand the commands that are listed here.

christianvogt requested a review from emilys314 October 11, 2024 17:05

openshift-ci bot requested review from lucferbux and pnaik1 October 11, 2024 17:05

emilys314 reviewed Oct 11, 2024

View reviewed changes

christianvogt force-pushed the ct-unknown branch 3 times, most recently from 60aea6c to e378a3a Compare October 11, 2024 19:15

emilys314 reviewed Oct 14, 2024

View reviewed changes

christianvogt force-pushed the ct-unknown branch from e378a3a to 5680d01 Compare October 15, 2024 13:27

christianvogt force-pushed the ct-unknown branch from 5680d01 to 5cb948c Compare October 15, 2024 17:32

support editing connection with unmatched connection type

12ebf0a

christianvogt force-pushed the ct-unknown branch from 5cb948c to 12ebf0a Compare October 15, 2024 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support editing connection with unmatched connection type #3326

support editing connection with unmatched connection type #3326

christianvogt commented Oct 11, 2024 •

edited

Loading

openshift-ci bot commented Oct 11, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading

emilys314 Oct 11, 2024

emilys314 Oct 11, 2024

christianvogt Oct 11, 2024

emilys314 Oct 11, 2024

christianvogt Oct 11, 2024

emilys314 Oct 14, 2024

emilys314 Oct 11, 2024

emilys314 Oct 14, 2024

christianvogt Oct 15, 2024

christianvogt commented Oct 15, 2024

christianvogt commented Oct 15, 2024

openshift-ci bot commented Oct 15, 2024

		const [connectionTypes, connectionTypesLoaded, connectionTypesError] =
		useWatchConnectionTypes(true);

support editing connection with unmatched connection type #3326

Are you sure you want to change the base?

support editing connection with unmatched connection type #3326

Conversation

christianvogt commented Oct 11, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Oct 11, 2024

codecov bot commented Oct 11, 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

Choose a reason for hiding this comment

christianvogt commented Oct 15, 2024

christianvogt commented Oct 15, 2024

openshift-ci bot commented Oct 15, 2024

christianvogt commented Oct 11, 2024 •

edited

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading