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:
- Set the project to opendatahub: oc project opendatahub
- 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

Screen shots

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)

/cc @simrandhaliw

codecov · 2024-10-15T13:04:35Z

Codecov Report

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

Please upload report for BASE (main@2e859cb). Learn more about missing BASE report.
Report is 2 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        ?   85.12%           
=======================================
  Files           ?     1337           
  Lines           ?    30117           
  Branches        ?     8265           
=======================================
  Hits            ?    25638           
  Misses          ?     4479           
  Partials        ?        0

Files with missing lines	Coverage Δ
.../pages/connectionTypes/ConnectionTypesTableRow.tsx	`69.09% <ø> (ø)`
.../connectionTypes/manage/EditConnectionTypePage.tsx	`71.42% <50.00%> (ø)`

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 2e859cb...aca6c4c. 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.

christianvogt · 2024-10-28T18:50:18Z

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

+    opendatahub.io/connection-type: 'true'
+    opendatahub.io/dashboard: 'true'
+  annotations:
+    opendatahub.io/enabled: 'true'


Remove all opendatahub.io/enabled annotations.

jeff-phillips-18 · 2024-10-29T11:28:13Z

/hold cancel

christianvogt · 2024-10-29T14:28:54Z

/lgtm
/approve

openshift-ci · 2024-10-29T14:29:02Z

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

~~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 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

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

christianvogt reviewed Oct 28, 2024

View reviewed changes

Add P0 OOTB connection types

aca6c4c

jeff-phillips-18 force-pushed the ootb-connection-types branch from da01542 to aca6c4c Compare October 29, 2024 11:21

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

openshift-ci bot assigned christianvogt Oct 29, 2024

openshift-ci bot added the lgtm label Oct 29, 2024

openshift-ci bot added the approved label Oct 29, 2024

openshift-merge-bot bot merged commit e02104b into opendatahub-io:main Oct 29, 2024
6 checks passed

jeff-phillips-18 deleted the ootb-connection-types branch October 29, 2024 15:03

christianvogt mentioned this pull request Oct 29, 2024

add model serving compatible select to connection type form #3396

Merged

6 tasks

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 •

edited

Loading

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

christianvogt Oct 28, 2024

jeff-phillips-18 commented Oct 29, 2024

christianvogt commented Oct 29, 2024

openshift-ci bot commented Oct 29, 2024

Add P0 OOTB connection types #3335

Add P0 OOTB connection types #3335

Conversation

jeff-phillips-18 commented Oct 15, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Screen shots

Request review criteria:

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

christianvogt Oct 28, 2024

Choose a reason for hiding this comment

jeff-phillips-18 commented Oct 29, 2024

christianvogt commented Oct 29, 2024

openshift-ci bot commented Oct 29, 2024

jeff-phillips-18 commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

christianvogt left a comment •

edited

Loading