Add P0 OOTB connection types #3335

jeff-phillips-18 · 2024-10-15T12:22:22Z

Description

Adds yaml files for OOTB connection types. Updates the connection type page to not allow editing of the OOTB connection types. Fixes an issue where the connection types table does not update when new connections are added or modified.

How Has This Been Tested?

Run the UI and navigate to Settings -> Connection types
Install the connection types:
- oc apply -f manifests/common/connection-types/s3.yaml
- oc apply -f manifests/common/connection-types/oci-compliant-registry-v1.yaml
- oc apply -f manifests/common/connection-types/uri-v1.yaml
Verify the connection types are shown in the UI without needing to refresh the page within 30 seconds
From the OpenShift Console, go to Administrator -> Home -> Search
- Select the opendatahub project
- Select Resource = OdhApplication
- Find the jupyter OdhApplication and copy its ownerReference section of the Yaml file
- Go back to the Search page and Select the ConfigMap Resource
- Edit the s3, oci-compliant-registry-v1, and uri-v1 configmaps:
  - Add the copied ownerReference section to each of them
- Check the UI and see that each now has Pre-installed for the creator column
- Verify the Edit action in the kebab is disabled for each.

Test Impact

None, test already covered this functionality

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)

openshift-ci · 2024-10-15T12:22:29Z

