-
Notifications
You must be signed in to change notification settings - Fork 168
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
Merge feature branch f/storage-class #2503
Merge feature branch f/storage-class #2503
Conversation
… AppContext and additional property. Storage classes info only gets loaded anew on app starts or hard browser page refresh. Custom hook usePreferredStorageClass for decision logic. StorageClassKind array of cluster storage classes. get logic in App / AppContext and additional property. Storage classes info only gets loaded anew on app starts or hard browser page refresh. Custom hook usePreferredStorageClass for decision logic.
first try at storageclasses get and promises
…tring equality compare instead of string includes. also using strict equality comparison
fix bug when filtering cluster storageclasses and now doing correct s…
Merge `main` into `f/storage-class`
frontend/src/pages/projects/screens/spawner/storage/usePreferredStorageClass.ts
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx
Outdated
Show resolved
Hide resolved
@shalberd thanks for all the feedback it's really useful, I'll continue reading through it and making changes / comments tomorrow morning. My afternoons tend to get lost to meetings / planning etc - but getting your work merged, this week if possible is my goal. |
frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/detail/storage/ManageStorageModal.tsx
Outdated
Show resolved
Hide resolved
@alexcreasy looking good as a whole, I only have one small nit with the const name: https://github.com/opendatahub-io/odh-dashboard/pull/2503/files#r1502735537 and another small one with order of properties import in AppContext https://github.com/opendatahub-io/odh-dashboard/pull/2503/files#r1489324880 |
/lgtm my comments can be resolved |
@shalberd: changing LGTM is restricted to collaborators In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, shalberd 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 |
Closes: RHOAIENG-2872
Description
Adds ability to set a storage class via Dashboard CR value
storageClassName
when no default storage class is set on the clusterHow Has This Been Tested?
Test with and without a default storage class.
When testing without a default storage class:
storageClassName
to a given storage class (you can create a new one if neccessary).Test with a default storage class:
Test Impact
Relevant tests have been updated to work with new features.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main