Skip to content
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

MR fixes identified during architecture review #3263

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

mturley
Copy link
Contributor

@mturley mturley commented Sep 26, 2024

Followup to #3252 and #3249 as well as some misc discussion points from arch review (https://issues.redhat.com/browse/RHOAIENG-13439).

Description

  • After feat(13045): mr permissions and rolebindings #3249 was rebased on the changes from Use a ResourceWatcher for DSC status in the backend instead of fetching it once at app startup to resolve model registries namespace #3252, testing it revealed that because we removed the model registry namespace from the checks in backend/src/utils/route-security.ts and rolebindings we create in MR settings include that namespace in the request body, creation of rolebindings failed due to the namespace checking in requestSecurityGuard. We do not want to modify route-security because it is a fragile workaround and modifying it involves high risk. The fix instead is to simply not include a namespace in the rolebinding object when we send it to the backend and instead inject it just before calling the cluster API.
  • When deleting a registry in the backend, we were not returning the promise from the deleteModelRegistryAndSecret util which means we weren't catching errors that came up in this deletion.
  • There was a TODO comment asking if we needed some code in our error handler util. That code is also present in frontend/src/api/pipelines/errorUtils.ts, and it is needed because some requests in MR throw a NotReadyError which should be ignored by this handler. Removed the TODO comment.

How Has This Been Tested?

Tested on modelregistry-ui cluster, we can now create rolebindings in MR settings again

Test Impact

N/A, no tests in the backend

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

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Sep 26, 2024
@mturley mturley changed the title [WIP] MR fixes identified during architecture review MR fixes identified during architecture review Sep 26, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Sep 26, 2024
Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more-or-less what I expected. Let me test it out

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

Copy link
Contributor

openshift-ci bot commented Sep 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

openshift-ci bot commented Sep 26, 2024

New changes are detected. LGTM label has been removed.

@mturley mturley added the lgtm label Sep 26, 2024
Copy link
Contributor

openshift-ci bot commented Sep 27, 2024

New changes are detected. LGTM label has been removed.

@mturley mturley added the lgtm label Sep 27, 2024
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.98%. Comparing base (053909f) to head (3b6b067).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3263      +/-   ##
==========================================
+ Coverage   84.96%   84.98%   +0.01%     
==========================================
  Files        1302     1302              
  Lines       29101    29102       +1     
  Branches     7828     7828              
==========================================
+ Hits        24727    24731       +4     
+ Misses       4374     4371       -3     
Files with missing lines Coverage Δ
frontend/src/api/modelRegistry/errorUtils.ts 100.00% <ø> (ø)
...ntend/src/services/modelRegistrySettingsService.ts 56.52% <100.00%> (+0.96%) ⬆️

... and 4 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 053909f...3b6b067. Read the comment docs.

@openshift-merge-bot openshift-merge-bot bot merged commit 3783c3f into opendatahub-io:main Sep 27, 2024
8 checks passed
@mturley mturley deleted the mr-mvp-signoff-fixes branch September 27, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants