-
Notifications
You must be signed in to change notification settings - Fork 990
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
Fixes #36626 - safe checks for the pxe_loader #9785
Conversation
Issues: #36626 |
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.
Null vs empty string checks strikes again. They so often cause problems.
Looks like it also fixes a failure in the method pxe_loader_efi?
below it, which has the exact same issue as the provisioning template.
A quick look through the code suggests it's always assumed to be a string, or it uses .present?
which should deal with empty strings.
That makes me think this is the correct fix.
As as I say that, I do see test failures are related. It probably breaks things with inheritance. |
c839969
to
3e1949c
Compare
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 have a feeling that the PXE loader should be a proper field on the OS model instead of just a parameter. Dealing with strings here is messy.
@@ -1,7 +1,11 @@ | |||
module PxeLoaderSuggestion | |||
extend ActiveSupport::Concern | |||
def suggest_default_pxe_loader |
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 method doesn't guarantee that self.pxe_loader
is set to a string. Was that the intention?
I think this now also no longer returns self.pxe_loader
at all, unless it is set via the preferred_loader. It doesn't look like that's used so perhaps the API should be formalized to always return nil?
Another difference is that if the user set the pxe-loader parameter to the empty string it will now ignore it and use the preferred loader. I'm not sure if that's common or even valid, but certainly a difference.
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 method doesn't guarantee that self.pxe_loader is set to a string. Was that the intention?
Nope, fixed and added test.
Another difference is that if the user set the pxe-loader parameter to the empty string it will now ignore it and use the preferred loader.
Good point, but how many CUs (if any) did it, hard to say ...
@ekohl any other comments or can we get this in? |
I find this very difficult to review, because it's a part of Foreman I don't know very well. I'm not even sure how it is exactly intended to function. Could you rebase so we get an up to date packit build, including the whole history? Then I can try it out easily. |
Actually, I can update your branch so I did a trivial rebase. |
@host.pxe_loader returns empty string if it's not set on the host or the hostgroup. This fix issue in render phase when @host.pxe_loader is nil and include? is called. Create host form now require to select a PXE loader, None or something else. Same as in the API.
Another report from CU: https://projects.theforeman.org/issues/36832 |
Closing in favor of #9993 |
@host.pxe_loader returns empty string if it's
not set on the host or the hostgroup.
This fix issue in render phase when
@host.pxe_loader is nil and include? is called.
Create host form now require to select a PXE loader, None or something else. Same as in the API.