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

iSCSI UI #435

Merged
merged 19 commits into from
Mar 16, 2023
Merged

iSCSI UI #435

merged 19 commits into from
Mar 16, 2023

Conversation

joseivanlopez
Copy link
Contributor

@joseivanlopez joseivanlopez commented Feb 20, 2023

Problem

The storage client was already adapted for dealing with the iSCSI D-Bus API recently shipped by the storage sevice, see:

But there is no UI yet for configuring iSCSI.

Solution

Add a new page for configuring iSCSI.

Testing

  • Tested manually

NOTE: The UI was heavily tested in a manual way. Unit tests will come soon in a separate PR, once we have finished more urgent tasks.

Screenshots

Show/Hide

Screenshot from 2023-03-16 13-20-06

Screenshot from 2023-03-16 13-20-19

Screenshot from 2023-03-16 13-20-34

Screenshot from 2023-03-16 13-20-52

Screenshot from 2023-03-16 13-21-25

@coveralls
Copy link

coveralls commented Feb 23, 2023

Coverage Status

Coverage: 76.205% (-4.3%) from 80.523% when pulling fed9904 on iscsi-ui into 0209d74 on master.

@joseivanlopez joseivanlopez force-pushed the iscsi-ui branch 3 times, most recently from 911c479 to eb4269d Compare March 7, 2023 23:33
@joseivanlopez joseivanlopez force-pushed the iscsi-ui branch 2 times, most recently from 5c78dd5 to 74f0681 Compare March 14, 2023 15:50
@joseivanlopez joseivanlopez force-pushed the iscsi-ui branch 5 times, most recently from c6810d1 to ef9cb41 Compare March 16, 2023 13:05
@joseivanlopez joseivanlopez marked this pull request as ready for review March 16, 2023 13:06
Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

LGTM

service/lib/dinstaller/storage/iscsi/manager.rb Outdated Show resolved Hide resolved
web/src/client/storage.js Outdated Show resolved Hide resolved
web/src/client/storage.js Outdated Show resolved Hide resolved
web/src/components/storage/iscsi/AuthFields.jsx Outdated Show resolved Hide resolved
Comment on lines +29 to +49
const RowActions = ({ actions, id, ...props }) => {
const actionsToggle = (props) => (
<DropdownToggle
id={id}
aria-label="Actions"
toggleIndicator={null}
isDisabled={props.isDisabled}
onToggle={props.onToggle}
>
<Icon name="more_vert" size="24" />
</DropdownToggle>
);

return (
<ActionsColumn
items={actions}
actionsToggle={actionsToggle}
{...props}
/>
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder: As discussed off-line, in future iterations we should extract this to a common component or similar. It's used here, at users and in another place I do not remember right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is implemeted by ConnectionsTable, FirstUser, RootAuthMethods, InitiatorPresenter and NodesPresenter.

web/src/components/storage/iscsi/LoginForm.jsx Outdated Show resolved Hide resolved
web/src/components/storage/iscsi/NodeStartupOptions.js Outdated Show resolved Hide resolved
web/src/index.js Outdated Show resolved Hide resolved
web/src/utils.js Show resolved Hide resolved
};

export default function DiscoverForm({ onSubmit: onSubmitProp, onCancel }) {
const [savedData, setSavedData] = useLocalStorage("dinstaller-iscsi-discovery", {});
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: most probably, we should place the prefix globally

Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@joseivanlopez joseivanlopez merged commit 817ebd9 into master Mar 16, 2023
@joseivanlopez joseivanlopez deleted the iscsi-ui branch March 16, 2023 15:22
@joseivanlopez joseivanlopez mentioned this pull request Mar 16, 2023
@joseivanlopez joseivanlopez mentioned this pull request Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants