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

webui: add support for ISCSI storage for installation destination #5283

Closed
wants to merge 2 commits into from

Conversation

KKoukiou
Copy link
Contributor

@KKoukiou KKoukiou commented Oct 27, 2023

  • Blocked on: webui: apis: dont repeat yourself #5281
  • Needs more test coverage
  • Needs reverse chap support in the UI (followup)
  • Needs better presentation of the error messages - now presenting directly what we get from backend - (followup)
  • Needs designer review - the current design is self inspired (followup) - @garrett is in sick leave
iscsi-storage-anaconda.mp4

@pep8speaks
Copy link

pep8speaks commented Oct 27, 2023

Hello @KKoukiou! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 46:100: E501 line too long (107 > 99 characters)
Line 51:100: E501 line too long (109 > 99 characters)
Line 103:100: E501 line too long (102 > 99 characters)

Line 156:100: E501 line too long (142 > 99 characters)

Comment last updated at 2023-10-31 09:18:11 UTC

@KKoukiou KKoukiou added the blocked Don't merge this pull request! label Oct 27, 2023
Increase timeout for wait_boot method, as nm-manager-wait-online-initrd
takes super long time to start.
* @param {Object} object
* @returns {Object} The converted object
*/
export const objectToDBus = (object) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should indicate in the name that the function produces string variants.

Copy link
Contributor

@vojtechtrefny vojtechtrefny 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 to me in general.

One note: In Gtk UI the last "step" of ISCSI login shows all disks/targets that are now available and user can choose which ones will be added to the disk selection. With the WebUI the targets are shown but then on the Destination page "nothing happens" and it was a little bit confusing for me.

Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

Looks great to me.

@KKoukiou
Copy link
Contributor Author

KKoukiou commented Nov 1, 2023

Looks good to me in general.

One note: In Gtk UI the last "step" of ISCSI login shows all disks/targets that are now available and user can choose which ones will be added to the disk selection. With the WebUI the targets are shown but then on the Destination page "nothing happens" and it was a little bit confusing for me.

I see your point. I guess the designer would have figured some better user experience. Maybe after closing the login dialog I can show a notification above the disk selector that now new disks are available, and mention the disk names.

@jstodola
Copy link
Contributor

jstodola commented Nov 1, 2023

Katerina, can you also add an end2end test to cover the whole installation and boot process, please?

@KKoukiou
Copy link
Contributor Author

KKoukiou commented Nov 1, 2023

Katerina, can you also add an end2end test to cover the whole installation and boot process, please?

Sure. This checks that the iscsi device is selectable only, you 're right. I honestly did not make the test run the whole installation on the purpose, as believed that anaconda does not have any specific to iscsi code, after the device is selected. I see now that i was probably wrong, storage/bootloader/base.py for example has a lot of special cases for iscsi.

I will adjust the test to run full installation.

@KKoukiou
Copy link
Contributor Author

rhinstaller/anaconda-webui#31 moved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Don't merge this pull request! f40 webui
Development

Successfully merging this pull request may close these issues.

5 participants