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

Merge feature branch f/storage-class #2503

Merged

Conversation

alexcreasy
Copy link
Contributor

@alexcreasy alexcreasy commented Feb 13, 2024

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 cluster

How Has This Been Tested?

Test with and without a default storage class.

When testing without a default storage class:

  1. Set dashboard CR value storageClassName to a given storage class (you can create a new one if neccessary).
  2. From the project detail page, create a "Cluster storage"
  3. Verify the correct storage class is assigned to the PVC via the openshift console.
  4. Create a Workbench, create a new storage class in the process, verify the PVC as in step 3.
  5. Edit the above work bench, in the modal that pops up, create a new PVC again and verify as in step 3.

Test with a default storage class:

  1. Set a default storage class on the cluster
  2. Follow test instructions exactly as above, but verify at each step that the storage class is set to the default, not the CR value.

Test Impact

Relevant tests have been updated to work with new features.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • 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 tests & storybook 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 (find relevant UX in the SMEs section).

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

shalberd and others added 8 commits September 30, 2023 09:34
… 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…
@alexcreasy alexcreasy added the do-not-merge/work-in-progress This PR is in WIP state label Feb 13, 2024
@alexcreasy
Copy link
Contributor Author

@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.

@shalberd
Copy link
Contributor

@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

@shalberd
Copy link
Contributor

shalberd commented Feb 26, 2024

/lgtm

my comments can be resolved

@alexcreasy alexcreasy changed the title [WIP] Merge feature branch f/storage-class Merge feature branch f/storage-class Feb 26, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Feb 26, 2024
Copy link
Contributor

openshift-ci bot commented Feb 26, 2024

@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.

Copy link
Contributor

openshift-ci bot commented Feb 27, 2024

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

@openshift-merge-bot openshift-merge-bot bot merged commit cf3812d into opendatahub-io:main Feb 27, 2024
6 checks passed
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.

3 participants