-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Begin data connection clean up #3660
Begin data connection clean up #3660
Conversation
Skipping CI for Draft Pull Request. |
39db0e7
to
ea0b5ed
Compare
6ebc0c1
to
4944255
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey you started removing connections (the new code) instead of the data connections (the old stuff) can you check the rest of your PR? thanks
...rc/pages/modelServing/screens/projects/InferenceServiceModal/ManageInferenceServiceModal.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/modelServing/screens/projects/kServeModal/ManageKServeModal.tsx
Outdated
Show resolved
Hide resolved
c0e597d
to
881757a
Compare
frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDeploy.cy.ts
Show resolved
Hide resolved
250e904
to
6d2eb0e
Compare
FYI This will cause merge conflicts with #3590 - depending on what goes in first |
6d2eb0e
to
d5bad83
Compare
d5bad83
to
0ed7e70
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3660 +/- ##
==========================================
- Coverage 84.15% 83.48% -0.67%
==========================================
Files 1453 1453
Lines 33866 33787 -79
Branches 9384 9363 -21
==========================================
- Hits 28499 28207 -292
- Misses 5367 5580 +213
... and 45 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also how much are we going to remove? The PR states it's the beginning, but the jira item implies all?
Additionally you have some orphaned files you can remove related to data connections to, like the entire folder of frontend/src/pages/projects/screens/detail/data-connections
frontend/src/__tests__/cypress/cypress/tests/mocked/modelServing/servingRuntimeList.cy.ts
Outdated
Show resolved
Hide resolved
@emilys314 The plan is to remove it all, but we had discussed just starting with the removal of the feature flag and conditionally rendered code related to it, as well as fixing some of the test cases to use connections. A second deliverable would be removal of the remaining dead code. I assumed that it might be easier to open a second jira ticket for that, but unsure. |
0ed7e70
to
29e3643
Compare
frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDeploy.cy.ts
Show resolved
Hide resolved
frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelVersionDeploy.cy.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm Tested locally and everything worked as I expected. Code changes seem reasonable but I'll leave it up to the experts 😄
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: Griffin-Sullivan 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:
Approvers can indicate their approval by writing |
* Data connection clean up * Clean up some DataConnections code in Cypress tests * Remove connectionTypes SupportedArea * Cypress fixes
* Data connection clean up * Clean up some DataConnections code in Cypress tests * Remove connectionTypes SupportedArea * Cypress fixes
Closes: RHOAIENG-15563
Description
This PR begins to clean up old code from data connections. Removes the isConnectionTypesEnabled feature flag from the list of options, and cleans up some Cypress code related to testing data connections
How Has This Been Tested?
tested locally
Test Impact
Cypress tests updated to remove some data connections tests
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main