Skip to content

Conversation

@stanek-michal
Copy link
Contributor

@stanek-michal stanek-michal commented Jun 13, 2025

needs review, arm64 not properly tested yet (getting ERANGE)

issue: #150

@stanek-michal stanek-michal requested a review from a team as a code owner June 13, 2025 03:12
@haesbaert
Copy link
Collaborator

I did some testing and it works.

You can remove the arm64 support for now, the others don't support it yet, we can add all of them together as a second step.

Also please make sure to have the same command line order as krun-fedora.sh and krun-rhel.sh
I'm a bit nitpicky with "everything looking the same", this is the usage for the other one:

eru:quark: ./krun-fedora.sh
usage: krun-fedora.sh initramfs.gz FEDORAVERSION command

I'm fine with updating them all to be more verbose like the one here (usage should still be lower case though).

@haesbaert
Copy link
Collaborator

Also please make sure to have the same command line order as krun-fedora.sh and krun-rhel.sh I'm a bit nitpicky with "everything looking the same", this is the usage for the other one:

Uh now I realize the order is the same, if you remove the arch option, but that is too weird, an optional parameter in between two required ones, how about you move the arch to a -a amd64? with this we can also deal with the other ones later.

@haesbaert
Copy link
Collaborator

Some more points

  • The temporary files are not deleted, they should
  • Can you hide the [INFO] prints under a -v, we can add the same later on the other ones.
  • Indentation should be tabs, not two spaces

@stanek-michal
Copy link
Contributor Author

stanek-michal commented Jun 14, 2025

thanks for the review, I fixed whitespace, style, logs, usage, args, cleaning up tmp on success path, some other stuff

@haesbaert
Copy link
Collaborator

thanks for the review, I fixed whitespace, style, logs, usage, args, cleaning up tmp on success path, some other stuff

I've fixed some more things, basically made them a bit more similar to rhel/fedora, and added the tests to the CI, you can see on the build there.

I'm ok with the current state, if you're ok with it too, just squash it into a nice commit message (we're trying to do proper commit messages once they make it to the tree), and you can merge.

Add krun-ubuntu.sh which downloads and extracts kernel image
from official Ubuntu mirrors, then hands it off to generic krun.sh.

Handle both compressed and uncompressed kernels.

Support 20.04, 22.04, 24.04, 25.04, amd64 only for now.

Add Ubuntu to buildkite CI.

Enhance all distro scripts with better logging and error handling.
@stanek-michal
Copy link
Contributor Author

Looks great, I squashed the commits. You can ACK and merge

@haesbaert haesbaert merged commit 3908b43 into main Jun 16, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants