-
Notifications
You must be signed in to change notification settings - Fork 30
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
add info/error messages for SSD/HDD support - LSO mode #1437
add info/error messages for SSD/HDD support - LSO mode #1437
Conversation
5f46d95
to
fed4688
Compare
@@ -0,0 +1,71 @@ | |||
import * as React from 'react'; |
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.
this file is mostly a refactor, moving existing functionality into a separate hook... only needs high-level review...
@@ -0,0 +1,57 @@ | |||
import { LABEL_OPERATOR, LABEL_SELECTOR } from '@odf/core/constants'; |
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.
this file is mostly a refactor, moving existing common utils into a separate file... only needs high-level review...
@@ -0,0 +1,100 @@ | |||
import * as React from 'react'; |
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.
this file is mostly a refactor, moving existing functionality into a separate hook... only needs high-level review...
fed4688
to
d04bfe2
Compare
d04bfe2
to
e53efdc
Compare
React.useEffect(() => { | ||
// Reset pvCount that could have been set by another StorageClass. | ||
dispatch({ type: 'capacityAndNodes/pvCount', payload: 0 }); | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, []); | ||
|
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 remember we added this as part of "Performance mode" epic, but on taking a second look I see that we don't need this.
https://github.com/red-hat-storage/odf-console/blob/master/packages/odf/components/create-storage-system/reducer.ts#L192 should reset the redux state.
React.useEffect(() => { | ||
// Reset selected nodes that could have been set by another StorageClass. | ||
dispatch({ type: 'wizard/setNodes', payload: [] }); | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, []); | ||
|
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 remember we added this as part of "Performance mode" epic, but on taking a second look I see that we don't need this.
https://github.com/red-hat-storage/odf-console/blob/master/packages/odf/components/create-storage-system/reducer.ts#L192 should reset the redux state.
...orage-system/create-storage-system-steps/capacity-and-nodes-step/capacity-and-nodes-step.tsx
Show resolved
Hide resolved
LGTM |
make sense, will probably add it in a different PR though... it's already getting long due to lot of refactoring... |
/test odf-console-e2e-aws |
@bipuladh ⬆️ (just FYI) |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh, SanjalKatiyar 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 |
/test odf-console-e2e-aws |
f6b7f36
into
red-hat-storage:master
https://issues.redhat.com/browse/RHSTOR-5835
https://issues.redhat.com/browse/RHSTOR-5834
LSO configured as part of StorageSystem deployment -
SSDs are detected (not blocking users from deployment):
No SSDs are detected (blocking users from deployment):
LSO pre-configured before StorageSystem deployment -
No SSDs detected (blocking users from deployment):
In case LSO is deleted or any other edge case (not blocking users from deployment, showing a general info message instead):