-
Notifications
You must be signed in to change notification settings - Fork 163
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
Fix for untrue !Deleted flag #3049
Fix for untrue !Deleted flag #3049
Conversation
3a73507
to
570aa34
Compare
3596a57
to
32eb024
Compare
frontend/src/pages/projects/screens/detail/notebooks/__tests__/useNotebookImageData.spec.ts
Outdated
Show resolved
Hide resolved
32eb024
to
37c6f8e
Compare
frontend/src/pages/projects/screens/detail/notebooks/__tests__/useNotebookImageData.spec.ts
Outdated
Show resolved
Hide resolved
3dc53ca
to
447b9d1
Compare
@lucferbux can you please review this change |
e7c057e
to
39e6700
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.
Ok, with those tweaks the PR should be ready to go, it's great that the testint caught our error before integrating.
frontend/src/pages/projects/screens/detail/notebooks/useNotebookImageData.ts
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/detail/notebooks/useNotebookImageData.ts
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/detail/notebooks/useNotebookImageData.ts
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/detail/notebooks/useNotebookImageData.ts
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/detail/notebooks/useNotebookImageData.ts
Show resolved
Hide resolved
frontend/src/pages/projects/screens/detail/notebooks/useNotebookImageData.ts
Outdated
Show resolved
Hide resolved
f36a69f
to
ad52267
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.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lucferbux 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 |
/hold |
Hi @lucferbux , I won't get to this today in the end, but will try to do it the first thing tomorrow. Andriana is on PTO, so she won't respond till next week, I'm afraid. |
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.
I reviewed the code briefly and put two comments. Otherwise LGTM, but I'm not a typescript developer, so take this into account 🙂
Also, I tried the image on my cluster and seems that the fix works as expected.
frontend/src/pages/projects/screens/detail/notebooks/__tests__/useNotebookImageData.spec.ts
Show resolved
Hide resolved
frontend/src/pages/projects/screens/detail/notebooks/useNotebookImageData.ts
Show resolved
Hide resolved
ad52267
to
1a4e8cd
Compare
/unhold |
/lgtm |
Closes: RHOAIENG-8553
Description
Fixes a bug where a workbench created with a custom images shows a "! Deleted" flag when not true
Steps to reproduce this bug is in the JIRA ticket linked. Note the cluster type. I'm unable to reproduce this bug on a ROSA cluster currently.
Please reach out to me on slack if you need help testing this.
How Has This Been Tested?
npm run test
. Locally on an OCP clusterTest Impact
A jest test was added that checks if the fix fails when the image doesn't match
Before:
After:
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main