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

Rhel7 branch non ibft boot #1437

Merged

Conversation

rvykydal
Copy link
Contributor

No description provided.

@rvykydal
Copy link
Contributor Author

rvykydal commented Apr 16, 2018

The patch for improving the UI feedback is dealing with propagating of storage check errors into UI where it seems easy to make a regression by trying to fix it too much, so (given it is 6th minor release) I decided to play conservative and safe here. I'll add inline comments for explanation.
It should improve the messages for

if errors:
msg = msg + ": " + "; ".join(errors)
self.errors = errors
raise BootLoaderError(msg)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This (the exception msg) gets to the error detail dialog for autopartitiong case.

msg = "Failed to find a suitable stage1 device"
if errors:
msg = msg + ": " + "; ".join(errors)
self.errors = errors
Copy link
Contributor Author

@rvykydal rvykydal Apr 16, 2018

Choose a reason for hiding this comment

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

The self.errors (self is a Bootloader instance) will be gathered when creating error details for custom partitioning.
Here the errors are collected locally in errors variable because is_valid_stage1_device clears the self.errors when called so older errors from the loop above would be lost.

Copy link
Member

@jkonecny12 jkonecny12 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 but I have one question though.

Isn't the --nonibftiscsiboot too long, what is wrong with the --nonibftboot name?

@rvykydal
Copy link
Contributor Author

@jkonecny12
Non-ibft boot is allowed by default (having iscsi target attached from ibft does not mean one has to boot from it).
Non-ibft iscsi boot is allowed only when using this option.

@jkonecny12
Copy link
Member

jenkins, test this please

1 similar comment
@jkonecny12
Copy link
Member

jenkins, test this please

@rvykydal
Copy link
Contributor Author

Added CI fixes (style guide: bootloader -> boot loader)

@jkonecny12
Copy link
Member

jenkins, test this please

The option allow placing bootloader on non-iBFT iSCSI disks

Resolves: rhbz#1562301
@rvykydal
Copy link
Contributor Author

@poncovka
Vendy, does the approach in the third patch look better?
If so, I'll squash it with the second patch.

Copy link
Contributor

@poncovka poncovka 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 now. Thank you!

@rvykydal rvykydal merged commit b9856e3 into rhinstaller:rhel7-branch Apr 27, 2018
@rvykydal
Copy link
Contributor Author

Ported to master with 0236294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants