Skip to content
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 #36847 - Rename initrd for Debian #9870

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

obol89
Copy link
Contributor

@obol89 obol89 commented Oct 20, 2023

initrd in official Debian repositories has .gz extension, not .img as it is in the template currently.

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@ekohl
Copy link
Member

ekohl commented Oct 20, 2023

ok to test

I've verified the URLs provided in https://projects.theforeman.org/issues/36847 so I think this is correct.

sbernhard
sbernhard previously approved these changes Oct 30, 2023
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failures are relevant: the snapshots need to be updated.

@sbernhard
Copy link
Contributor

sbernhard commented Dec 5, 2023

@obol89
Copy link
Contributor Author

obol89 commented Dec 14, 2023

Can you please update the snapshots @obol89

It should only be about these 2 files. Should be easys to update them manually on disk without recording the snapshots completely:

https://github.com/theforeman/foreman/blob/develop/test/unit/foreman/renderer/snapshots/ProvisioningTemplate/iPXE/Preseed_default_iPXE.ubuntu4dhcp.snap.txt https://github.com/theforeman/foreman/blob/develop/test/unit/foreman/renderer/snapshots/ProvisioningTemplate/iPXE/Preseed_default_iPXE.debian4dhcp.snap.txt

I just updated the files. I'm not sure if I did it correctly from the merge request / github perspective, but please let me know if there is anything else to do.

@sbernhard
Copy link
Contributor

Ready to merge @ekohl ?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking versions:

So all supported Debian versions have this.

It's also weird we have a line initrd below it which is correct. Looks like the specific line was introduced in 91de027. That came from theforeman/community-templates@8deda37. In theforeman/community-templates#496 it is explicitly discussed, but I suppose nobody verified it for Debian.

@@ -5,7 +5,7 @@ echo Trying to ping DNS: ${netX/dns}
ping --count 1 ${netX/dns} || echo Ping to DNS failed or ping command not available.


kernel http://ftp.debian.org/debian/dists/wheezy/main/installer-amd64/current/images/netboot/debian-installer/amd64/linux initrd=initrd.img interface=auto url=http://foreman.example.com/unattended/provision ramdisk_size=10800 root=/dev/rd/0 rw auto BOOTIF=01-${netX/mac:hexhyp} auto=true netcfg/disable_dhcp=false locale=en_US hostname=snapshot-ipv4-dhcp-deb10 domain=snap.example.com
kernel http://ftp.debian.org/debian/dists/wheezy/main/installer-amd64/current/images/netboot/debian-installer/amd64/linux initrd=initrd.gz interface=auto url=http://foreman.example.com/unattended/provision ramdisk_size=10800 root=/dev/rd/0 rw auto BOOTIF=01-${netX/mac:hexhyp} auto=true netcfg/disable_dhcp=false locale=en_US hostname=snapshot-ipv4-dhcp-deb10 domain=snap.example.com
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should really update our tests to have something newer than Wheezy, but that's outside the scope of this PR so I opened #9972.

@@ -36,7 +36,7 @@ ping --count 1 ${netX/dns} || echo Ping to DNS failed or ping command not availa
<% kernel = boot_files_uris[0] -%>
<% initrd = boot_files_uris[1] -%>

kernel <%= kernel %> initrd=initrd.img interface=auto url=<%= url %> ramdisk_size=10800 root=/dev/rd/0 rw auto <%= snippet("preseed_kernel_options", variables: {ipxe_net: true}).strip %>
kernel <%= kernel %> initrd=initrd.gz interface=auto url=<%= url %> ramdisk_size=10800 root=/dev/rd/0 rw auto <%= snippet("preseed_kernel_options", variables: {ipxe_net: true}).strip %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this use the initrd variable that's defined on line 37? Shouldn't it be:

Suggested change
kernel <%= kernel %> initrd=initrd.gz interface=auto url=<%= url %> ramdisk_size=10800 root=/dev/rd/0 rw auto <%= snippet("preseed_kernel_options", variables: {ipxe_net: true}).strip %>
kernel <%= kernel %> initrd=<%= initrd %> interface=auto url=<%= url %> ramdisk_size=10800 root=/dev/rd/0 rw auto <%= snippet("preseed_kernel_options", variables: {ipxe_net: true}).strip %>

I'd expect it to then correctly use this code:

PXEFILES = {:kernel => "linux", :initrd => "initrd.gz"}

@@ -5,7 +5,7 @@ echo Trying to ping DNS: ${netX/dns}
ping --count 1 ${netX/dns} || echo Ping to DNS failed or ping command not available.


kernel http://archive.ubuntu.com/ubuntu/dists/focal/main/installer-amd64/current/images/netboot/ubuntu-installer/amd64/linux initrd=initrd.img interface=auto url=http://foreman.example.com/unattended/provision ramdisk_size=10800 root=/dev/rd/0 rw auto BOOTIF=01-${netX/mac:hexhyp} console-setup/ask_detect=false console-setup/layout=USA console-setup/variant=USA keyboard-configuration/layoutcode=us localechooser/translation/warn-light=true localechooser/translation/warn-severe=true netcfg/disable_dhcp=false locale=en_US hostname=snapshot-ipv4-dhcp-ubuntu18 domain=snap.example.com
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confusingly enough, this combination is never valid. For Focal you only have legacy-images and that directory only has boot.img.gz, not initrd.*.

@github-actions github-actions bot added the Stale label Apr 2, 2024
Copy link

Thank you for your contribution! This PR has been inactive for 3 months, closing for now. Feel free to reopen when you return to it. This is an automated process.

@ekohl
Copy link
Member

ekohl commented Apr 26, 2024

I'm going to reopen this because I think it's still needed.

@ekohl ekohl reopened this Apr 26, 2024
@sbernhard
Copy link
Contributor

[test]

@ekohl
Copy link
Member

ekohl commented May 10, 2024

I opened #10156 with #9870 (comment) and resolved the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants