-
Notifications
You must be signed in to change notification settings - Fork 8
Add uki-serve command to pxe boot uki files #276
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
==========================================
- Coverage 31.13% 29.21% -1.93%
==========================================
Files 22 24 +2
Lines 3533 3769 +236
==========================================
+ Hits 1100 1101 +1
- Misses 2254 2489 +235
Partials 179 179 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Itxaka <[email protected]>
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.
Pull Request Overview
This PR adds a new uki-pxe
command to serve UKI boot artifacts over PXE/DHCP/TFTP/HTTP, embeds an EFI key enroller in constants, and registers the CLI entry.
- Introduce
ServeUkiPXE
inpkg/utils/pxeUki.go
to handle DHCP, PXE, TFTP, and HTTP serving for UKI. - Embed
efi-key-enroller.efi
asEfiKeyEnroller
inpkg/constants/constants.go
. - Add
uki-pxe
CLI command (internal/cmd/pxeUki.go
) and register it in the app (internal/cmd/app.go
).
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 9 comments.
File | Description |
---|---|
pkg/utils/pxeUki.go | Implement ServeUkiPXE and related protocol handlers |
pkg/constants/constants.go | Embed EFI key enroller binary as EfiKeyEnroller |
internal/cmd/pxeUki.go | Define uki-pxe CLI command with flags and action |
internal/cmd/app.go | Register the uki-pxe command in the main CLI application |
Files not reviewed (2)
- Dockerfile: Language not supported
- renovate.json: Language not supported
Comments suppressed due to low confidence (1)
pkg/utils/pxeUki.go:21
- Add unit or integration tests for
ServeUkiPXE
, covering each protocol handler (DHCP, PXE, TFTP, HTTP).
func ServeUkiPXE(keydir, isoFile string, log types.KairosLogger) error {
Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
This PR adds the uki-serve command to support serving PXE boot files for UKI images. Key changes include:
- Adding a new utility package (pkg/utils/pxeUki.go) that implements PXE, DHCP, TFTP, and HTTP servers for booting.
- Including a new embedded constant (EfiKeyEnroller) in pkg/constants/constants.go.
- Introducing a new CLI command (internal/cmd/pxeUki.go) and updating the application command registry (internal/cmd/app.go).
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
pkg/utils/pxeUki.go | New implementation of server routines for PXE boot UKI files. |
pkg/constants/constants.go | Adds the EfiKeyEnroller constant for EFI key enroller. |
internal/cmd/pxeUki.go | New CLI command for starting the PXE boot server for UKI. |
internal/cmd/app.go | Registers the new uki-pxe command with the application. |
Files not reviewed (2)
- Dockerfile: Language not supported
- renovate.json: Language not supported
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Itxaka <[email protected]>
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.
Pull Request Overview
This PR introduces a new uki-pxe
command and associated utilities to serve PXE (DHCP/PXE/TFTP) and HTTP endpoints for UKI boot workflows, including embedding an EFI key enroller.
- Add
ServeUkiPXE
utility to run DHCP, PXE, TFTP, and HTTP servers for UKI boot. - Embed the EFI key enroller binary and expose it via TFTP/HTTP.
- Register the new
uki-pxe
CLI command and wire it into the application.
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
pkg/utils/pxeUki.go | Implement ServeUkiPXE with serveDHCP , servePXE , serveTFTP , and serveHTTP . |
pkg/constants/constants.go | Embed efi-key-enroller.efi as EfiKeyEnroller . |
internal/cmd/pxeUki.go | Define UkiPXECmd CLI command for serving PXE UKI files. |
internal/cmd/app.go | Register UkiPXECmd in the command list. |
Files not reviewed (2)
- Dockerfile: Language not supported
- renovate.json: Language not supported
Comments suppressed due to low confidence (2)
pkg/utils/pxeUki.go:1
- [nitpick] The file name
pxeUki.go
uses camelCase. Go conventions recommend lowercase with underscores (e.g.,pxe_uki.go
) to match package file naming style.
package utils
pkg/utils/pxeUki.go:23
- This new
ServeUkiPXE
function and its sub-components (DHCP, PXE, TFTP, HTTP servers) lack accompanying unit or integration tests. Adding tests will help validate its behavior under various network scenarios.
func ServeUkiPXE(keydir, isoFile string, log types.KairosLogger) error {
Signed-off-by: Itxaka <[email protected]>
Remove check for dhcp in the PXE server Signed-off-by: Itxaka <[email protected]>
Signed-off-by: Itxaka <[email protected]>
pkg/utils/pxeUki.go
Outdated
}) | ||
|
||
http.Handle("/", handler) | ||
listenAddr := os.Getenv("HTTP_LISTEN_ADDR") |
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 better to have this as a cmd flag? 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.
Umm this was added by copilot but it's wrong, http server is harcoded to run in there I think as the whole thing doesn't deal with different http ports at all. So I think this need to be dropped
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.
just small nit, didn't tested but changes looks good
Co-authored-by: Ettore Di Giacinto <[email protected]>
Signed-off-by: Itxaka <[email protected]>
Part of kairos-io/kairos#1887