-
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 #9993
Fixes #36626 - safe checks for the pxe_loader #9993
Conversation
afe64c3
to
403b25c
Compare
app/views/unattended/provisioning_templates/discovery/debian_kexec.erb
Outdated
Show resolved
Hide resolved
<% if @host.pxe_loader.include?('UEFI') || @host.pxe_loader.include?('BIOS') -%> | ||
<%- if @host.pxe_loader.include?('UEFI') -%> | ||
<% if pxe_loader.include?('UEFI') || pxe_loader.include?('BIOS') -%> | ||
<%- if pxe_loader.include?('UEFI') -%> |
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 wonder if this should be@host.pxe_loader_efi?
(and modify that to handle nil
values for 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.
Isn't that something we decided not to do in https://github.com/theforeman/foreman/pull/9785/files?
I'm for having it explicitly in the template, so users know what's going on.
403b25c
to
be4a699
Compare
be4a699
to
998d5fc
Compare
Rebased on the latest I'm still trying to figure out why Katello is failing; it looks like it's failing for every PR. |
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.
Looks like Katello tests are stuck for some reason. Integration tests don't seem related too.
All in all LGTM, but I'll leave the final decision to @ekohl.
It's been a month since the last approval and no concerns have been raised; I'm merging the PR. |
No description provided.