[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 gkrumbach07 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:

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-15T13:04:35Z

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.07%. Comparing base (4d29174) to head (da01542).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
.../connectionTypes/manage/EditConnectionTypePage.tsx	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3335      +/-   ##
==========================================
+ Coverage   84.83%   85.07%   +0.23%     
==========================================
  Files        1326     1327       +1     
  Lines       29651    29774     +123     
  Branches     8101     8150      +49     
==========================================
+ Hits        25155    25329     +174     
+ Misses       4496     4445      -51

Files with missing lines	Coverage Δ
.../pages/connectionTypes/ConnectionTypesTableRow.tsx	`67.92% <ø> (ø)`
.../connectionTypes/manage/EditConnectionTypePage.tsx	`71.42% <50.00%> (-8.58%)`	⬇️

... and 39 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 4d29174...da01542. Read the comment docs.

christianvogt

While the Edit action is disabled, i can select the duplicate action, then edit the URL from /duplicate/ to /edit/ and then I can save any changes. I believe the operator should just revert the change, however I don't think the user should be able to perform this action from the ui.

The delete action should not be enabled either. Should it be hidden? @simrandhaliw

Do we know if we should allow the admin to disable these connection types?

manifests/common/connection-types/oci-compliant-registry-v1.yaml

jeff-phillips-18 · 2024-10-16T16:42:25Z

Screen shot if user modifies the URL to edit an OOTB connection type

frontend/src/pages/connectionTypes/ConnectionTypesTableRow.tsx

christianvogt · 2024-10-17T20:24:17Z

/hold

Since we are not yet releasing this feature, I want to be absolutely sure these resources are correct because I'm also unsure how these resources would be reconciled if we need to make additional changes.

christianvogt · 2024-10-17T20:48:08Z

manifests/common/connection-types/s3.yaml

+    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","properties":{},"required":true},{"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":{}},{"type":"short-text","name":"Region","envVar":"AWS_DEFAULT_REGION","required":false,"properties":{}},{"type":"short-text","name":"Bucket","envVar":"AWS_S3_BUCKET","required":false,"properties":{}},{"type":"short-text","name":"Path","envVar":"AWS_S3_PATH","properties":{}}]'


path is not part of data connections today. I do not believe it should be part of the the connection type. Will start a discussion on this.

Agreed -- we should make sure that field pops up when selecting an S3-esk connection in the Model Serving modal.

christianvogt · 2024-10-17T20:56:33Z

frontend/src/pages/connectionTypes/manage/EditConnectionTypePage.tsx

+  if (existingConnectionType && ownedByDSC(existingConnectionType)) {
+    return (
+      <ApplicationsPage
+        loaded={isLoaded}
+        errorMessage="Unable to edit"
+        loadError={new Error('This connection type is not editable')}
+        empty
+      />
+    );
+  }


I feel like we should just redirect the user back to /conectionTypes instead of displaying this page. The only way they can get here is by direct URL anyways.

andrewballantyne

@christianvogt

Since we are not yet releasing this feature, I want to be absolutely sure these resources are correct because I'm also unsure how these resources would be reconciled if we need to make additional changes.

Can we make sure we can do this no matter what -- We'll need to do updates almost guaranteed to these resources at some point on upgrade. Can we not make this a first-class feature to allow for manifests (eg. devflags) to change?

andrewballantyne · 2024-10-21T17:43:11Z

manifests/common/connection-types/oci-compliant-registry-v1.yaml

Do we have support for this in the UI yet?

andrewballantyne · 2024-10-21T17:43:49Z

manifests/common/connection-types/s3.yaml

+    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","properties":{},"required":true},{"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":{}},{"type":"short-text","name":"Region","envVar":"AWS_DEFAULT_REGION","required":false,"properties":{}},{"type":"short-text","name":"Bucket","envVar":"AWS_S3_BUCKET","required":false,"properties":{}},{"type":"short-text","name":"Path","envVar":"AWS_S3_PATH","properties":{}}]'


Agreed -- we should make sure that field pops up when selecting an S3-esk connection in the Model Serving modal.

christianvogt · 2024-10-21T18:59:14Z

@andrewballantyne these resources will be updated on upgrade like the other resources. However they aren't reconciled immediately on change in case a privileged user alters them.

Seems like some churn is still happening on the definition side of these resources.

andrewballantyne · 2024-10-21T19:06:53Z

@christianvogt Sorry, I'm not clear what you mean by "a privileged user alters them"... either they are reconciled by the operator or they are not I thought 🤔 If you oc apply these resources, they are not quite the same as if you used devFlags or an operator upgrade (which I believe to be the same if memory serves).

christianvogt · 2024-10-21T20:32:06Z

@andrewballantyne we found out that the resources are not reconciled on change by the operator but only on upgrade or when the operator controller is restarted. So what I was getting at is if a privileged user happens to change the resource, it doesn't get reset until one of those scenarios hit. This is something @lucferbux and I found strange and would need some further looking into if we wanted to change that behavior.

openshift-ci bot requested review from Gkrumbach07 and lucferbux October 15, 2024 12:22

christianvogt reviewed Oct 15, 2024

View reviewed changes

manifests/common/connection-types/oci-compliant-registry-v1.yaml Outdated Show resolved Hide resolved

jeff-phillips-18 force-pushed the ootb-connection-types branch from 2c0e398 to 5bc94a5 Compare October 16, 2024 16:40

christianvogt reviewed Oct 16, 2024

View reviewed changes

frontend/src/pages/connectionTypes/ConnectionTypesTableRow.tsx Show resolved Hide resolved

jeff-phillips-18 force-pushed the ootb-connection-types branch 2 times, most recently from d43ce05 to c211947 Compare October 16, 2024 21:19

openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Oct 17, 2024

christianvogt reviewed Oct 17, 2024

View reviewed changes

Add P0 OOTB connection types

da01542

jeff-phillips-18 force-pushed the ootb-connection-types branch from c211947 to da01542 Compare October 21, 2024 15:59

andrewballantyne requested changes Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add P0 OOTB connection types #3335

Add P0 OOTB connection types #3335

jeff-phillips-18 commented Oct 15, 2024

openshift-ci bot commented Oct 15, 2024

codecov bot commented Oct 15, 2024 •

edited

Loading

christianvogt left a comment •

edited

Loading

jeff-phillips-18 commented Oct 16, 2024

christianvogt commented Oct 17, 2024

christianvogt Oct 17, 2024

andrewballantyne Oct 21, 2024

christianvogt Oct 17, 2024

andrewballantyne left a comment

andrewballantyne Oct 21, 2024

andrewballantyne Oct 21, 2024

christianvogt commented Oct 21, 2024

andrewballantyne commented Oct 21, 2024

christianvogt commented Oct 21, 2024

Add P0 OOTB connection types #3335

Are you sure you want to change the base?

Add P0 OOTB connection types #3335

Conversation

jeff-phillips-18 commented Oct 15, 2024

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Oct 15, 2024

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

christianvogt left a comment • edited Loading

Choose a reason for hiding this comment

jeff-phillips-18 commented Oct 16, 2024

Screen shot if user modifies the URL to edit an OOTB connection type

christianvogt commented Oct 17, 2024

christianvogt Oct 17, 2024

Choose a reason for hiding this comment

andrewballantyne Oct 21, 2024

Choose a reason for hiding this comment

christianvogt Oct 17, 2024

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne Oct 21, 2024

Choose a reason for hiding this comment

andrewballantyne Oct 21, 2024

Choose a reason for hiding this comment

christianvogt commented Oct 21, 2024

andrewballantyne commented Oct 21, 2024

christianvogt commented Oct 21, 2024

codecov bot commented Oct 15, 2024 •

edited

Loading

christianvogt left a comment •

edited

Loading