-
Notifications
You must be signed in to change notification settings - Fork 1k
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 #36691 - use 'connectefi scsi' by default #9810
Conversation
Issues: #36691 |
[test unit] |
app/views/unattended/provisioning_templates/snippet/pxegrub2_chainload.erb
Show resolved
Hide resolved
3ea4dd2
to
5976312
Compare
Failed tests are unrelated to this 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.
Cross linking: #10118
app/views/unattended/provisioning_templates/snippet/pxegrub2_chainload.erb
Outdated
Show resolved
Hide resolved
ca2358c
to
d12176d
Compare
Many tests failed but I think they are not related to this PR. |
There was the idea from @goarsna to let GRUB2 find out if |
Having
GRUB config:
Means if we enable it by default and the module ...
|
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.
Can we add a tests file from my PR here too to have some coverage?
app/views/unattended/provisioning_templates/snippet/pxegrub2_chainload.erb
Outdated
Show resolved
Hide resolved
app/views/unattended/provisioning_templates/snippet/pxegrub2_chainload.erb
Outdated
Show resolved
Hide resolved
b265475
to
0bfd657
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.
Small nitpick and we're done
app/views/unattended/provisioning_templates/snippet/pxegrub2_chainload.erb
Outdated
Show resolved
Hide resolved
app/views/unattended/provisioning_templates/snippet/pxegrub2_chainload.erb
Outdated
Show resolved
Hide resolved
app/views/unattended/provisioning_templates/snippet/pxegrub2_chainload.erb
Outdated
Show resolved
Hide resolved
28f1281
to
158fab2
Compare
Looks like the tests are green. The other option with truthy did not work because the value can be scsi or pciroot, too |
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'd be ok with this if it was verified by someone, but slightly concerned the test failures could be related
app/views/unattended/provisioning_templates/snippet/pxegrub2_chainload.erb
Outdated
Show resolved
Hide resolved
I get the impression our CI disagrees. It says Invalid command and fails with Out of Memory on a grub check. So either the grub check is incorrect, or it breaks on non-patched grub. |
Co-Authored-by: Shimon Shtein <[email protected]> Co-Authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
Thank you very much @ekohl and grub-script-check 17:38 $ LANG=C grub-script-check unit/foreman/renderer/snapshots/ProvisioningTemplate/PXEGrub2/PXEGrub2_default_local_boot.host4dhcp.snap.txt |
All tests are green, @ekohl if you okay I'm okay to merge |
Yay for CI. Verifying snapshots with external validator tools has proven useful. |
I'm a bit worried this might not work as expected for another than global host parameter if any. To my knowledge, the including templates (global default and local boot) are rendered once and the same config is deployed to all SmartProxies (AKA the Build PXE default button). In other words, the scope in which the snippet is rendered, is not a host, but it's the Foreman server itself. I'd be more than happy to be proven wrong. |
Partly true. The "local boot" template is host scoped and gets rendered whenever a host's build state changes. If you render non-host scoped global template (Build PXE default button), the Two things here:
|
Thank you so much for the clarification |
No description provided.