-
Notifications
You must be signed in to change notification settings - Fork 108
Boot loader ref file changes #4505
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReorganizes and consolidates boot loader reference documentation by replacing the ipxe-specific reference module with more generic HTTP and shared TFTP/HTTP root reference modules and wiring them into the main bootloader binary location assembly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
The PR preview for cdfce8a is available at theforeman-foreman-documentation-preview-pr-4505.surge.sh The following output files are affected by this PR: |
Co-authored-by: Maximilian Kolb <[email protected]>
|
Ready for review. I see the conflict, but it doesn't show in the web editor, and I don't see it on my command line either. Not sure why it's a conflict either, I renamed the |
Lennonka
left a comment
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.
Left you a couple of suggestions.
guides/common/modules/con_bootloader-binary-location-overview.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_bootloader-binary-location-overview.adoc
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,107 @@ | |||
| :_mod-docs-content-type: REFERENCE | |||
|
|
|||
| [id="ref_http-bootloaders_{context}"] | |||
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.
| [id="ref_http-bootloaders_{context}"] | |
| [id="http-based-boot-loaders"] |
Unless we plan to re-use, there's no reason to include _{context}
Please, make the file name match the ID.
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.
At least remove the ref_ from the ID, please.
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.
Will do, just want to check with eng if the name change I made is correct.
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.
@Lennonka , we discussed the naming with @stejskalleos . I had changed the original name from "iPXE / Grub2 UEFI HTTP(S) boot loaders". Leos pointed out that these boot loaders are not based on HTTP, they only use the protocol.
I am considering using "HTTP-compatible" or "HTTP-backed". Leos suggested you might have a better idea perhaps :) WDYT?
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.
Maybe "over HTTP"?
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.
That could work but I'd prefer an adjectival phrase here, would make it easiest to name the category and refer to it in later text.
…adoc Co-authored-by: Lena Ansorgová <[email protected]>
…adoc Co-authored-by: Lena Ansorgová <[email protected]>
Co-authored-by: Lena Ansorgová <[email protected]>
Co-authored-by: Lena Ansorgová <[email protected]>
stejskalleos
left a comment
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.
Looking good, great work!
Added a few suggestions in the comments; feel free to ask if anything is unclear.
guides/common/modules/con_bootloader-binary-location-overview.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_bootloader-binary-location-overview.adoc
Outdated
Show resolved
Hide resolved
| [role="_abstract"] | ||
| In addition to the TFTP-based PXE mechanism, {Project} supports boot methods that use HTTP. | ||
| HTTP-based boot methods use the *HTTPBoot* module of {SmartProxy} to deliver boot loaders and installation files. | ||
| HTTP-based boot methods in {Project} are *iPXE*, *iPXE chainloading*, and *Grub2 UEFI HTTP Boot*. |
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.
And UEFI HTTPboot
To explain, we have a naming problem here.
HTTPboot in Smart Proxy is not UEFI HTTP boot.
Both use HTTP (duh), but they are two different features from two different products.
HTTPboot in Smart proxy enables the HTTP protocol for provisioning, so Grub2, iPXE & UEFI (directly) can get the files from SP.
HTTP boot is also a feature of UEFI. Sometimes it's mentioned as HTTP boot, sometimes it's HTTPBoot UEFI, and sometimes both.
It retrieves the files directly from Smart Proxy, skipping other bootloaders, like Grub2.
You can boot any custom bootloader, provided it is a valid efi binary, and UEFI knows how to boot it.
^^ But I'm not 100% sure here, because I've never tried that.
Summoning @lzap , could you please confirm that I'm correct?
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.
Updated after our discussion
What changes are you introducing?
Further adjust newly added files for downstream docs
Why are you introducing these changes? (Explanation, links to references, issues, etc.)
Conclusion of #4422
Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
Contributor checklists
Please cherry-pick my commits into:
Summary by Sourcery
Update bootloader reference documentation for HTTP and shared TFTP/HTTP usage and remove obsolete iPXE-specific module.
Documentation: